From 49f39acef03e8e3a41df9c3a07256fd031a0de31 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Tue, 9 Nov 2021 23:16:23 -0500 Subject: [PATCH] fix: Return {} if no dataset is found by name, instead of HTTP 404 (was ObjectDoesNotExist) - Return HTTP 400 Bad Request for missing request parameters when creating datasets - Return HTTP 404 Not Found on status action for missing dataset, instead of returning {} - Return HTTP 404 Not Found on metadata action, instead of raising ObjectDoesNotExist - Return HTTP 404 Not Found on coverage action, instead of returning {} --- backend/processor/views.py | 75 +++++++++++++++---- backend/tests/processor/test_views.py | 47 ++++++++---- docs/backend/reference/index.rst | 9 +++ docs/backend/reference/integration.rst | 18 +++++ docs/backend/reference/overview.rst | 8 ++ .../{reference.rst => reference/settings.rst} | 23 +----- docs/changelog.rst | 20 ++++- 7 files changed, 149 insertions(+), 51 deletions(-) create mode 100644 docs/backend/reference/index.rst create mode 100644 docs/backend/reference/integration.rst create mode 100644 docs/backend/reference/overview.rst rename docs/backend/{reference.rst => reference/settings.rst} (52%) diff --git a/backend/processor/views.py b/backend/processor/views.py index e14d458c..a420cf9b 100644 --- a/backend/processor/views.py +++ b/backend/processor/views.py @@ -6,7 +6,7 @@ from django.views.decorators.http import require_POST from dqt.models import Dataset, FieldLevelCheck, ProgressMonitorDataset from psycopg2.sql import SQL, Identifier -from rest_framework import status, viewsets +from rest_framework import serializers, status, viewsets from rest_framework.decorators import action from rest_framework.response import Response @@ -21,6 +21,15 @@ def create_dataset_filter(request): return JsonResponse({"status": "ok"}) +class CreateDatasetSerializer(serializers.Serializer): + name = serializers.CharField() + collection_id = serializers.IntegerField() + + +class DeleteDatasetSerializer(serializers.Serializer): + dataset_id = serializers.IntegerField() + + class DatasetViewSet(viewsets.GenericViewSet): queryset = Dataset.objects.all() # ViewSet's don't allow typed paths like . @@ -28,37 +37,71 @@ class DatasetViewSet(viewsets.GenericViewSet): # https://github.com/encode/django-rest-framework/issues/6148#issuecomment-725297421 lookup_value_regex = "[0-9]+" + def get_serializer_class(self): + if self.request.method == "DELETE": + return DeleteDatasetSerializer + return CreateDatasetSerializer + def create(self, request): - message = {"name": request.data.get("name"), "collection_id": request.data.get("collection_id")} - publish(json.dumps(message), "ocds_kingfisher_extractor_init") + """ + Publishes a message to RabbitMQ to create the dataset with the given ``name`` and ``collection_id``. + """ + serializer = self.get_serializer( + data={"name": request.data.get("name"), "collection_id": request.data.get("collection_id")} + ) + serializer.is_valid(raise_exception=True) + publish(json.dumps(serializer.data), "ocds_kingfisher_extractor_init") return Response(status=status.HTTP_202_ACCEPTED) def destroy(self, request, pk=None): - message = {"dataset_id": int(pk)} - publish(json.dumps(message), "wiper_init") + """ + Publishes a message to RabbitMQ to wipe the dataset. + """ + serializer = self.get_serializer(data={"dataset_id": pk}) + serializer.is_valid(raise_exception=True) + publish(json.dumps(serializer.data), "wiper_init") return Response(status=status.HTTP_202_ACCEPTED) @action(detail=False) def find_by_name(self, request): - dataset = get_object_or_404(self.get_queryset(), name=request.GET.get("name")) - return Response({"id": dataset.id}) + """ + Returns the ID of the dataset with the ``name`` given in the query string, as an object like ``{"id": 123}``, + or ``{}`` if no name matches. + """ + try: + dataset = self.get_queryset().get(name=request.GET.get("name")) + return Response({"id": dataset.pk}) + except Dataset.DoesNotExist: + return Response({}) @action(detail=True) def status(self, request, pk=None): + """ + Returns the dataset's status, or ``{}`` if no status is set. + """ + self.get_object() # trigger 404 if no dataset try: monitor = ProgressMonitorDataset.objects.values("state", "phase").get(dataset__pk=pk) + return Response(monitor) except ProgressMonitorDataset.DoesNotExist: - monitor = {} - return Response(monitor) + return Response({}) @action(detail=True) def metadata(self, request, pk=None): - meta = self.get_queryset().values_list("meta__collection_metadata", flat=True).get(pk=pk) + """ + Returns the dataset's collection metadata. + """ + meta = get_object_or_404(self.get_queryset().values_list("meta__collection_metadata", flat=True), pk=pk) return Response(meta or {}) @action(detail=True) def coverage(self, request, pk=None): - map = { + """ + Returns the dataset's coverage statistics. + """ + self.get_object() # trigger 404 if no dataset + + mapping = { "parties": ["parties.id"], "plannings": ["planning.budget"], "tenders": ["tender.id"], @@ -83,7 +126,11 @@ def coverage(self, request, pk=None): "contracts.milestones.id", "contracts.implementation.milestones.id", ], - "amendments": ["tender.amendments.id", "awards.amendments.id", "contract.amendments.id"], + "amendments": [ + "tender.amendments.id", + "awards.amendments.id", + "contract.amendments.id", + ], } with connections["data"].cursor() as cursor: @@ -98,13 +145,13 @@ def coverage(self, request, pk=None): cursor.execute( SQL(statement).format(table=Identifier(FieldLevelCheck._meta.db_table)), - {"checks": tuple(j for i in map.values() for j in i), "dataset_id": pk}, + {"checks": tuple(j for i in mapping.values() for j in i), "dataset_id": pk}, ) results = cursor.fetchall() counts = {} - for key, items in map.items(): + for key, items in mapping.items(): counts[key] = 0 for i in items: for r in results: diff --git a/backend/tests/processor/test_views.py b/backend/tests/processor/test_views.py index 1971224a..cc118e71 100644 --- a/backend/tests/processor/test_views.py +++ b/backend/tests/processor/test_views.py @@ -32,6 +32,13 @@ def test_require_POST(self): self.assertEqual(response.status_code, 405) + @patch("processor.views.publish") + def test_create_no_values(self, publish): + response = self.client.post("/datasets/", {}, "application/json") + + self.assertEqual(response.status_code, 400) + publish.assert_not_called() + @patch("processor.views.publish") def test_create(self, publish): response = self.client.post( @@ -60,17 +67,29 @@ def test_destroy(self, publish): self.assertEqual(response.status_code, 202) publish.assert_called_once_with('{"dataset_id": 123}', "wiper_init") + def test_find_by_name_no_name(self): + self.create(Dataset, name="anything") + response = self.client.get("/datasets/find_by_name/") + + self.assertEqual(response.status_code, 200) + self.assertJSONEqual(response.content, {}) + + def test_find_by_name_no_match(self): + self.create(Dataset, name="anything") + response = self.client.get("/datasets/find_by_name/?name=other") + + self.assertEqual(response.status_code, 200) + self.assertJSONEqual(response.content, {}) + def test_find_by_name(self): dataset = self.create(Dataset, name="anything") - response = self.client.get("/datasets/find_by_name/?name=anything") self.assertEqual(response.status_code, 200) self.assertJSONEqual(response.content, {"id": dataset.pk}) - def test_metadata_no_meta(self): + def test_metadata_no_values(self): dataset = self.create(Dataset, name="anything") - response = self.client.get(f"/datasets/{dataset.pk}/metadata/") self.assertEqual(response.status_code, 200) @@ -78,7 +97,6 @@ def test_metadata_no_meta(self): def test_metadata(self): dataset = self.create(Dataset, name="anything", meta={"collection_metadata": {"ocid_prefix": "ocds-213czf"}}) - response = self.client.get(f"/datasets/{dataset.pk}/metadata/") self.assertEqual(response.status_code, 200) @@ -112,15 +130,8 @@ def test_coverage(self): }, ) - def test_status_no_dataset(self): - response = self.client.get("/datasets/123/status/") - - self.assertEqual(response.status_code, 200) - self.assertJSONEqual(response.content, {}) - def test_status_no_progress(self): dataset = self.create(Dataset, name="anything") - response = self.client.get(f"/datasets/{dataset.pk}/status/") self.assertEqual(response.status_code, 200) @@ -129,7 +140,6 @@ def test_status_no_progress(self): def test_status_no_values(self): dataset = self.create(Dataset, name="anything") self.create(ProgressMonitorDataset, dataset=dataset) - response = self.client.get(f"/datasets/{dataset.pk}/status/") self.assertEqual(response.status_code, 200) @@ -138,8 +148,19 @@ def test_status_no_values(self): def test_status(self): dataset = self.create(Dataset, name="anything") self.create(ProgressMonitorDataset, dataset=dataset, phase="CHECKED", state="OK") - response = self.client.get(f"/datasets/{dataset.pk}/status/") self.assertEqual(response.status_code, 200) self.assertJSONEqual(response.content, {"phase": "CHECKED", "state": "OK"}) + + def test_status_no_dataset(self): + response = self.client.get("/datasets/123/status/") + self.assertEqual(response.status_code, 404) + + def test_metadata_no_dataset(self): + response = self.client.get("/datasets/123/metadata/") + self.assertEqual(response.status_code, 404) + + def test_coverage_no_dataset(self): + response = self.client.get("/datasets/123/coverage/") + self.assertEqual(response.status_code, 404) diff --git a/docs/backend/reference/index.rst b/docs/backend/reference/index.rst new file mode 100644 index 00000000..feb45000 --- /dev/null +++ b/docs/backend/reference/index.rst @@ -0,0 +1,9 @@ +Reference +========= + +.. toctree:: + :maxdepth: 2 + + overview + settings + integration diff --git a/docs/backend/reference/integration.rst b/docs/backend/reference/integration.rst new file mode 100644 index 00000000..70c4657b --- /dev/null +++ b/docs/backend/reference/integration.rst @@ -0,0 +1,18 @@ +Pelican backend integration +=========================== + +Pelican backend's database is treated as a read-only `legacy database `__, with ``managed = False`` in all model's ``Meta`` class, and with a ``DATABASE_ROUTERS`` setting that routes queries to its database. + +To update ``backend/dqt/models.py`` following changes to Pelican backend's database schema: + +- Run ``python backend/manage.py inspectdb > backend/dqt/models.py`` +- Replace comments at top of file +- Replace ``models.DO_NOTHING`` with ``on_delete=models.CASCADE`` +- ``Dataset``: Add methods +- ``Dataset.meta``: Add ``blank=True, default=dict`` +- ``DatasetFilter.dataset_id_original``: Rename to ``dataset_original``, add ``related_name="dataset_filter_parent"`` +- ``DatasetFilter.dataset_id_filtered``: Rename to ``dataset_filtered``, add ``related_name="dataset_filter_child"`` +- ``ProgressMonitorDataset.dataset``: Add ``related_name="progress"`` +- ``ProgressMonitorItem.item``: Rename to ``data_item`` +- ``Report.type``: Change ``TextField`` to ``CharField``, add ``max_length=255``, and remove ``# This field type is a guess.`` + diff --git a/docs/backend/reference/overview.rst b/docs/backend/reference/overview.rst new file mode 100644 index 00000000..cab2090a --- /dev/null +++ b/docs/backend/reference/overview.rst @@ -0,0 +1,8 @@ +Technical overview +================== + +The Django project is made up of three apps: + +- ``dqt``: Serves API requests for the :doc:`../../frontend/index` +- ``exporter``: Generates the exports to Google Docs +- ``processor``: Serves API requests for managing Pelican backend diff --git a/docs/backend/reference.rst b/docs/backend/reference/settings.rst similarity index 52% rename from docs/backend/reference.rst rename to docs/backend/reference/settings.rst index f5017b5d..6c92bc3f 100644 --- a/docs/backend/reference.rst +++ b/docs/backend/reference/settings.rst @@ -1,26 +1,5 @@ -Reference -========= - -Pelican backend integration ---------------------------- - -Pelican backend's database is treated as a read-only `legacy database `__, with ``managed = False`` in all model's ``Meta`` class, and with a ``DATABASE_ROUTERS`` setting that routes queries to its database. - -To update ``backend/dqt/models.py`` following changes to Pelican backend's database schema: - -- Run ``python backend/manage.py inspectdb > backend/dqt/models.py`` -- Replace comments at top of file -- Replace ``models.DO_NOTHING`` with ``on_delete=models.CASCADE`` -- ``Dataset``: Add methods -- ``Dataset.meta``: Add ``blank=True, default=dict`` -- ``DatasetFilter.dataset_id_original``: Rename to ``dataset_original``, add ``related_name="dataset_filter_parent"`` -- ``DatasetFilter.dataset_id_filtered``: Rename to ``dataset_filtered``, add ``related_name="dataset_filter_child"`` -- ``ProgressMonitorDataset.dataset``: Add ``related_name="progress"`` -- ``ProgressMonitorItem.item``: Rename to ``data_item`` -- ``Report.type``: Change ``TextField`` to ``CharField``, add ``max_length=255``, and remove ``# This field type is a guess.`` - Environment variables ---------------------- +===================== See `OCP's approach to Django settings `__. New variables are: diff --git a/docs/changelog.rst b/docs/changelog.rst index b54bc345..58f1905a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,10 +6,26 @@ This changelog only notes major changes, to notify other developers. 2021-11-09 ---------- -- refactor: Rewrite processor API. +- fix: Return ``{}`` if no dataset is found by name, instead of raising ``ObjectDoesNotExist``. +- refactor: Rewrite the API endpoints for managing Pelican backend. + + - Requests: + + - Use the DELETE method to delete datasets. + - Use the GET method for the safe operation of finding a dataset by name, instead of the POST method. + - Accept the primary key in the URL path, instead of in the request body. + + - Responses: + + - Return a HTTP 2xx code, instead of ``"status": "ok"`` in the JSON response. + - Return HTTP 202 Accepted for creating and deleting datasets asynchronously. + - Return HTTP 400 Bad Request for missing request parameters when creating datasets. + - Return HTTP 404 Not Found, instead of raising ``ObjectDoesNotExist``. + - Return HTTP 405 Method Not Allowed for incorrect HTTP methods, instead of HTTP 200 with a JSON error message. + - Return a JSON object from all endpoints, instead of sometimes null, a number or a string. 2021-11-08 ---------- -- refactor: Split Django applications from Django project. :commit:`df4b678` :commit:`fe94f41` +- refactor: Split Django applications from Django project. :commit:`df4b678` :commit:`fe94f41` :commit:`f01bcaf` - refactor: Move static assets out of code directory. :commit:`80bbd09`