From 75c3e1075154439787354d72381215a0c77ebc87 Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Fri, 5 May 2023 10:21:22 +0200 Subject: [PATCH 1/5] Catch API exception and serve helpful error response --- apps/greencheck/api/legacy_views.py | 21 ++++++++++++-- .../greencheck/tests/test_legacy_directory.py | 28 +++++++++++++++++-- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/apps/greencheck/api/legacy_views.py b/apps/greencheck/api/legacy_views.py index 24eff7af..181ad9b3 100644 --- a/apps/greencheck/api/legacy_views.py +++ b/apps/greencheck/api/legacy_views.py @@ -5,6 +5,7 @@ from django.views.decorators.cache import cache_page from django_countries import countries from rest_framework import response +from rest_framework import exceptions from rest_framework.decorators import api_view, permission_classes, renderer_classes from rest_framework.permissions import AllowAny from rest_framework_jsonp.renderers import JSONPRenderer @@ -12,7 +13,7 @@ from ..domain_check import GreenDomainChecker from ..models import Greencheck, GreenDomain from .legacy_image_view import legacy_greencheck_image -from ..serializers import GreenDomainSerializer +from ..serializers import GreenDomainSerializer, HostingDocumentSerializer logger = logging.getLogger(__name__) @@ -112,7 +113,6 @@ def directory(request): country_list = {} for country in countries: - country_obj = { "iso": country.code, "tld": f".{country.code.lower()}", @@ -136,8 +136,22 @@ def directory_provider(self, id): what they do, and evidence supporting their sustainability claims """ - provider = Hostingprovider.objects.get(pk=id) + try: + provider_id = int(id) + except ValueError as ex: + logger.warning(ex) + raise exceptions.NotAcceptable( + ( + "You need to send a valid numeric ID to identify the " + f"provider you are requesting information about. Received ID was: '{id}'" + ) + ) + + provider = Hostingprovider.objects.get(pk=provider_id) datacenters = [dc.legacy_representation() for dc in provider.datacenter.all() if dc] + supporting_evidence = HostingDocumentSerializer( + provider.public_supporting_evidence(), many=True + ).data # basic case, no datacenters or certificates provider_dict = { @@ -153,6 +167,7 @@ def directory_provider(self, id): "energyprovider": None, "partner": provider.partner, "datacenters": datacenters, + "supporting_evidence": supporting_evidence, } return response.Response([provider_dict]) diff --git a/apps/greencheck/tests/test_legacy_directory.py b/apps/greencheck/tests/test_legacy_directory.py index 081b6e4d..9443b348 100644 --- a/apps/greencheck/tests/test_legacy_directory.py +++ b/apps/greencheck/tests/test_legacy_directory.py @@ -21,7 +21,6 @@ @pytest.fixture def green_dc_certificate(datacenter): - return DatacenterCertificate( energyprovider="Some Energy Co.", mainenergy_type="mixed", @@ -138,7 +137,6 @@ class TestGreenWebDirectoryDetail: """ def test_directory_provider(self, db, hosting_provider_a, client): - hosting_provider_a.save() url_path = reverse("legacy-directory-detail", args=[hosting_provider_a.id]) @@ -255,3 +253,29 @@ def test_directory_provider_with_certificates( ]: assert key in cert + def test_directory_provider_raises_against_undefined_provider( + self, db, hosting_provider_a, client + ): + """ + Make sure that when client side code tries to request a provider + with the id of 'undefined' we serve an appropriate error + 40x response, rather than raising an helpful 500 server error + """ + # Given: a valid hosting provider, but an invalid id of 'undefined' from + # client side code making requests + hosting_provider_a.save() + url_path = reverse("legacy-directory-detail", args=["undefined"]) + + # When: I send an API reqeest + resp = client.get(url_path) + + # Then: I should receive a helpful API error response + assert resp.status_code == 406 + + # And: with hints about how to fix my request + error_detail = resp.data["detail"] + error_message = str(error_detail) + + assert error_detail.code == "not_acceptable" + assert "You need to send a valid numeric ID" in error_message + assert "Received ID was: 'undefined'" in error_message From 2382f72effae5e41721003d5dad9316c3f32c58c Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Fri, 5 May 2023 10:29:34 +0200 Subject: [PATCH 2/5] Tidy up formatting with Ruff Also remove public evidence API tweak - better to add that separately --- apps/greencheck/api/legacy_views.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/apps/greencheck/api/legacy_views.py b/apps/greencheck/api/legacy_views.py index 181ad9b3..3271ecb5 100644 --- a/apps/greencheck/api/legacy_views.py +++ b/apps/greencheck/api/legacy_views.py @@ -1,7 +1,7 @@ import json import logging -from apps.accounts.models import Hostingprovider, Datacenter +from apps.accounts.models import Hostingprovider from django.views.decorators.cache import cache_page from django_countries import countries from rest_framework import response @@ -12,8 +12,7 @@ from ..domain_check import GreenDomainChecker from ..models import Greencheck, GreenDomain -from .legacy_image_view import legacy_greencheck_image -from ..serializers import GreenDomainSerializer, HostingDocumentSerializer +from ..serializers import GreenDomainSerializer logger = logging.getLogger(__name__) @@ -149,9 +148,6 @@ def directory_provider(self, id): provider = Hostingprovider.objects.get(pk=provider_id) datacenters = [dc.legacy_representation() for dc in provider.datacenter.all() if dc] - supporting_evidence = HostingDocumentSerializer( - provider.public_supporting_evidence(), many=True - ).data # basic case, no datacenters or certificates provider_dict = { From 02a7506ea6c3f54b279da46f7fdaaaf02401e010 Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Fri, 5 May 2023 10:47:02 +0200 Subject: [PATCH 3/5] Add import back in Ruff thought it wasn't used, but we use legacy_views as a module for importing all the old legacy views from --- apps/greencheck/api/legacy_views.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/apps/greencheck/api/legacy_views.py b/apps/greencheck/api/legacy_views.py index 3271ecb5..12a44709 100644 --- a/apps/greencheck/api/legacy_views.py +++ b/apps/greencheck/api/legacy_views.py @@ -1,7 +1,7 @@ import json import logging -from apps.accounts.models import Hostingprovider + from django.views.decorators.cache import cache_page from django_countries import countries from rest_framework import response @@ -10,10 +10,15 @@ from rest_framework.permissions import AllowAny from rest_framework_jsonp.renderers import JSONPRenderer +from ...accounts.models import Hostingprovider from ..domain_check import GreenDomainChecker from ..models import Greencheck, GreenDomain from ..serializers import GreenDomainSerializer +# we import legacy_greencheck_image, to provide one module to import all +# legacy API views from +from .legacy_image_view import legacy_greencheck_image # noqa + logger = logging.getLogger(__name__) checker = GreenDomainChecker() @@ -142,7 +147,8 @@ def directory_provider(self, id): raise exceptions.NotAcceptable( ( "You need to send a valid numeric ID to identify the " - f"provider you are requesting information about. Received ID was: '{id}'" + "provider you are requesting information about. " + f"Received ID was: '{id}'" ) ) @@ -163,7 +169,6 @@ def directory_provider(self, id): "energyprovider": None, "partner": provider.partner, "datacenters": datacenters, - "supporting_evidence": supporting_evidence, } return response.Response([provider_dict]) From 98717d7f45f84226fff5c8ada08d7964a09b2345 Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Fri, 5 May 2023 15:52:34 +0200 Subject: [PATCH 4/5] Update the HTTP error status code --- apps/greencheck/api/legacy_views.py | 2 +- apps/greencheck/tests/test_legacy_directory.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/greencheck/api/legacy_views.py b/apps/greencheck/api/legacy_views.py index 12a44709..5b6488e3 100644 --- a/apps/greencheck/api/legacy_views.py +++ b/apps/greencheck/api/legacy_views.py @@ -144,7 +144,7 @@ def directory_provider(self, id): provider_id = int(id) except ValueError as ex: logger.warning(ex) - raise exceptions.NotAcceptable( + raise exceptions.ParseError( ( "You need to send a valid numeric ID to identify the " "provider you are requesting information about. " diff --git a/apps/greencheck/tests/test_legacy_directory.py b/apps/greencheck/tests/test_legacy_directory.py index 9443b348..3298be09 100644 --- a/apps/greencheck/tests/test_legacy_directory.py +++ b/apps/greencheck/tests/test_legacy_directory.py @@ -270,7 +270,7 @@ def test_directory_provider_raises_against_undefined_provider( resp = client.get(url_path) # Then: I should receive a helpful API error response - assert resp.status_code == 406 + assert resp.status_code == 400 # And: with hints about how to fix my request error_detail = resp.data["detail"] From 351891077a1355fdbf3c322080f858cf35f46fae Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Fri, 5 May 2023 16:03:56 +0200 Subject: [PATCH 5/5] Oops. Fix the error code too --- apps/greencheck/tests/test_legacy_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/greencheck/tests/test_legacy_directory.py b/apps/greencheck/tests/test_legacy_directory.py index 3298be09..b4a3d482 100644 --- a/apps/greencheck/tests/test_legacy_directory.py +++ b/apps/greencheck/tests/test_legacy_directory.py @@ -276,6 +276,6 @@ def test_directory_provider_raises_against_undefined_provider( error_detail = resp.data["detail"] error_message = str(error_detail) - assert error_detail.code == "not_acceptable" + assert error_detail.code == "parse_error" assert "You need to send a valid numeric ID" in error_message assert "Received ID was: 'undefined'" in error_message