From 048151378aa4a44c3a40d5a76f228157f43fe669 Mon Sep 17 00:00:00 2001 From: James McKinney <26463+jpmckinney@users.noreply.github.com> Date: Tue, 9 Nov 2021 15:16:20 -0500 Subject: [PATCH] refactor: Start rewrite of processor views - fix: Return HTTP error code instead of HTTP 200 with a JSON error message - fix: Use GET method for idempotent dataset_id endpoint, instead of POST method - fix: Return JSON response from all app endpoints, instead of plain-text response - chore: Use Django's require_POST decorator instead of custom code (HTTP 405 instead of HTTP 400) - chore: Remove unnecessary safe=False argument to JsonResponse --- backend/dqt/views.py | 2 +- backend/processor/views.py | 57 ++++++++------------------- backend/tests/processor/test_views.py | 14 +++---- docs/changelog.rst | 5 +++ 4 files changed, 29 insertions(+), 49 deletions(-) diff --git a/backend/dqt/views.py b/backend/dqt/views.py index 3f17dc51..34d9d90a 100644 --- a/backend/dqt/views.py +++ b/backend/dqt/views.py @@ -30,7 +30,7 @@ def dataset_filter_items(request): dataset_id_original = input_message["dataset_id_original"] filter_message = input_message["filter_message"] - # building query in a safely manner + # See similar code in dataset_filter.py in pelican-backend. try: variables = {"dataset_id_original": dataset_id_original} parts = ["SELECT count(*) FROM data_item WHERE dataset_id = %(dataset_id_original)s"] diff --git a/backend/processor/views.py b/backend/processor/views.py index c35ebab9..34c230d5 100644 --- a/backend/processor/views.py +++ b/backend/processor/views.py @@ -1,7 +1,8 @@ import simplejson as json from django.db import connections -from django.http import HttpResponse, HttpResponseBadRequest, JsonResponse +from django.http import JsonResponse from django.views.decorators.csrf import csrf_exempt +from django.views.decorators.http import require_POST from dqt.models import Dataset, FieldLevelCheck, ProgressMonitorDataset from psycopg2.sql import SQL, Identifier @@ -9,80 +10,56 @@ @csrf_exempt +@require_POST def create_dataset_filter(request): - if request.method == "GET": - return HttpResponseBadRequest(reason="Only post method is accepted.") - publish(request.body, "dataset_filter_extractor_init") - return HttpResponse("done") + return JsonResponse({"status": "ok"}) @csrf_exempt +@require_POST def dataset_start(request): - if request.method == "GET": - return JsonResponse({"status": "error", "data": {"reason": "Only post method is accepted."}}) - - routing_key = "ocds_kingfisher_extractor_init" - body = json.loads(request.body.decode("utf-8")) - dataset_name = body.get("name") - message = { - "name": dataset_name, + "name": body.get("name"), "collection_id": body.get("collection_id"), - # "ancestor_id": ancestor_id, - # "max_items": max_items, } - - publish(json.dumps(message), routing_key) + publish(json.dumps(message), "ocds_kingfisher_extractor_init") return JsonResponse( - {"status": "ok", "data": {"message": f"Dataset {dataset_name} on Pelican started"}}, safe=False + {"status": "ok", "data": {"message": "Started dataset '%(name)s' for collection %(collection_id)s" % message}} ) @csrf_exempt +@require_POST def dataset_wipe(request): - if request.method == "GET": - return JsonResponse({"status": "error", "data": {"reason": "Only post method is accepted."}}) - - routing_key = "wiper_init" - body = json.loads(request.body.decode("utf-8")) message = { "dataset_id": body.get("dataset_id"), } + publish(json.dumps(message), "wiper_init") - publish(json.dumps(message), routing_key) - - return JsonResponse( - {"status": "ok", "data": {"message": f"Dataset id {body.get('dataset_id')} on Pelican will be wiped"}}, - safe=False, - ) + return JsonResponse({"status": "ok", "data": {"message": "Wiping dataset %(dataset_id)s" % message}}) @csrf_exempt def dataset_progress(request, dataset_id): try: monitor = ProgressMonitorDataset.objects.values("state", "phase").get(dataset__id=dataset_id) - return JsonResponse({"status": "ok", "data": monitor}, safe=False) + return JsonResponse({"status": "ok", "data": monitor}) except ProgressMonitorDataset.DoesNotExist: - return JsonResponse({"status": "ok", "data": None}, safe=False) + return JsonResponse({"status": "ok", "data": None}) @csrf_exempt def dataset_id(request): - if request.method == "GET": - return JsonResponse({"status": "error", "data": {"reason": "Only post method is accepted."}}) - - body = json.loads(request.body.decode("utf-8")) - dataset_name = body.get("name") + dataset = Dataset.objects.get(name=request.GET.get("name")) - dataset = Dataset.objects.get(name=dataset_name) - return JsonResponse({"status": "ok", "data": dataset.id if dataset else None}, safe=False) + return JsonResponse({"status": "ok", "data": dataset.id if dataset else None}) @csrf_exempt @@ -140,11 +117,11 @@ def dataset_availability(request, dataset_id): if r[0] == i: counts[key] += int(r[1]) - return JsonResponse({"status": "ok", "data": counts}, safe=False) + return JsonResponse({"status": "ok", "data": counts}) @csrf_exempt def dataset_metadata(request, dataset_id): meta = Dataset.objects.values_list("meta__collection_metadata", flat=True).get(id=dataset_id) - return JsonResponse({"status": "ok", "data": meta}, safe=False) + return JsonResponse({"status": "ok", "data": meta}) diff --git a/backend/tests/processor/test_views.py b/backend/tests/processor/test_views.py index 6bb98183..0330929c 100644 --- a/backend/tests/processor/test_views.py +++ b/backend/tests/processor/test_views.py @@ -26,11 +26,11 @@ def create(self, model, **kwargs): return obj def test_require_POST(self): - for path in ("create_dataset_filter",): + for path in ("create_dataset_filter", "dataset_start", "dataset_wipe"): with self.subTest(path=path): response = self.client.get(f"/api/{path}") - self.assertEqual(response.status_code, 400) + self.assertEqual(response.status_code, 405) @patch("processor.views.publish") def test_dataset_start(self, publish): @@ -40,7 +40,7 @@ def test_dataset_start(self, publish): self.assertEqual(response.status_code, 200) self.assertJSONEqual( - response.content, {"status": "ok", "data": {"message": "Dataset anything on Pelican started"}} + response.content, {"status": "ok", "data": {"message": "Started dataset 'anything' for collection 123"}} ) publish.assert_called_once_with('{"name": "anything", "collection_id": 123}', "ocds_kingfisher_extractor_init") @@ -51,7 +51,7 @@ def test_create_dataset_filter(self, publish): ) self.assertEqual(response.status_code, 200) - self.assertEqual(response.content, b"done") + self.assertJSONEqual(response.content, {"status": "ok"}) publish.assert_called_once_with( b'{"dataset_id_original": 123, "filter_message": {}}', "dataset_filter_extractor_init" ) @@ -61,15 +61,13 @@ def test_dataset_wipe(self, publish): response = self.client.post("/api/dataset_wipe", {"dataset_id": 123}, "application/json") self.assertEqual(response.status_code, 200) - self.assertJSONEqual( - response.content, {"status": "ok", "data": {"message": "Dataset id 123 on Pelican will be wiped"}} - ) + self.assertJSONEqual(response.content, {"status": "ok", "data": {"message": "Wiping dataset 123"}}) publish.assert_called_once_with('{"dataset_id": 123}', "wiper_init") def test_dataset_id(self): dataset = self.create(Dataset, name="anything") - response = self.client.post("/api/dataset_id", {"name": "anything"}, "application/json") + response = self.client.get("/api/dataset_id?name=anything") self.assertEqual(response.status_code, 200) self.assertJSONEqual(response.content, {"status": "ok", "data": dataset.pk}) diff --git a/docs/changelog.rst b/docs/changelog.rst index 5fa14aac..11872bef 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -3,6 +3,11 @@ Changelog This changelog only notes major changes, to notify other developers. +2021-11-09 +---------- + +- refactor: Start rewrite of processor API. + 2021-11-08 ----------