From 3c7ef0bb981d75cd38e9da82dad9bac20de23e8d Mon Sep 17 00:00:00 2001 From: Arun Siluvery Date: Thu, 14 Nov 2024 15:20:30 +0000 Subject: [PATCH 01/24] Add unique constraint for licence and good in GoodOnLicence model It should not be possible to have the same good_on_application twice on a given licence. --- .../tests/test_finalise_application.py | 15 +++++++++++++++ ...0020_alter_goodonlicence_unique_together.py | 18 ++++++++++++++++++ api/licences/models.py | 3 +++ 3 files changed, 36 insertions(+) create mode 100644 api/licences/migrations/0020_alter_goodonlicence_unique_together.py diff --git a/api/applications/tests/test_finalise_application.py b/api/applications/tests/test_finalise_application.py index 30a5ca95a6..3b48a96292 100644 --- a/api/applications/tests/test_finalise_application.py +++ b/api/applications/tests/test_finalise_application.py @@ -1,6 +1,7 @@ import pytest from datetime import datetime +from django.db.utils import IntegrityError from django.urls import reverse from django.utils import timezone from parameterized import parameterized @@ -791,3 +792,17 @@ def test_approve_quantity_greater_than_applied_for_failure(self): response_data, {"errors": {f"quantity-{self.good_on_application.id}": [strings.Licence.INVALID_QUANTITY_ERROR]}}, ) + + def test_goods_unique_on_licence(self): + + licence = StandardLicenceFactory(case=self.standard_application, status=LicenceStatus.DRAFT) + + with pytest.raises(IntegrityError) as e: + GoodOnLicence.objects.create( + good=self.good_on_application, licence=licence, usage=0.0, quantity=5.0, value=100.00 + ) + GoodOnLicence.objects.create( + good=self.good_on_application, licence=licence, usage=0.0, quantity=5.0, value=100.00 + ) + + self.assertIn(f"({str(licence.id)}, {str(self.good_on_application.id)}) already exists", e.value.args[0]) diff --git a/api/licences/migrations/0020_alter_goodonlicence_unique_together.py b/api/licences/migrations/0020_alter_goodonlicence_unique_together.py new file mode 100644 index 0000000000..21b0f9496d --- /dev/null +++ b/api/licences/migrations/0020_alter_goodonlicence_unique_together.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.16 on 2024-11-14 12:31 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("applications", "0084_standardapplication_subject_to_itar_controls"), + ("licences", "0019_auto_20210506_0340"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="goodonlicence", + unique_together={("licence", "good")}, + ), + ] diff --git a/api/licences/models.py b/api/licences/models.py index 02738e5134..cf8e2369cf 100644 --- a/api/licences/models.py +++ b/api/licences/models.py @@ -158,3 +158,6 @@ class GoodOnLicence(TimestampableModel): usage = models.FloatField(null=False, blank=False, default=0) quantity = models.FloatField(null=False, blank=False) value = models.DecimalField(max_digits=15, decimal_places=2, null=False, blank=False) + + class Meta: + unique_together = ("licence", "good") From 2a52e1f0bb1c31fcbc0278b79307577dabe66540 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Mon, 18 Nov 2024 16:35:24 +0000 Subject: [PATCH 02/24] create simple endpoint for dbt platform healthcheck --- api/conf/settings.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/api/conf/settings.py b/api/conf/settings.py index 581d9011e5..2bfa26115c 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -126,6 +126,7 @@ "health_check", "health_check.contrib.celery", "health_check.contrib.celery_ping", + "health_check.contrib.migrations", "django_audit_log_middleware", "lite_routing", "api.appeals", @@ -139,12 +140,17 @@ if not IS_ENV_DBT_PLATFORM: INSTALLED_APPS += [ - "health_check.db", "health_check.cache", + "health_check.db", "health_check.storage", - "health_check.contrib.migrations", ] +HEALTH_CHECK = { + "SUBSETS": { + "startup-liveness-probe": ["MigrationsHealthCheck"], + }, +} + MOCK_VIRUS_SCAN_ACTIVATE_ENDPOINTS = env("MOCK_VIRUS_SCAN_ACTIVATE_ENDPOINTS") if MOCK_VIRUS_SCAN_ACTIVATE_ENDPOINTS: From 03aa76d025df4a524cca3077bba51e2c23ed31d4 Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Mon, 18 Nov 2024 14:23:46 +0000 Subject: [PATCH 03/24] Add countries endpoint initial commit --- api/data_workspace/v2/serializers.py | 9 +++++++++ api/data_workspace/v2/urls.py | 2 ++ api/data_workspace/v2/views.py | 12 +++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/api/data_workspace/v2/serializers.py b/api/data_workspace/v2/serializers.py index d5f48f94cc..1b3da1cb42 100644 --- a/api/data_workspace/v2/serializers.py +++ b/api/data_workspace/v2/serializers.py @@ -2,6 +2,7 @@ from api.cases.enums import LicenceDecisionType from api.cases.models import Case +from api.staticdata.countries.models import Country class LicenceDecisionSerializer(serializers.ModelSerializer): @@ -31,3 +32,11 @@ def get_decision_made_at(self, case): .earliest("created_at") .created_at ) + + +class CountrySerializer(serializers.ModelSerializer): + code = serializers.CharField(source="id") + + class Meta: + model = Country + fields = ("code", "name") diff --git a/api/data_workspace/v2/urls.py b/api/data_workspace/v2/urls.py index a8f6055a1a..50dd911495 100644 --- a/api/data_workspace/v2/urls.py +++ b/api/data_workspace/v2/urls.py @@ -10,3 +10,5 @@ views.LicenceDecisionViewSet, basename="dw-licence-decisions", ) + +router_v2.register("countries", views.CountryViewSet, basename="dw-countries") diff --git a/api/data_workspace/v2/views.py b/api/data_workspace/v2/views.py index ce28e0dd81..553a254665 100644 --- a/api/data_workspace/v2/views.py +++ b/api/data_workspace/v2/views.py @@ -10,9 +10,11 @@ from api.core.authentication import DataWorkspaceOnlyAuthentication from api.core.helpers import str_to_bool from api.data_workspace.v2.serializers import ( + CountrySerializer, LicenceDecisionSerializer, LicenceDecisionType, ) +from api.staticdata.countries.models import Country class DisableableLimitOffsetPagination(LimitOffsetPagination): @@ -23,10 +25,13 @@ def paginate_queryset(self, queryset, request, view=None): return super().paginate_queryset(queryset, request, view) -class LicenceDecisionViewSet(viewsets.ReadOnlyModelViewSet): +class BaseViewSet(viewsets.ReadOnlyModelViewSet): authentication_classes = (DataWorkspaceOnlyAuthentication,) pagination_class = DisableableLimitOffsetPagination renderer_classes = tuple(api_settings.DEFAULT_RENDERER_CLASSES) + (PaginatedCSVRenderer,) + + +class LicenceDecisionViewSet(BaseViewSet): serializer_class = LicenceDecisionSerializer def get_queryset(self): @@ -55,3 +60,8 @@ def get_queryset(self): .order_by("-reference_code") ) return queryset + + +class CountryViewSet(BaseViewSet): + serializer_class = CountrySerializer + queryset = Country.objects.all().order_by("id", "name") From 7c778b1bfc224335bea4ad42e0fc8cecc6377bde Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Mon, 18 Nov 2024 15:17:07 +0000 Subject: [PATCH 04/24] Add basic BDD tests for countries --- .../v2/tests/bdd/scenarios/countries.feature | 10 ++++++ .../v2/tests/bdd/test_countries.py | 31 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 api/data_workspace/v2/tests/bdd/scenarios/countries.feature create mode 100644 api/data_workspace/v2/tests/bdd/test_countries.py diff --git a/api/data_workspace/v2/tests/bdd/scenarios/countries.feature b/api/data_workspace/v2/tests/bdd/scenarios/countries.feature new file mode 100644 index 0000000000..2e90fe8960 --- /dev/null +++ b/api/data_workspace/v2/tests/bdd/scenarios/countries.feature @@ -0,0 +1,10 @@ +@db +Feature: Countries and Territories + +Scenario: Check that the correct country code and name is included in the extract + When I fetch the list of countries + Then the correct country code and name is included in the extract + +Scenario: Check that the correct territory code and name is included in the extract + When I fetch the list of countries + Then the correct territory code and name is included in the extract diff --git a/api/data_workspace/v2/tests/bdd/test_countries.py b/api/data_workspace/v2/tests/bdd/test_countries.py new file mode 100644 index 0000000000..8a5bc7e40e --- /dev/null +++ b/api/data_workspace/v2/tests/bdd/test_countries.py @@ -0,0 +1,31 @@ +from django.urls import reverse +import pytest +from pytest_bdd import ( + then, + when, + scenarios, +) + +scenarios("./scenarios/countries.feature") + + +@pytest.fixture() +def countries_list_url(): + return reverse("data_workspace:v2:dw-countries-list") + + +@when("I fetch the list of countries", target_fixture="countries") +def fetch_countries(countries_list_url, unpage_data): + return unpage_data(countries_list_url) + + +@then("the correct country code and name is included in the extract") +def correct_country_code_included_in_extract(countries): + example_country = {"code": "AE", "name": "United Arab Emirates"} + assert example_country in countries + + +@then("the correct territory code and name is included in the extract") +def correct_territory_code_included_in_extract(countries): + example_territory = {"code": "AE-DU", "name": "Dubai"} + assert example_territory in countries From 65a8da9892b220da2698bd6fdb97263bbba63aec Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Mon, 18 Nov 2024 15:33:40 +0000 Subject: [PATCH 05/24] Add BDD tests for adding or removing countries --- .../v2/tests/bdd/scenarios/countries.feature | 13 ++++++++ .../v2/tests/bdd/test_countries.py | 31 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/api/data_workspace/v2/tests/bdd/scenarios/countries.feature b/api/data_workspace/v2/tests/bdd/scenarios/countries.feature index 2e90fe8960..85c9f7adb8 100644 --- a/api/data_workspace/v2/tests/bdd/scenarios/countries.feature +++ b/api/data_workspace/v2/tests/bdd/scenarios/countries.feature @@ -8,3 +8,16 @@ Scenario: Check that the correct country code and name is included in the extrac Scenario: Check that the correct territory code and name is included in the extract When I fetch the list of countries Then the correct territory code and name is included in the extract + +Scenario: A new country appears in the extract when added to countries list + Given I add a new country to the countries list + When I fetch the list of countries + Then the new country appears in the extract + +Scenario: A new country disappears from the extract when removed from countries list + Given I add a new country to the countries list + When I fetch the list of countries + Then the new country appears in the extract + Given I remove the new country from the countries list + When I fetch the list of countries + Then the new country does not appear in the extract diff --git a/api/data_workspace/v2/tests/bdd/test_countries.py b/api/data_workspace/v2/tests/bdd/test_countries.py index 8a5bc7e40e..103deb3e59 100644 --- a/api/data_workspace/v2/tests/bdd/test_countries.py +++ b/api/data_workspace/v2/tests/bdd/test_countries.py @@ -1,11 +1,14 @@ from django.urls import reverse import pytest from pytest_bdd import ( + given, then, when, scenarios, ) +from api.staticdata.countries.models import Country + scenarios("./scenarios/countries.feature") @@ -14,6 +17,11 @@ def countries_list_url(): return reverse("data_workspace:v2:dw-countries-list") +@pytest.fixture() +def example_country(): + return {"id": "EX", "name": "Example", "is_eu": False} + + @when("I fetch the list of countries", target_fixture="countries") def fetch_countries(countries_list_url, unpage_data): return unpage_data(countries_list_url) @@ -29,3 +37,26 @@ def correct_country_code_included_in_extract(countries): def correct_territory_code_included_in_extract(countries): example_territory = {"code": "AE-DU", "name": "Dubai"} assert example_territory in countries + + +@given("I add a new country to the countries list") +def add_new_country_to_countries_list(example_country): + Country.objects.create(**example_country) + + +@then("the new country appears in the extract") +def new_country_appears_in_extract(countries, example_country): + new_country = {"code": example_country["id"], "name": example_country["name"]} + assert new_country in countries + + +@given("I remove the new country from the countries list") +def remove_new_country_from_countries_list(example_country): + new_country = Country.objects.get(id=example_country["id"]) + new_country.delete() + + +@then("the new country does not appear in the extract") +def new_country_does_not_appear_in_extract(countries, example_country): + new_country = {"code": example_country["id"], "name": example_country["name"]} + assert new_country not in countries From 398e1718f5c147d76b4dbabc561cd9e5914115b2 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Tue, 19 Nov 2024 11:46:07 +0000 Subject: [PATCH 06/24] added all health checks back in to all deployment environments --- api/conf/settings.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/api/conf/settings.py b/api/conf/settings.py index 2bfa26115c..f8cd5f19d4 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -124,9 +124,12 @@ "api.external_data", "api.support", "health_check", + "health_check.cache", "health_check.contrib.celery", "health_check.contrib.celery_ping", + "health_check.db", "health_check.contrib.migrations", + "health_check.storage", "django_audit_log_middleware", "lite_routing", "api.appeals", @@ -138,12 +141,6 @@ "drf_spectacular", ] -if not IS_ENV_DBT_PLATFORM: - INSTALLED_APPS += [ - "health_check.cache", - "health_check.db", - "health_check.storage", - ] HEALTH_CHECK = { "SUBSETS": { From 5e5b2bb4abdccf5f558d5bbfd58853916f495ab4 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Tue, 19 Nov 2024 12:20:20 +0000 Subject: [PATCH 07/24] add comment --- api/conf/settings.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/conf/settings.py b/api/conf/settings.py index f8cd5f19d4..34dc6b24d2 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -141,7 +141,11 @@ "drf_spectacular", ] - +""" +This takes any healthchecks added into the list and creates a callable URL for them +which will be the key corresponding to the list value. +e.g. BASEURL.com/healthcheck/startup-liveness-probe/ +""" HEALTH_CHECK = { "SUBSETS": { "startup-liveness-probe": ["MigrationsHealthCheck"], From a07532ca3bab164b0ddafd944877f91b81c47ae7 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Wed, 20 Nov 2024 15:26:25 +0000 Subject: [PATCH 08/24] health check simplified --- api/conf/settings.py | 4 +++- api/core/apps.py | 12 ++++++++++++ api/core/health_checks.py | 13 +++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 api/core/apps.py create mode 100644 api/core/health_checks.py diff --git a/api/conf/settings.py b/api/conf/settings.py index 34dc6b24d2..08a7fc2072 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -148,7 +148,7 @@ """ HEALTH_CHECK = { "SUBSETS": { - "startup-liveness-probe": ["MigrationsHealthCheck"], + "startup-liveness-probe": ["ServiceAvailableHealthCheck"], }, } @@ -302,6 +302,8 @@ TIME_TESTS = True # If True, print the length of time it takes to run each test SUPPRESS_TEST_OUTPUT = env("SUPPRESS_TEST_OUTPUT") +LITE_API_URL = env.str("LITE_API_URL", f"https://lite-api-{ENV}.london.cloudapps.digital") + # Internationalization # https://docs.djangoproject.com/en/2.1/topics/i18n/ diff --git a/api/core/apps.py b/api/core/apps.py new file mode 100644 index 0000000000..45d39184b3 --- /dev/null +++ b/api/core/apps.py @@ -0,0 +1,12 @@ +from django.apps import AppConfig + +from health_check.plugins import plugin_dir + + +class CoreAppConfig(AppConfig): + name = "api.core" + + def ready(self): + from .health_checks import ServiceAvailableHealthCheck + + plugin_dir.register(ServiceAvailableHealthCheck) diff --git a/api/core/health_checks.py b/api/core/health_checks.py new file mode 100644 index 0000000000..ac53549621 --- /dev/null +++ b/api/core/health_checks.py @@ -0,0 +1,13 @@ +from health_check.backends import BaseHealthCheckBackend +from django.http import HttpResponse + +from rest_framework import status + + +class ServiceAvailableHealthCheck(BaseHealthCheckBackend): + #: The status endpoints will respond with a 200 status code + #: even if the check errors. + critical_service = False + + def check_status(self): + return HttpResponse(status=status.HTTP_200_OK) From 69eb7177b328979805e78aa74e6c96be7d080fdf Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Wed, 20 Nov 2024 17:49:43 +0000 Subject: [PATCH 09/24] change of approach --- api/conf/settings.py | 11 ----------- api/conf/urls.py | 3 ++- api/core/apps.py | 12 ------------ api/core/health_checks.py | 13 ------------- api/healthcheck/tests/test_healthcheck.py | 9 +++++++++ api/healthcheck/views.py | 10 ++++++++++ 6 files changed, 21 insertions(+), 37 deletions(-) delete mode 100644 api/core/apps.py delete mode 100644 api/core/health_checks.py diff --git a/api/conf/settings.py b/api/conf/settings.py index 08a7fc2072..d8e3683b15 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -141,17 +141,6 @@ "drf_spectacular", ] -""" -This takes any healthchecks added into the list and creates a callable URL for them -which will be the key corresponding to the list value. -e.g. BASEURL.com/healthcheck/startup-liveness-probe/ -""" -HEALTH_CHECK = { - "SUBSETS": { - "startup-liveness-probe": ["ServiceAvailableHealthCheck"], - }, -} - MOCK_VIRUS_SCAN_ACTIVATE_ENDPOINTS = env("MOCK_VIRUS_SCAN_ACTIVATE_ENDPOINTS") if MOCK_VIRUS_SCAN_ACTIVATE_ENDPOINTS: diff --git a/api/conf/urls.py b/api/conf/urls.py index 653ea20614..5651471a1e 100644 --- a/api/conf/urls.py +++ b/api/conf/urls.py @@ -5,11 +5,12 @@ from django.conf import settings import api.core.views -from api.healthcheck.views import HealthCheckPingdomView +from api.healthcheck.views import HealthCheckPingdomView, ServiceAvailableHealthCheckView urlpatterns = [ path("healthcheck/", include("health_check.urls")), path("pingdom/ping.xml", HealthCheckPingdomView.as_view(), name="healthcheck-pingdom"), + path("service-available-check/", ServiceAvailableHealthCheckView.as_view(), name="service-available-check"), path("applications/", include("api.applications.urls")), path("assessments/", include("api.assessments.urls")), path("audit-trail/", include("api.audit_trail.urls")), diff --git a/api/core/apps.py b/api/core/apps.py deleted file mode 100644 index 45d39184b3..0000000000 --- a/api/core/apps.py +++ /dev/null @@ -1,12 +0,0 @@ -from django.apps import AppConfig - -from health_check.plugins import plugin_dir - - -class CoreAppConfig(AppConfig): - name = "api.core" - - def ready(self): - from .health_checks import ServiceAvailableHealthCheck - - plugin_dir.register(ServiceAvailableHealthCheck) diff --git a/api/core/health_checks.py b/api/core/health_checks.py deleted file mode 100644 index ac53549621..0000000000 --- a/api/core/health_checks.py +++ /dev/null @@ -1,13 +0,0 @@ -from health_check.backends import BaseHealthCheckBackend -from django.http import HttpResponse - -from rest_framework import status - - -class ServiceAvailableHealthCheck(BaseHealthCheckBackend): - #: The status endpoints will respond with a 200 status code - #: even if the check errors. - critical_service = False - - def check_status(self): - return HttpResponse(status=status.HTTP_200_OK) diff --git a/api/healthcheck/tests/test_healthcheck.py b/api/healthcheck/tests/test_healthcheck.py index 93bd374e0e..06e7f03350 100644 --- a/api/healthcheck/tests/test_healthcheck.py +++ b/api/healthcheck/tests/test_healthcheck.py @@ -64,3 +64,12 @@ def test_healthcheck_down(client, backends): b"0.23" b"\n" ) + + +def test_service_available_check(self): + backends.reset() + backends.register(HealthCheckOk) + url = reverse("service-available-check") + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) diff --git a/api/healthcheck/views.py b/api/healthcheck/views.py index dcab1569ed..e8ac852ac8 100644 --- a/api/healthcheck/views.py +++ b/api/healthcheck/views.py @@ -1,4 +1,5 @@ from health_check.views import MainView +from django.http import HttpResponse class HealthCheckPingdomView(MainView): @@ -8,3 +9,12 @@ def render_to_response(self, context, status): context["errored_plugins"] = [plugin for plugin in context["plugins"] if plugin.errors] context["total_response_time"] = sum([plugin.time_taken for plugin in context["plugins"]]) return super().render_to_response(context=context, status=status, content_type="text/xml") + + +class ServiceAvailableHealthCheckView(MainView): + def get(self, request, *args, **kwargs): + status = 200 + return self.render_to_response(status) + + def render_to_response(self, status): + return HttpResponse("STATUS OK" if status == 200 else "STATUS NOT OK", status=status) From 6728fd05a788e307a651b3f11b536f581bf7caad Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Wed, 20 Nov 2024 17:52:29 +0000 Subject: [PATCH 10/24] remove unused --- api/conf/settings.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/conf/settings.py b/api/conf/settings.py index d8e3683b15..d5d3a4cca4 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -291,8 +291,6 @@ TIME_TESTS = True # If True, print the length of time it takes to run each test SUPPRESS_TEST_OUTPUT = env("SUPPRESS_TEST_OUTPUT") -LITE_API_URL = env.str("LITE_API_URL", f"https://lite-api-{ENV}.london.cloudapps.digital") - # Internationalization # https://docs.djangoproject.com/en/2.1/topics/i18n/ From 399188afd6d1c02c29f33ead3bd659af3cc53704 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Wed, 20 Nov 2024 18:12:42 +0000 Subject: [PATCH 11/24] amend test --- api/healthcheck/tests/test_healthcheck.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/healthcheck/tests/test_healthcheck.py b/api/healthcheck/tests/test_healthcheck.py index 06e7f03350..42b16764d4 100644 --- a/api/healthcheck/tests/test_healthcheck.py +++ b/api/healthcheck/tests/test_healthcheck.py @@ -67,8 +67,6 @@ def test_healthcheck_down(client, backends): def test_service_available_check(self): - backends.reset() - backends.register(HealthCheckOk) url = reverse("service-available-check") response = self.client.get(url) From c995fc6c28ca9d593b189b830220c738bb742593 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Wed, 20 Nov 2024 18:18:20 +0000 Subject: [PATCH 12/24] amend test --- api/healthcheck/tests/test_healthcheck.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/healthcheck/tests/test_healthcheck.py b/api/healthcheck/tests/test_healthcheck.py index 42b16764d4..fafcee14d2 100644 --- a/api/healthcheck/tests/test_healthcheck.py +++ b/api/healthcheck/tests/test_healthcheck.py @@ -66,8 +66,8 @@ def test_healthcheck_down(client, backends): ) -def test_service_available_check(self): +def test_service_available_check(client): url = reverse("service-available-check") - response = self.client.get(url) + response = client.get(url) - self.assertEqual(response.status_code, 200) + assert response.status_code == 200 From eb750387294f6d3f3a279fa40bb3d963e29f31dd Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Wed, 20 Nov 2024 18:57:41 +0000 Subject: [PATCH 13/24] more testing --- api/healthcheck/tests/test_healthcheck.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/api/healthcheck/tests/test_healthcheck.py b/api/healthcheck/tests/test_healthcheck.py index fafcee14d2..65b6241ca0 100644 --- a/api/healthcheck/tests/test_healthcheck.py +++ b/api/healthcheck/tests/test_healthcheck.py @@ -66,7 +66,18 @@ def test_healthcheck_down(client, backends): ) -def test_service_available_check(client): +def test_service_available_check_broken(client, backends): + backends.reset() + backends.register(HealthCheckBroken) + url = reverse("service-available-check") + response = client.get(url) + + assert response.status_code == 200 + + +def test_service_available_check_ok(client, backends): + backends.reset() + backends.register(HealthCheckOk) url = reverse("service-available-check") response = client.get(url) From 53f618bd529ff4f8ad86d859edbd8c2ab93229a0 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Thu, 21 Nov 2024 09:02:19 +0000 Subject: [PATCH 14/24] update --- api/healthcheck/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/healthcheck/views.py b/api/healthcheck/views.py index e8ac852ac8..1fb02a3ed8 100644 --- a/api/healthcheck/views.py +++ b/api/healthcheck/views.py @@ -1,5 +1,6 @@ from health_check.views import MainView from django.http import HttpResponse +from rest_framework import status class HealthCheckPingdomView(MainView): @@ -13,8 +14,7 @@ def render_to_response(self, context, status): class ServiceAvailableHealthCheckView(MainView): def get(self, request, *args, **kwargs): - status = 200 - return self.render_to_response(status) + return self.render_to_response() - def render_to_response(self, status): - return HttpResponse("STATUS OK" if status == 200 else "STATUS NOT OK", status=status) + def render_to_response(self): + return HttpResponse(status.HTTP_200_OK) From 05353b0195c7099a0902ffae3ba779d57c1ea0bd Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Thu, 21 Nov 2024 10:03:10 +0000 Subject: [PATCH 15/24] comment added --- api/healthcheck/tests/test_healthcheck.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/api/healthcheck/tests/test_healthcheck.py b/api/healthcheck/tests/test_healthcheck.py index 65b6241ca0..34d31586f9 100644 --- a/api/healthcheck/tests/test_healthcheck.py +++ b/api/healthcheck/tests/test_healthcheck.py @@ -66,6 +66,13 @@ def test_healthcheck_down(client, backends): ) +""" +The tests below expect a 200 response whether healthchecks produce a healthy +response or not as the url is used by DBT platform pipeline to check that +the django app is alive. +""" + + def test_service_available_check_broken(client, backends): backends.reset() backends.register(HealthCheckBroken) From 554bf27a62eb5145db45ecd43848168d6437ad1a Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 13:47:52 +0000 Subject: [PATCH 16/24] Turn off silk when running tests --- api/conf/settings_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/conf/settings_test.py b/api/conf/settings_test.py index 204b5d34b1..fcaa9933c8 100644 --- a/api/conf/settings_test.py +++ b/api/conf/settings_test.py @@ -20,3 +20,9 @@ DB_ANONYMISER_AWS_REGION = "eu-west-2" DB_ANONYMISER_AWS_STORAGE_BUCKET_NAME = "anonymiser-bucket" DB_ANONYMISER_AWS_ENDPOINT_URL = None + +try: + INSTALLED_APPS.remove("silk") + MIDDLEWARE.remove("silk.middleware.SilkyMiddleware") +except ValueError: + pass From 302e3c46b2824b44c24921543384def2d7dbacc5 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 14:39:05 +0000 Subject: [PATCH 17/24] Add metadata router --- api/data_workspace/metadata/__init__.py | 0 api/data_workspace/metadata/routers.py | 65 +++++++++++++++++++ api/data_workspace/metadata/tests/__init__.py | 0 .../metadata/tests/serializers.py | 0 .../metadata/tests/test_metadata.py | 35 ++++++++++ api/data_workspace/metadata/tests/urls.py | 22 +++++++ api/data_workspace/metadata/tests/views.py | 13 ++++ 7 files changed, 135 insertions(+) create mode 100644 api/data_workspace/metadata/__init__.py create mode 100644 api/data_workspace/metadata/routers.py create mode 100644 api/data_workspace/metadata/tests/__init__.py create mode 100644 api/data_workspace/metadata/tests/serializers.py create mode 100644 api/data_workspace/metadata/tests/test_metadata.py create mode 100644 api/data_workspace/metadata/tests/urls.py create mode 100644 api/data_workspace/metadata/tests/views.py diff --git a/api/data_workspace/metadata/__init__.py b/api/data_workspace/metadata/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/api/data_workspace/metadata/routers.py b/api/data_workspace/metadata/routers.py new file mode 100644 index 0000000000..d1d9fc6042 --- /dev/null +++ b/api/data_workspace/metadata/routers.py @@ -0,0 +1,65 @@ +from rest_framework.response import Response +from rest_framework.reverse import reverse +from rest_framework.routers import DefaultRouter +from rest_framework.views import APIView + +from django.urls import ( + NoReverseMatch, + path, +) + + +class TableMetadataView(APIView): + _ignore_model_permissions = True + schema = None # exclude from schema + metadata = None + + def get(self, request, *args, **kwargs): + tables = [] + namespace = request.resolver_match.namespace + for table_metadata in self.metadata: + url_name = table_metadata["endpoint"] + if namespace: + url_name = f"{namespace}:{url_name}" + try: + url = reverse( + url_name, + args=args, + kwargs=kwargs, + request=request, + ) + except NoReverseMatch: + # Don't bail out if eg. no list routes exist, only detail routes. + continue + + tables.append( + { + "table_name": table_metadata["table_name"], + "endpoint": url, + } + ) + return Response({"tables": tables}) + + +class TableMetadataRouter(DefaultRouter): + def get_metadata_view(self, urls): + metadata = [] + list_name = self.routes[0].name + for prefix, viewset, basename in self.registry: + metadata.append( + { + "table_name": viewset.DataWorkspace.table_name, + "endpoint": list_name.format(basename=basename), + } + ) + + return TableMetadataView.as_view(metadata=metadata) + + def get_urls(self): + urls = super().get_urls() + + view = self.get_metadata_view(urls) + metadata_url = path("table-metadata/", view, name="table-metadata") + urls.append(metadata_url) + + return urls diff --git a/api/data_workspace/metadata/tests/__init__.py b/api/data_workspace/metadata/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/api/data_workspace/metadata/tests/serializers.py b/api/data_workspace/metadata/tests/serializers.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/api/data_workspace/metadata/tests/test_metadata.py b/api/data_workspace/metadata/tests/test_metadata.py new file mode 100644 index 0000000000..312be274dd --- /dev/null +++ b/api/data_workspace/metadata/tests/test_metadata.py @@ -0,0 +1,35 @@ +from django.urls import ( + include, + path, + reverse, +) + +from rest_framework.test import URLPatternsTestCase + + +class MetadataTestCase(URLPatternsTestCase): + urlpatterns = [ + path("api/", include("api.data_workspace.metadata.tests.urls")), + ] + + def setUp(self): + super().setUp() + + self.url = reverse("table-metadata") + + def test_metadata_endpoint(self): + response = self.client.get(self.url) + self.assertEqual(response.status_code, 200) + + def test_metadata_tables_definitions(self): + response = self.client.get(self.url) + output = response.json() + self.assertEqual( + output["tables"], + [ + { + "table_name": "fake_table", + "endpoint": "http://testserver/api/endpoints/fake-table/", + }, + ], + ) diff --git a/api/data_workspace/metadata/tests/urls.py b/api/data_workspace/metadata/tests/urls.py new file mode 100644 index 0000000000..15d987b719 --- /dev/null +++ b/api/data_workspace/metadata/tests/urls.py @@ -0,0 +1,22 @@ +from django.urls import ( + include, + path, +) + +from ..routers import TableMetadataRouter + +from . import views + + +test_router = TableMetadataRouter() + +test_router.register( + "fake-table", + views.FakeTableViewSet, + basename="dw-fake-table", +) + + +urlpatterns = [ + path("endpoints/", include(test_router.urls)), +] diff --git a/api/data_workspace/metadata/tests/views.py b/api/data_workspace/metadata/tests/views.py new file mode 100644 index 0000000000..2a06829bbd --- /dev/null +++ b/api/data_workspace/metadata/tests/views.py @@ -0,0 +1,13 @@ +from rest_framework import viewsets +from rest_framework.response import Response + + +class FakeTableViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "fake_table" + + def list(self, request): + return Response({}) + + def retrieve(self, request, pk): + return Response({}) From 0bf2a34c0b8634be709b13e02e71bf49e085e807 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 14:41:17 +0000 Subject: [PATCH 18/24] Switch to using metadata router on v2 endpoints --- api/data_workspace/v2/urls.py | 6 +++--- api/data_workspace/v2/views.py | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/api/data_workspace/v2/urls.py b/api/data_workspace/v2/urls.py index 50dd911495..97b074cee0 100644 --- a/api/data_workspace/v2/urls.py +++ b/api/data_workspace/v2/urls.py @@ -1,9 +1,9 @@ -from rest_framework.routers import DefaultRouter - from api.data_workspace.v2 import views +from api.data_workspace.metadata.routers import TableMetadataRouter + -router_v2 = DefaultRouter() +router_v2 = TableMetadataRouter() router_v2.register( "licence-decisions", diff --git a/api/data_workspace/v2/views.py b/api/data_workspace/v2/views.py index 553a254665..5947903af9 100644 --- a/api/data_workspace/v2/views.py +++ b/api/data_workspace/v2/views.py @@ -34,6 +34,9 @@ class BaseViewSet(viewsets.ReadOnlyModelViewSet): class LicenceDecisionViewSet(BaseViewSet): serializer_class = LicenceDecisionSerializer + class DataWorkspace: + table_name = "licence_decisions" + def get_queryset(self): queryset = ( ( @@ -65,3 +68,6 @@ def get_queryset(self): class CountryViewSet(BaseViewSet): serializer_class = CountrySerializer queryset = Country.objects.all().order_by("id", "name") + + class DataWorkspace: + table_name = "countries" From 54b563eaa0d06223d3bf4b87435dbcb428593e90 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 14:49:30 +0000 Subject: [PATCH 19/24] Add indexes to table metadata definition --- api/data_workspace/metadata/routers.py | 5 ++++- api/data_workspace/metadata/tests/test_metadata.py | 6 ++++++ api/data_workspace/metadata/tests/urls.py | 5 +++++ api/data_workspace/metadata/tests/views.py | 12 ++++++++++++ 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/api/data_workspace/metadata/routers.py b/api/data_workspace/metadata/routers.py index d1d9fc6042..e4b75f0d46 100644 --- a/api/data_workspace/metadata/routers.py +++ b/api/data_workspace/metadata/routers.py @@ -36,6 +36,7 @@ def get(self, request, *args, **kwargs): { "table_name": table_metadata["table_name"], "endpoint": url, + "indexes": table_metadata["indexes"], } ) return Response({"tables": tables}) @@ -46,10 +47,12 @@ def get_metadata_view(self, urls): metadata = [] list_name = self.routes[0].name for prefix, viewset, basename in self.registry: + data_workspace_metadata = viewset.DataWorkspace metadata.append( { - "table_name": viewset.DataWorkspace.table_name, + "table_name": data_workspace_metadata.table_name, "endpoint": list_name.format(basename=basename), + "indexes": getattr(data_workspace_metadata, "indexes", []), } ) diff --git a/api/data_workspace/metadata/tests/test_metadata.py b/api/data_workspace/metadata/tests/test_metadata.py index 312be274dd..f4da9eeb00 100644 --- a/api/data_workspace/metadata/tests/test_metadata.py +++ b/api/data_workspace/metadata/tests/test_metadata.py @@ -30,6 +30,12 @@ def test_metadata_tables_definitions(self): { "table_name": "fake_table", "endpoint": "http://testserver/api/endpoints/fake-table/", + "indexes": [], + }, + { + "table_name": "another_fake_table", + "endpoint": "http://testserver/api/endpoints/another-fake-table/", + "indexes": ["one", "two", "three"], }, ], ) diff --git a/api/data_workspace/metadata/tests/urls.py b/api/data_workspace/metadata/tests/urls.py index 15d987b719..9a8fdac714 100644 --- a/api/data_workspace/metadata/tests/urls.py +++ b/api/data_workspace/metadata/tests/urls.py @@ -16,6 +16,11 @@ basename="dw-fake-table", ) +test_router.register( + "another-fake-table", + views.AnotherFakeTableViewSet, + basename="dw-another-fake-table", +) urlpatterns = [ path("endpoints/", include(test_router.urls)), diff --git a/api/data_workspace/metadata/tests/views.py b/api/data_workspace/metadata/tests/views.py index 2a06829bbd..f1b3d9350d 100644 --- a/api/data_workspace/metadata/tests/views.py +++ b/api/data_workspace/metadata/tests/views.py @@ -11,3 +11,15 @@ def list(self, request): def retrieve(self, request, pk): return Response({}) + + +class AnotherFakeTableViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "another_fake_table" + indexes = ["one", "two", "three"] + + def list(self, request): + return Response({}) + + def retrieve(self, request, pk): + return Response({}) From 29cf53b7ddb7ced97a3cb248f21c02c7a49a8bde Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 15:10:30 +0000 Subject: [PATCH 20/24] Add fields metadata for data workspace endpoints --- api/data_workspace/metadata/routers.py | 2 ++ .../metadata/tests/test_metadata.py | 2 ++ api/data_workspace/metadata/tests/views.py | 1 + api/data_workspace/v2/views.py | 30 +++++++++++++++++++ 4 files changed, 35 insertions(+) diff --git a/api/data_workspace/metadata/routers.py b/api/data_workspace/metadata/routers.py index e4b75f0d46..73bebc9861 100644 --- a/api/data_workspace/metadata/routers.py +++ b/api/data_workspace/metadata/routers.py @@ -37,6 +37,7 @@ def get(self, request, *args, **kwargs): "table_name": table_metadata["table_name"], "endpoint": url, "indexes": table_metadata["indexes"], + "fields": table_metadata["fields"], } ) return Response({"tables": tables}) @@ -53,6 +54,7 @@ def get_metadata_view(self, urls): "table_name": data_workspace_metadata.table_name, "endpoint": list_name.format(basename=basename), "indexes": getattr(data_workspace_metadata, "indexes", []), + "fields": getattr(data_workspace_metadata, "fields", []), } ) diff --git a/api/data_workspace/metadata/tests/test_metadata.py b/api/data_workspace/metadata/tests/test_metadata.py index f4da9eeb00..595935d093 100644 --- a/api/data_workspace/metadata/tests/test_metadata.py +++ b/api/data_workspace/metadata/tests/test_metadata.py @@ -31,11 +31,13 @@ def test_metadata_tables_definitions(self): "table_name": "fake_table", "endpoint": "http://testserver/api/endpoints/fake-table/", "indexes": [], + "fields": [], }, { "table_name": "another_fake_table", "endpoint": "http://testserver/api/endpoints/another-fake-table/", "indexes": ["one", "two", "three"], + "fields": [{"name": "id", "primary_key": True, "type": "UUID"}], }, ], ) diff --git a/api/data_workspace/metadata/tests/views.py b/api/data_workspace/metadata/tests/views.py index f1b3d9350d..5d33fe9d53 100644 --- a/api/data_workspace/metadata/tests/views.py +++ b/api/data_workspace/metadata/tests/views.py @@ -17,6 +17,7 @@ class AnotherFakeTableViewSet(viewsets.ViewSet): class DataWorkspace: table_name = "another_fake_table" indexes = ["one", "two", "three"] + fields = [{"name": "id", "primary_key": True, "type": "UUID"}] def list(self, request): return Response({}) diff --git a/api/data_workspace/v2/views.py b/api/data_workspace/v2/views.py index 5947903af9..42cea1c901 100644 --- a/api/data_workspace/v2/views.py +++ b/api/data_workspace/v2/views.py @@ -35,6 +35,25 @@ class LicenceDecisionViewSet(BaseViewSet): serializer_class = LicenceDecisionSerializer class DataWorkspace: + fields = [ + { + "name": "id", + "type": "UUID", + "primary_key": True, + }, + { + "name": "reference_code", + "type": "String", + }, + { + "name": "decision", + "type": "String", + }, + { + "name": "decision_made_at", + "type": "DateTime", + }, + ] table_name = "licence_decisions" def get_queryset(self): @@ -70,4 +89,15 @@ class CountryViewSet(BaseViewSet): queryset = Country.objects.all().order_by("id", "name") class DataWorkspace: + fields = [ + { + "name": "code", + "type": "String", + "primary_key": True, + }, + { + "name": "name", + "type": "String", + }, + ] table_name = "countries" From fb9dcbc5b2ba9d729882850ffe475670e00db9a5 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 15:21:04 +0000 Subject: [PATCH 21/24] Ensure endpoints without a list view don't error --- api/data_workspace/metadata/tests/urls.py | 6 ++++++ api/data_workspace/metadata/tests/views.py | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/api/data_workspace/metadata/tests/urls.py b/api/data_workspace/metadata/tests/urls.py index 9a8fdac714..4657ff133b 100644 --- a/api/data_workspace/metadata/tests/urls.py +++ b/api/data_workspace/metadata/tests/urls.py @@ -22,6 +22,12 @@ basename="dw-another-fake-table", ) +test_router.register( + "detail-only", + views.DetailOnlyViewSet, + basename="dw-detail-only-table", +) + urlpatterns = [ path("endpoints/", include(test_router.urls)), ] diff --git a/api/data_workspace/metadata/tests/views.py b/api/data_workspace/metadata/tests/views.py index 5d33fe9d53..7315102896 100644 --- a/api/data_workspace/metadata/tests/views.py +++ b/api/data_workspace/metadata/tests/views.py @@ -24,3 +24,13 @@ def list(self, request): def retrieve(self, request, pk): return Response({}) + + +class DetailOnlyViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "another_fake_table" + indexes = ["one", "two", "three"] + fields = [{"name": "id", "primary_key": True, "type": "UUID"}] + + def retrieve(self, request, pk): + return Response({}) From b53c230836a346aa9508c52bff7b7792b50a437a Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 15:23:55 +0000 Subject: [PATCH 22/24] Add test to verify namespaced URLs work for metadata --- .../metadata/tests/test_metadata.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/api/data_workspace/metadata/tests/test_metadata.py b/api/data_workspace/metadata/tests/test_metadata.py index 595935d093..5bf7090aba 100644 --- a/api/data_workspace/metadata/tests/test_metadata.py +++ b/api/data_workspace/metadata/tests/test_metadata.py @@ -10,12 +10,14 @@ class MetadataTestCase(URLPatternsTestCase): urlpatterns = [ path("api/", include("api.data_workspace.metadata.tests.urls")), + path("namespaced/", include(("api.data_workspace.metadata.tests.urls", "namespaced"), namespace="namespaced")), ] def setUp(self): super().setUp() self.url = reverse("table-metadata") + self.namespaced_url = reverse("namespaced:table-metadata") def test_metadata_endpoint(self): response = self.client.get(self.url) @@ -41,3 +43,24 @@ def test_metadata_tables_definitions(self): }, ], ) + + def test_metadata_tables_definitions_with_namespace(self): + response = self.client.get(self.namespaced_url) + output = response.json() + self.assertEqual( + output["tables"], + [ + { + "table_name": "fake_table", + "endpoint": "http://testserver/namespaced/endpoints/fake-table/", + "indexes": [], + "fields": [], + }, + { + "table_name": "another_fake_table", + "endpoint": "http://testserver/namespaced/endpoints/another-fake-table/", + "indexes": ["one", "two", "three"], + "fields": [{"name": "id", "primary_key": True, "type": "UUID"}], + }, + ], + ) From 01bd99864276b77116054eb975d37a58d3c987db Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 15:33:33 +0000 Subject: [PATCH 23/24] Derive URLs from table names in DW endpoints --- api/data_workspace/metadata/routers.py | 6 ++++++ .../metadata/tests/test_metadata.py | 19 ++++++++++++++++++ api/data_workspace/metadata/tests/urls.py | 20 +++---------------- api/data_workspace/metadata/tests/views.py | 2 +- api/data_workspace/v2/urls.py | 10 ++-------- 5 files changed, 31 insertions(+), 26 deletions(-) diff --git a/api/data_workspace/metadata/routers.py b/api/data_workspace/metadata/routers.py index 73bebc9861..83d158d36c 100644 --- a/api/data_workspace/metadata/routers.py +++ b/api/data_workspace/metadata/routers.py @@ -44,6 +44,12 @@ def get(self, request, *args, **kwargs): class TableMetadataRouter(DefaultRouter): + def register(self, viewset): + prefix = viewset.DataWorkspace.table_name.replace("_", "-") + basename = f"dw-{prefix}" + + super().register(prefix, viewset, basename) + def get_metadata_view(self, urls): metadata = [] list_name = self.routes[0].name diff --git a/api/data_workspace/metadata/tests/test_metadata.py b/api/data_workspace/metadata/tests/test_metadata.py index 5bf7090aba..3013ab46fb 100644 --- a/api/data_workspace/metadata/tests/test_metadata.py +++ b/api/data_workspace/metadata/tests/test_metadata.py @@ -23,6 +23,25 @@ def test_metadata_endpoint(self): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) + def test_urls(self): + self.assertEqual( + reverse("dw-fake-table-list"), + "/api/endpoints/fake-table/", + ) + self.assertEqual( + reverse("dw-fake-table-detail", kwargs={"pk": "test"}), + "/api/endpoints/fake-table/test/", + ) + + self.assertEqual( + reverse("namespaced:dw-fake-table-list"), + "/namespaced/endpoints/fake-table/", + ) + self.assertEqual( + reverse("namespaced:dw-fake-table-detail", kwargs={"pk": "test"}), + "/namespaced/endpoints/fake-table/test/", + ) + def test_metadata_tables_definitions(self): response = self.client.get(self.url) output = response.json() diff --git a/api/data_workspace/metadata/tests/urls.py b/api/data_workspace/metadata/tests/urls.py index 4657ff133b..e48b46e6bf 100644 --- a/api/data_workspace/metadata/tests/urls.py +++ b/api/data_workspace/metadata/tests/urls.py @@ -10,23 +10,9 @@ test_router = TableMetadataRouter() -test_router.register( - "fake-table", - views.FakeTableViewSet, - basename="dw-fake-table", -) - -test_router.register( - "another-fake-table", - views.AnotherFakeTableViewSet, - basename="dw-another-fake-table", -) - -test_router.register( - "detail-only", - views.DetailOnlyViewSet, - basename="dw-detail-only-table", -) +test_router.register(views.FakeTableViewSet) +test_router.register(views.AnotherFakeTableViewSet) +test_router.register(views.DetailOnlyViewSet) urlpatterns = [ path("endpoints/", include(test_router.urls)), diff --git a/api/data_workspace/metadata/tests/views.py b/api/data_workspace/metadata/tests/views.py index 7315102896..52ab583c94 100644 --- a/api/data_workspace/metadata/tests/views.py +++ b/api/data_workspace/metadata/tests/views.py @@ -28,7 +28,7 @@ def retrieve(self, request, pk): class DetailOnlyViewSet(viewsets.ViewSet): class DataWorkspace: - table_name = "another_fake_table" + table_name = "detail_only_table" indexes = ["one", "two", "three"] fields = [{"name": "id", "primary_key": True, "type": "UUID"}] diff --git a/api/data_workspace/v2/urls.py b/api/data_workspace/v2/urls.py index 97b074cee0..8b63d581a3 100644 --- a/api/data_workspace/v2/urls.py +++ b/api/data_workspace/v2/urls.py @@ -4,11 +4,5 @@ router_v2 = TableMetadataRouter() - -router_v2.register( - "licence-decisions", - views.LicenceDecisionViewSet, - basename="dw-licence-decisions", -) - -router_v2.register("countries", views.CountryViewSet, basename="dw-countries") +router_v2.register(views.LicenceDecisionViewSet) +router_v2.register(views.CountryViewSet) From 08a44903d5907aed619167e985c0ed76bd37d080 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 20 Nov 2024 19:42:24 +0000 Subject: [PATCH 24/24] Add automatic type annotations for table metadata --- api/data_workspace/metadata/routers.py | 73 +++++++++++++++- .../metadata/tests/auto_field_urls.py | 22 +++++ .../metadata/tests/serializers.py | 48 ++++++++++ .../metadata/tests/test_metadata.py | 87 +++++++++++++++++++ api/data_workspace/metadata/tests/views.py | 69 +++++++++++++++ api/data_workspace/v2/serializers.py | 6 +- api/data_workspace/v2/views.py | 30 ------- 7 files changed, 301 insertions(+), 34 deletions(-) create mode 100644 api/data_workspace/metadata/tests/auto_field_urls.py diff --git a/api/data_workspace/metadata/routers.py b/api/data_workspace/metadata/routers.py index 83d158d36c..669844cce2 100644 --- a/api/data_workspace/metadata/routers.py +++ b/api/data_workspace/metadata/routers.py @@ -1,3 +1,7 @@ +import datetime +import typing + +from rest_framework import serializers from rest_framework.response import Response from rest_framework.reverse import reverse from rest_framework.routers import DefaultRouter @@ -43,8 +47,66 @@ def get(self, request, *args, **kwargs): return Response({"tables": tables}) +def is_optional(field): + return typing.get_origin(field) is typing.Union and type(None) in typing.get_args(field) + + +def get_fields(view): + try: + serializer = view.get_serializer() + except AttributeError: + return [] + + primary_key_field = getattr(view.DataWorkspace, "primary_key", "id") + + fields = [] + for field in serializer.fields.values(): + if isinstance(field, serializers.HiddenField): + continue + + field_metadata = {"name": field.field_name} + if field.field_name == primary_key_field: + field_metadata["primary_key"] = True + + if isinstance(field, serializers.UUIDField): + field_metadata["type"] = "UUID" + if field.allow_null: + field_metadata["nullable"] = True + + elif isinstance(field, serializers.CharField): + field_metadata["type"] = "String" + if field.allow_null: + field_metadata["nullable"] = True + + elif isinstance(field, serializers.SerializerMethodField): + method = getattr(field.parent, field.method_name) + return_type = method.__annotations__["return"] + + if is_optional(return_type): + field_metadata["nullable"] = True + return_type, _ = typing.get_args(return_type) + + if return_type is str: + field_metadata["type"] = "String" + elif return_type is datetime.datetime: + field_metadata["type"] = "DateTime" + else: # pragma: no cover + raise NotImplementedError( + f"Return type of {return_type} for {serializer.__class__.__name__}.{field.method_name} not handled" + ) + + else: # pragma: no cover + raise NotImplementedError(f"Annotation not found for {field}") + + fields.append(field_metadata) + return fields + + class TableMetadataRouter(DefaultRouter): def register(self, viewset): + if not hasattr(viewset, "DataWorkspace"): # pragma: no cover + raise NotImplementedError(f"No DataWorkspace configuration found for {viewset}") + prefix = viewset.DataWorkspace.table_name.replace("_", "-") basename = f"dw-{prefix}" @@ -53,14 +115,21 @@ def register(self, viewset): def get_metadata_view(self, urls): metadata = [] list_name = self.routes[0].name - for prefix, viewset, basename in self.registry: + for _, viewset, basename in self.registry: data_workspace_metadata = viewset.DataWorkspace + + view = viewset() + view.args = () + view.kwargs = {} + view.format_kwarg = {} + view.request = None + metadata.append( { "table_name": data_workspace_metadata.table_name, "endpoint": list_name.format(basename=basename), "indexes": getattr(data_workspace_metadata, "indexes", []), - "fields": getattr(data_workspace_metadata, "fields", []), + "fields": getattr(data_workspace_metadata, "fields", get_fields(view)), } ) diff --git a/api/data_workspace/metadata/tests/auto_field_urls.py b/api/data_workspace/metadata/tests/auto_field_urls.py new file mode 100644 index 0000000000..205b80b1b9 --- /dev/null +++ b/api/data_workspace/metadata/tests/auto_field_urls.py @@ -0,0 +1,22 @@ +from django.urls import ( + include, + path, +) + +from ..routers import TableMetadataRouter + +from . import views + + +test_router = TableMetadataRouter() + +test_router.register(views.HiddenFieldViewSet) +test_router.register(views.UUIDFieldViewSet) +test_router.register(views.CharFieldViewSet) +test_router.register(views.SerializerMethodFieldViewSet) +test_router.register(views.AutoPrimaryKeyViewSet) +test_router.register(views.ExplicitPrimaryKeyViewSet) + +urlpatterns = [ + path("endpoints/", include(test_router.urls)), +] diff --git a/api/data_workspace/metadata/tests/serializers.py b/api/data_workspace/metadata/tests/serializers.py index e69de29bb2..f3f3498e25 100644 --- a/api/data_workspace/metadata/tests/serializers.py +++ b/api/data_workspace/metadata/tests/serializers.py @@ -0,0 +1,48 @@ +import datetime + +from typing import Optional + +from rest_framework import serializers + + +class HiddenFieldSerializer(serializers.Serializer): + hidden_field = serializers.HiddenField(default="") + + +class UUIDFieldSerializer(serializers.Serializer): + uuid_field = serializers.UUIDField() + nullable_uuid_field = serializers.UUIDField(allow_null=True) + + +class CharFieldSerializer(serializers.Serializer): + char_field = serializers.CharField() + nullable_char_field = serializers.CharField(allow_null=True) + + +class SerializerMethodFieldSerializer(serializers.Serializer): + returns_string = serializers.SerializerMethodField() + returns_optional_string = serializers.SerializerMethodField() + returns_datetime = serializers.SerializerMethodField() + returns_optional_datetime = serializers.SerializerMethodField() + + def get_returns_string(self, instance) -> str: + return "string" + + def get_returns_optional_string(self, instance) -> Optional[str]: + return None + + def get_returns_datetime(self, instance) -> datetime.datetime: + return datetime.datetime.now() + + def get_returns_optional_datetime(self, instance) -> Optional[datetime.datetime]: + return None + + +class AutoPrimaryKeySerializer(serializers.Serializer): + id = serializers.UUIDField() + not_a_primary_key = serializers.UUIDField() + + +class ExplicitPrimaryKeySerializer(serializers.Serializer): + a_different_id = serializers.UUIDField() + not_a_primary_key = serializers.UUIDField() diff --git a/api/data_workspace/metadata/tests/test_metadata.py b/api/data_workspace/metadata/tests/test_metadata.py index 3013ab46fb..1968c4308b 100644 --- a/api/data_workspace/metadata/tests/test_metadata.py +++ b/api/data_workspace/metadata/tests/test_metadata.py @@ -11,6 +11,10 @@ class MetadataTestCase(URLPatternsTestCase): urlpatterns = [ path("api/", include("api.data_workspace.metadata.tests.urls")), path("namespaced/", include(("api.data_workspace.metadata.tests.urls", "namespaced"), namespace="namespaced")), + path( + "auto-fields/", + include(("api.data_workspace.metadata.tests.auto_field_urls", "auto-fields"), namespace="auto-fields"), + ), ] def setUp(self): @@ -18,6 +22,7 @@ def setUp(self): self.url = reverse("table-metadata") self.namespaced_url = reverse("namespaced:table-metadata") + self.auto_fields_url = reverse("auto-fields:table-metadata") def test_metadata_endpoint(self): response = self.client.get(self.url) @@ -83,3 +88,85 @@ def test_metadata_tables_definitions_with_namespace(self): }, ], ) + + def assertFieldsEqual(self, output, table_name, fields): + for table in output["tables"]: + if table["table_name"] == table_name: + self.assertEqual( + table["fields"], + fields, + ) + break + else: + self.fail(f"No table found with name {table_name}") + + def test_hidden_field_definition(self): + response = self.client.get(self.auto_fields_url) + output = response.json() + self.assertFieldsEqual( + output, + "hidden_field", + [], + ) + + def test_uuid_field_definition(self): + response = self.client.get(self.auto_fields_url) + output = response.json() + self.assertFieldsEqual( + output, + "uuid_field", + [ + {"name": "uuid_field", "type": "UUID"}, + {"name": "nullable_uuid_field", "type": "UUID", "nullable": True}, + ], + ) + + def test_char_field_definition(self): + response = self.client.get(self.auto_fields_url) + output = response.json() + self.assertFieldsEqual( + output, + "char_field", + [ + {"name": "char_field", "type": "String"}, + {"name": "nullable_char_field", "type": "String", "nullable": True}, + ], + ) + + def test_serializer_method_field(self): + response = self.client.get(self.auto_fields_url) + output = response.json() + self.assertFieldsEqual( + output, + "serializer_method_field", + [ + {"name": "returns_string", "type": "String"}, + {"name": "returns_optional_string", "type": "String", "nullable": True}, + {"name": "returns_datetime", "type": "DateTime"}, + {"name": "returns_optional_datetime", "type": "DateTime", "nullable": True}, + ], + ) + + def test_auto_primary_key(self): + response = self.client.get(self.auto_fields_url) + output = response.json() + self.assertFieldsEqual( + output, + "auto_primary_key", + [ + {"name": "id", "type": "UUID", "primary_key": True}, + {"name": "not_a_primary_key", "type": "UUID"}, + ], + ) + + def test_explicit_primary_key(self): + response = self.client.get(self.auto_fields_url) + output = response.json() + self.assertFieldsEqual( + output, + "explicit_primary_key", + [ + {"name": "a_different_id", "type": "UUID", "primary_key": True}, + {"name": "not_a_primary_key", "type": "UUID"}, + ], + ) diff --git a/api/data_workspace/metadata/tests/views.py b/api/data_workspace/metadata/tests/views.py index 52ab583c94..80e823ec3f 100644 --- a/api/data_workspace/metadata/tests/views.py +++ b/api/data_workspace/metadata/tests/views.py @@ -1,6 +1,8 @@ from rest_framework import viewsets from rest_framework.response import Response +from . import serializers + class FakeTableViewSet(viewsets.ViewSet): class DataWorkspace: @@ -34,3 +36,70 @@ class DataWorkspace: def retrieve(self, request, pk): return Response({}) + + +class HiddenFieldViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "hidden_field" + + def get_serializer(self): + return serializers.HiddenFieldSerializer() + + def list(self, request): + return Response({}) + + +class UUIDFieldViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "uuid_field" + + def get_serializer(self): + return serializers.UUIDFieldSerializer() + + def list(self, request): + return Response({}) + + +class CharFieldViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "char_field" + + def get_serializer(self): + return serializers.CharFieldSerializer() + + def list(self, request): + return Response({}) + + +class SerializerMethodFieldViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "serializer_method_field" + + def get_serializer(self): + return serializers.SerializerMethodFieldSerializer() + + def list(self, request): + return Response({}) + + +class AutoPrimaryKeyViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "auto_primary_key" + + def get_serializer(self): + return serializers.AutoPrimaryKeySerializer() + + def list(self, request): + return Response({}) + + +class ExplicitPrimaryKeyViewSet(viewsets.ViewSet): + class DataWorkspace: + table_name = "explicit_primary_key" + primary_key = "a_different_id" + + def get_serializer(self): + return serializers.ExplicitPrimaryKeySerializer() + + def list(self, request): + return Response({}) diff --git a/api/data_workspace/v2/serializers.py b/api/data_workspace/v2/serializers.py index 1b3da1cb42..ff78b36346 100644 --- a/api/data_workspace/v2/serializers.py +++ b/api/data_workspace/v2/serializers.py @@ -1,3 +1,5 @@ +import datetime + from rest_framework import serializers from api.cases.enums import LicenceDecisionType @@ -18,10 +20,10 @@ class Meta: "decision_made_at", ) - def get_decision(self, case): + def get_decision(self, case) -> str: return case.decision - def get_decision_made_at(self, case): + def get_decision_made_at(self, case) -> datetime.datetime: if case.decision not in LicenceDecisionType.decisions(): raise ValueError(f"Unknown decision type `{case.decision}`") # pragma: no cover diff --git a/api/data_workspace/v2/views.py b/api/data_workspace/v2/views.py index 42cea1c901..5947903af9 100644 --- a/api/data_workspace/v2/views.py +++ b/api/data_workspace/v2/views.py @@ -35,25 +35,6 @@ class LicenceDecisionViewSet(BaseViewSet): serializer_class = LicenceDecisionSerializer class DataWorkspace: - fields = [ - { - "name": "id", - "type": "UUID", - "primary_key": True, - }, - { - "name": "reference_code", - "type": "String", - }, - { - "name": "decision", - "type": "String", - }, - { - "name": "decision_made_at", - "type": "DateTime", - }, - ] table_name = "licence_decisions" def get_queryset(self): @@ -89,15 +70,4 @@ class CountryViewSet(BaseViewSet): queryset = Country.objects.all().order_by("id", "name") class DataWorkspace: - fields = [ - { - "name": "code", - "type": "String", - "primary_key": True, - }, - { - "name": "name", - "type": "String", - }, - ] table_name = "countries"