From 80d05622279233b9b46d0196455e49556c56cc83 Mon Sep 17 00:00:00 2001 From: Max Morlocke Date: Mon, 22 Jun 2020 22:36:27 -0400 Subject: [PATCH 1/4] add django rest framework to dev requirements --- requirements-dev.in | 1 + requirements-dev.txt | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/requirements-dev.in b/requirements-dev.in index 343538079..a0a431b12 100644 --- a/requirements-dev.in +++ b/requirements-dev.in @@ -1,6 +1,7 @@ black codecov django<2.3 +djangorestframework<3.12.0 freezegun mypy opentracing diff --git a/requirements-dev.txt b/requirements-dev.txt index 80c4b3570..9359b5bcb 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -13,10 +13,12 @@ chardet==3.0.4 # via requests click==7.0 # via black, pip-tools codecov==2.1.7 # via -r requirements-dev.in coverage==4.5.4 # via codecov, pytest-cov -django==2.2.13 # via -r requirements-dev.in +django==2.2.13 # via -r requirements-dev.in, djangorestframework +djangorestframework==3.11.0 # via -r requirements-dev.in fastdiff==0.2.0 # via snapshottest freezegun==0.3.15 # via -r requirements-dev.in idna==2.8 # via requests +importlib-metadata==1.6.1 # via pluggy, pytest isort==4.3.21 # via pylint lazy-object-proxy==1.4.3 # via astroid mccabe==0.6.1 # via pylint @@ -45,10 +47,14 @@ snapshottest==0.5.1 # via -r requirements-dev.in sqlparse==0.3.0 # via django termcolor==1.1.0 # via snapshottest toml==0.10.0 # via black, pylint -typed-ast==1.4.0 # via black, mypy +typed-ast==1.4.0 # via astroid, black, mypy typing-extensions==3.7.4.1 # via mypy urllib3==1.25.7 # via requests wasmer==0.3.0 # via fastdiff wcwidth==0.1.7 # via pytest werkzeug==1.0.1 # via -r requirements-dev.in wrapt==1.11.2 # via astroid +zipp==3.1.0 # via importlib-metadata + +# The following packages are considered to be unsafe in a requirements file: +# pip From 3749ec56f58e991d6bb84e154777bae5dd7bbbbb Mon Sep 17 00:00:00 2001 From: Max Morlocke Date: Mon, 22 Jun 2020 22:36:52 -0400 Subject: [PATCH 2/4] add a default django contrib for formatted errors --- ariadne/contrib/django/constants.py | 19 ++ ariadne/contrib/django/format_error.py | 114 ++++++++++++ tests/test_django_error_formatting.py | 241 +++++++++++++++++++++++++ 3 files changed, 374 insertions(+) create mode 100644 ariadne/contrib/django/constants.py create mode 100644 ariadne/contrib/django/format_error.py create mode 100644 tests/test_django_error_formatting.py diff --git a/ariadne/contrib/django/constants.py b/ariadne/contrib/django/constants.py new file mode 100644 index 000000000..5b9c9e591 --- /dev/null +++ b/ariadne/contrib/django/constants.py @@ -0,0 +1,19 @@ +FORMATTED_ERROR_MESSAGES = { + "ValidationError": {"short": "Invalid input", "details": None,}, + "ObjectDoesNotExist": { + "short": "Not found", + "details": "We were unable to locate the resource you requested.", + }, + "NotAuthenticated": { + "short": "Unauthorized", + "details": "You are not currently authorized to make this request.", + }, + "PermissionDenied": { + "short": "Forbidden", + "details": "You do not have permission to perform this action.", + }, + "MultipleObjectsReturned": { + "short": "Multiple found", + "details": "Multiple resources were returned when only a single resource was expected", + }, +} diff --git a/ariadne/contrib/django/format_error.py b/ariadne/contrib/django/format_error.py new file mode 100644 index 000000000..5c57e1a5d --- /dev/null +++ b/ariadne/contrib/django/format_error.py @@ -0,0 +1,114 @@ +import sys +from typing import Dict, Any + +import django.core.exceptions +from graphql import GraphQLError + +try: + import rest_framework.exceptions +except: + pass + +from ariadne import get_error_extension +from ariadne.contrib.django.constants import FORMATTED_ERROR_MESSAGES + + +def format_graphql_error( + error: GraphQLError, + error_field_name: str = "state", + constants: Dict[str, Any] = None, + debug: bool = False, +) -> Dict[str, Any]: + """ + We do not want to render arcane for-developer-only errors in the same way we render user facing errors. + So, we should use a custom field for user-feedback related errors. We will well established patterns and + practices used by ValidationError in core django and django rest framework. + Any non-form errors will also be rendered in non_field_errors. + """ + if constants is None: + constants = FORMATTED_ERROR_MESSAGES + + rest_framework_enabled = is_rest_framework_enabled() + + formatted = error.formatted + original_error = extract_original_error(error) + + if isinstance(original_error, django.core.exceptions.ValidationError): + formatted["message"] = constants["ValidationError"]["short"] + formatted[error_field_name] = get_full_django_validation_error_details( + original_error + ) + + elif rest_framework_enabled and isinstance( + original_error, rest_framework.exceptions.ValidationError + ): + formatted["message"] = constants["ValidationError"]["short"] + formatted[error_field_name] = original_error.get_full_details() + + elif isinstance(original_error, django.core.exceptions.ObjectDoesNotExist): + formatted["message"] = constants["ObjectDoesNotExist"]["short"] + formatted.setdefault(error_field_name, {}) + formatted[error_field_name].setdefault("non_field_errors", []) + formatted[error_field_name]["non_field_errors"].append( + constants["ObjectDoesNotExist"]["details"] + ) + + elif rest_framework_enabled and isinstance( + original_error, rest_framework.exceptions.NotAuthenticated + ): + formatted["message"] = constants["NotAuthenticated"]["short"] + formatted.setdefault(error_field_name, {}) + formatted[error_field_name].setdefault("non_field_errors", []) + formatted[error_field_name]["non_field_errors"].append( + constants["NotAuthenticated"]["details"] + ) + + elif any( + [ + isinstance(original_error, django.core.exceptions.PermissionDenied), + rest_framework_enabled + and isinstance(original_error, rest_framework.exceptions.PermissionDenied), + ] + ): + formatted["message"] = constants["PermissionDenied"]["short"] + formatted.setdefault(error_field_name, {}) + formatted[error_field_name].setdefault("non_field_errors", []) + formatted[error_field_name]["non_field_errors"].append( + constants["PermissionDenied"]["details"] + ) + + elif isinstance(original_error, django.core.exceptions.MultipleObjectsReturned): + formatted["message"] = constants["MultipleObjectsReturned"]["short"] + formatted.setdefault(error_field_name, {}) + formatted[error_field_name].setdefault("non_field_errors", []) + formatted[error_field_name]["non_field_errors"].append( + constants["MultipleObjectsReturned"]["details"] + ) + + if debug: + if "extensions" not in formatted: + formatted["extensions"] = {} + formatted["extensions"]["exception"] = get_error_extension(error) + return formatted + + +def extract_original_error(error: GraphQLError): + # Sometimes, ariadne nests the originally raised error. So, get to the bottom of it! + while getattr(error, "original_error", None): + error = error.original_error + return error + + +def get_full_django_validation_error_details( + error: django.core.exceptions.ValidationError, +) -> Dict[str, Any]: + if getattr(error, "message_dict", None) is not None: + return error.message_dict + elif getattr(error, "message", None) is not None: + return {"non_field_errors": [error.message]} + else: + return {"non_field_errors": error.messages} + + +def is_rest_framework_enabled() -> bool: + return "rest_framework.exceptions" in sys.modules diff --git a/tests/test_django_error_formatting.py b/tests/test_django_error_formatting.py new file mode 100644 index 000000000..bc71a6e2f --- /dev/null +++ b/tests/test_django_error_formatting.py @@ -0,0 +1,241 @@ +from unittest import mock + +import django.core.exceptions +import rest_framework.exceptions +from graphql import GraphQLError + +from ariadne.contrib.django.format_error import ( + format_graphql_error, + extract_original_error, + get_full_django_validation_error_details, + is_rest_framework_enabled, +) + + +def test_extract_original_error_no_original_error(): + graphql_error = GraphQLError(message="Meow") + extracted_error = extract_original_error(graphql_error) + assert extracted_error == graphql_error + + +def test_extract_original_error_single_layer(): + original_error = django.core.exceptions.ValidationError("Woof") + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = original_error + extracted_error = extract_original_error(graphql_error) + assert extracted_error == original_error + + +def test_extract_original_error_many_layers(): + original_error = django.core.exceptions.PermissionDenied("Moo") + intermediate_layer = GraphQLError(message="Oink") + graphql_error = GraphQLError(message="Meow") + intermediate_layer.original_error = original_error + graphql_error.original_error = intermediate_layer + extracted_error = extract_original_error(graphql_error) + assert extracted_error == original_error + + +def test_get_full_django_validation_error_details_plain_message(): + error = django.core.exceptions.ValidationError("meow") + error_details = get_full_django_validation_error_details(error) + assert error_details == {"non_field_errors": ["meow"]} + + +def test_get_full_django_validation_error_details_list_of_messages(): + error = django.core.exceptions.ValidationError(["meow", "woof"]) + error_details = get_full_django_validation_error_details(error) + assert error_details == {"non_field_errors": ["meow", "woof"]} + + +def test_get_full_django_validation_error_details_dictionary(): + error = django.core.exceptions.ValidationError({"cat": "meow", "dog": "woof"}) + error_details = get_full_django_validation_error_details(error) + assert error_details == {"cat": ["meow"], "dog": ["woof"]} + + +def test_format_graphql_error_no_original_error(): + graphql_error = GraphQLError(message="Meow") + formatted_error_messaging = format_graphql_error(graphql_error) + assert formatted_error_messaging == { + "message": "Meow", + "locations": None, + "path": None, + } + + +def test_format_graphql_error_django_validation_error(): + graphql_error = GraphQLError(message="Meow") + validation_error = django.core.exceptions.ValidationError( + {"cat": ["meow", "hiss"], "non_field_errors": ["oink"]} + ) + graphql_error.original_error = validation_error + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "message": "Invalid input", + "locations": None, + "path": None, + "state": {"cat": ["meow", "hiss"], "non_field_errors": ["oink"]}, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_rest_validation_error(): + graphql_error = GraphQLError(message="Meow") + validation_error = rest_framework.exceptions.ValidationError( + {"cat": ["meow", "hiss"], "non_field_errors": ["Test"]} + ) + graphql_error.original_error = validation_error + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "locations": None, + "message": "Invalid input", + "path": None, + "state": { + "cat": [ + { + "code": "invalid", + "message": rest_framework.exceptions.ErrorDetail( + string="meow", code="invalid" + ), + }, + { + "code": "invalid", + "message": rest_framework.exceptions.ErrorDetail( + string="hiss", code="invalid" + ), + }, + ], + "non_field_errors": [ + { + "code": "invalid", + "message": rest_framework.exceptions.ErrorDetail( + string="Test", code="invalid" + ), + }, + ], + }, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_authentication_error(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = rest_framework.exceptions.NotAuthenticated("") + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "locations": None, + "message": "Unauthorized", + "path": None, + "state": { + "non_field_errors": [ + "You are not currently authorized to make this request." + ] + }, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_object_does_not_exist_error(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = django.core.exceptions.ObjectDoesNotExist() + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "locations": None, + "message": "Not found", + "path": None, + "state": { + "non_field_errors": ["We were unable to locate the resource you requested."] + }, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_django_permission_denied(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = django.core.exceptions.PermissionDenied() + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "locations": None, + "message": "Forbidden", + "path": None, + "state": { + "non_field_errors": ["You do not have permission to perform this action."] + }, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_rest_permission_denied(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = rest_framework.exceptions.PermissionDenied("") + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "locations": None, + "message": "Forbidden", + "path": None, + "state": { + "non_field_errors": ["You do not have permission to perform this action."] + }, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_multiple_objects_error(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = django.core.exceptions.MultipleObjectsReturned() + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = { + "locations": None, + "message": "Multiple found", + "path": None, + "state": { + "non_field_errors": [ + "Multiple resources were returned when only a single resource was expected" + ] + }, + } + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_validation_error_rest_framework_disabled(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = rest_framework.exceptions.ValidationError( + {"cat": ["meow"]} + ) + with mock.patch( + "ariadne.contrib.django.format_error.is_rest_framework_enabled", + return_value=False, + ): + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = {"message": "Meow", "locations": None, "path": None} + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_not_authenticated_rest_framework_disabled(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = rest_framework.exceptions.NotAuthenticated("") + with mock.patch( + "ariadne.contrib.django.format_error.is_rest_framework_enabled", + return_value=False, + ): + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = {"message": "Meow", "locations": None, "path": None} + assert formatted_error_messaging == expected_value + + +def test_format_graphql_error_permission_denied_rest_framework_disabled(): + graphql_error = GraphQLError(message="Meow") + graphql_error.original_error = rest_framework.exceptions.PermissionDenied("") + with mock.patch( + "ariadne.contrib.django.format_error.is_rest_framework_enabled", + return_value=False, + ): + formatted_error_messaging = format_graphql_error(graphql_error) + expected_value = {"message": "Meow", "locations": None, "path": None} + assert formatted_error_messaging == expected_value + + +def test_is_rest_framework_enabled(): + value = is_rest_framework_enabled() + assert value == True From 17201974e4655c1cfc9d34d5c6b1f23a7fac2281 Mon Sep 17 00:00:00 2001 From: Max Morlocke Date: Mon, 22 Jun 2020 23:09:04 -0400 Subject: [PATCH 3/4] lint and test improvements --- ariadne/contrib/django/format_error.py | 17 +++++++++-------- tests/test_django_error_formatting.py | 13 ++++++++++++- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/ariadne/contrib/django/format_error.py b/ariadne/contrib/django/format_error.py index 5c57e1a5d..124193ba1 100644 --- a/ariadne/contrib/django/format_error.py +++ b/ariadne/contrib/django/format_error.py @@ -6,7 +6,7 @@ try: import rest_framework.exceptions -except: +except ImportError: pass from ariadne import get_error_extension @@ -20,8 +20,9 @@ def format_graphql_error( debug: bool = False, ) -> Dict[str, Any]: """ - We do not want to render arcane for-developer-only errors in the same way we render user facing errors. - So, we should use a custom field for user-feedback related errors. We will well established patterns and + We do not want to render arcane for-developer-only errors in the same way + we render user facing errors. So, we should use a custom field for + user-feedback related errors. We will well established patterns and practices used by ValidationError in core django and django rest framework. Any non-form errors will also be rendered in non_field_errors. """ @@ -86,8 +87,7 @@ def format_graphql_error( ) if debug: - if "extensions" not in formatted: - formatted["extensions"] = {} + formatted.setdefault("extensions", {}) formatted["extensions"]["exception"] = get_error_extension(error) return formatted @@ -103,11 +103,12 @@ def get_full_django_validation_error_details( error: django.core.exceptions.ValidationError, ) -> Dict[str, Any]: if getattr(error, "message_dict", None) is not None: - return error.message_dict + result = error.message_dict elif getattr(error, "message", None) is not None: - return {"non_field_errors": [error.message]} + result = {"non_field_errors": [error.message]} else: - return {"non_field_errors": error.messages} + result = {"non_field_errors": error.messages} + return result def is_rest_framework_enabled() -> bool: diff --git a/tests/test_django_error_formatting.py b/tests/test_django_error_formatting.py index bc71a6e2f..b8f99975a 100644 --- a/tests/test_django_error_formatting.py +++ b/tests/test_django_error_formatting.py @@ -236,6 +236,17 @@ def test_format_graphql_error_permission_denied_rest_framework_disabled(): assert formatted_error_messaging == expected_value +def test_format_error_with_debug(): + graphql_error = GraphQLError(message="Meow") + formatted_error_messaging = format_graphql_error(graphql_error, debug=True) + assert formatted_error_messaging == { + "message": "Meow", + "locations": None, + "path": None, + "extensions": {"exception": None}, + } + + def test_is_rest_framework_enabled(): value = is_rest_framework_enabled() - assert value == True + assert value From 6540f2fce48e03487e78b1a473fe79d97c32c92d Mon Sep 17 00:00:00 2001 From: Max Morlocke Date: Mon, 22 Jun 2020 23:40:55 -0400 Subject: [PATCH 4/4] corrected type definitions --- ariadne/contrib/django/constants.py | 2 +- ariadne/contrib/django/format_error.py | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/ariadne/contrib/django/constants.py b/ariadne/contrib/django/constants.py index 5b9c9e591..55e56d89b 100644 --- a/ariadne/contrib/django/constants.py +++ b/ariadne/contrib/django/constants.py @@ -1,4 +1,4 @@ -FORMATTED_ERROR_MESSAGES = { +FORMATTED_ERROR_MESSAGES: dict = { "ValidationError": {"short": "Invalid input", "details": None,}, "ObjectDoesNotExist": { "short": "Not found", diff --git a/ariadne/contrib/django/format_error.py b/ariadne/contrib/django/format_error.py index 124193ba1..065477ad2 100644 --- a/ariadne/contrib/django/format_error.py +++ b/ariadne/contrib/django/format_error.py @@ -1,5 +1,4 @@ import sys -from typing import Dict, Any import django.core.exceptions from graphql import GraphQLError @@ -16,9 +15,9 @@ def format_graphql_error( error: GraphQLError, error_field_name: str = "state", - constants: Dict[str, Any] = None, + constants: dict = None, debug: bool = False, -) -> Dict[str, Any]: +) -> dict: """ We do not want to render arcane for-developer-only errors in the same way we render user facing errors. So, we should use a custom field for @@ -92,16 +91,16 @@ def format_graphql_error( return formatted -def extract_original_error(error: GraphQLError): +def extract_original_error(error: GraphQLError) -> Exception: # Sometimes, ariadne nests the originally raised error. So, get to the bottom of it! while getattr(error, "original_error", None): - error = error.original_error + error = getattr(error, "original_error") return error def get_full_django_validation_error_details( error: django.core.exceptions.ValidationError, -) -> Dict[str, Any]: +) -> dict: if getattr(error, "message_dict", None) is not None: result = error.message_dict elif getattr(error, "message", None) is not None: