diff --git a/CHANGELOG.md b/CHANGELOG.md index 2efa0f40..97546f10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ any parts of the framework not mentioned in the documentation should generally b * Avoid `AttributeError` for PUT and PATCH methods when using `APIView` * Clear many-to-many relationships instead of deleting related objects during PATCH on `RelationshipView` * Allow POST, PATCH, DELETE for actions in `ReadOnlyModelViewSet`. It was problematic since 2.8.0. +* Properly format nested errors ### Changed diff --git a/example/tests/snapshots/__init__.py b/example/tests/snapshots/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/example/tests/snapshots/snap_test_errors.py b/example/tests/snapshots/snap_test_errors.py new file mode 100644 index 00000000..d77c1e24 --- /dev/null +++ b/example/tests/snapshots/snap_test_errors.py @@ -0,0 +1,121 @@ +# -*- coding: utf-8 -*- +# snapshottest: v1 - https://goo.gl/zC4yUc +from __future__ import unicode_literals + +from snapshottest import Snapshot + + +snapshots = Snapshot() + +snapshots['test_first_level_attribute_error 1'] = { + 'errors': [ + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/headline' + }, + 'status': '400' + } + ] +} + +snapshots['test_first_level_custom_attribute_error 1'] = { + 'errors': [ + { + 'detail': 'Too short', + 'source': { + 'pointer': '/data/attributes/body-text' + }, + 'title': 'Too Short title' + } + ] +} + +snapshots['test_second_level_array_error 1'] = { + 'errors': [ + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/comments/0/body' + }, + 'status': '400' + } + ] +} + +snapshots['test_second_level_dict_error 1'] = { + 'errors': [ + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/comment/body' + }, + 'status': '400' + } + ] +} + +snapshots['test_third_level_array_error 1'] = { + 'errors': [ + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/comments/0/attachments/0/data' + }, + 'status': '400' + } + ] +} + +snapshots['test_third_level_custom_array_error 1'] = { + 'errors': [ + { + 'code': 'invalid', + 'detail': 'Too short data', + 'source': { + 'pointer': '/data/attributes/comments/0/attachments/0/data' + }, + 'status': '400' + } + ] +} + +snapshots['test_third_level_dict_error 1'] = { + 'errors': [ + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/comments/0/attachment/data' + }, + 'status': '400' + } + ] +} + +snapshots['test_many_third_level_dict_errors 1'] = { + 'errors': [ + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/comments/0/attachment/data' + }, + 'status': '400' + }, + { + 'code': 'required', + 'detail': 'This field is required.', + 'source': { + 'pointer': '/data/attributes/comments/0/body' + }, + 'status': '400' + } + ] +} + +snapshots['test_deprecation_warning 1'] = 'Rendering nested serializer as relationship is deprecated. Use `ResourceRelatedField` instead if DummyNestedSerializer in serializer example.tests.test_errors.test_deprecation_warning..DummySerializer should remain a relationship. Otherwise set JSON_API_SERIALIZE_NESTED_SERIALIZERS_AS_ATTRIBUTE to True to render nested serializer as nested json attribute' diff --git a/example/tests/test_errors.py b/example/tests/test_errors.py new file mode 100644 index 00000000..75bc8842 --- /dev/null +++ b/example/tests/test_errors.py @@ -0,0 +1,240 @@ +import pytest +from django.conf.urls import url +from django.test import override_settings +from django.urls import reverse +from rest_framework import views + +from rest_framework_json_api import serializers + +from example.models import Blog + + +# serializers +class CommentAttachmentSerializer(serializers.Serializer): + data = serializers.CharField(allow_null=False, required=True) + + def validate_data(self, value): + if value and len(value) < 10: + raise serializers.ValidationError('Too short data') + + +class CommentSerializer(serializers.Serializer): + attachments = CommentAttachmentSerializer(many=True, required=False) + attachment = CommentAttachmentSerializer(required=False) + one_more_attachment = CommentAttachmentSerializer(required=False) + body = serializers.CharField(allow_null=False, required=True) + + +class EntrySerializer(serializers.Serializer): + blog = serializers.IntegerField() + comments = CommentSerializer(many=True, required=False) + comment = CommentSerializer(required=False) + headline = serializers.CharField(allow_null=True, required=True) + body_text = serializers.CharField() + + def validate(self, attrs): + body_text = attrs['body_text'] + if len(body_text) < 5: + raise serializers.ValidationError({'body_text': { + 'title': 'Too Short title', 'detail': 'Too short'} + }) + + +# view +class DummyTestView(views.APIView): + serializer_class = EntrySerializer + resource_name = 'entries' + + def post(self, request, *args, **kwargs): + serializer = self.serializer_class(data=request.data) + serializer.is_valid(raise_exception=True) + + +urlpatterns = [ + url('entries-nested', DummyTestView.as_view(), + name='entries-nested-list') +] + + +@pytest.fixture(scope='function') +def some_blog(db): + return Blog.objects.create(name='Some Blog', tagline="It's a blog") + + +def perform_error_test(client, data): + with override_settings( + JSON_API_SERIALIZE_NESTED_SERIALIZERS_AS_ATTRIBUTE=True, + ROOT_URLCONF=__name__ + ): + url = reverse('entries-nested-list') + response = client.post(url, data=data) + + return response.json() + + +def test_first_level_attribute_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + } + } + } + snapshot.assert_match(perform_error_test(client, data)) + + +def test_first_level_custom_attribute_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'body-text': 'body', + 'headline': 'headline' + } + } + } + with override_settings(JSON_API_FORMAT_FIELD_NAMES='dasherize'): + snapshot.assert_match(perform_error_test(client, data)) + + +def test_second_level_array_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + 'headline': 'headline', + 'comments': [ + { + } + ] + } + } + } + + snapshot.assert_match(perform_error_test(client, data)) + + +def test_second_level_dict_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + 'headline': 'headline', + 'comment': {} + } + } + } + + snapshot.assert_match(perform_error_test(client, data)) + + +def test_third_level_array_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + 'headline': 'headline', + 'comments': [ + { + 'body': 'test comment', + 'attachments': [ + { + } + ] + } + ] + } + } + } + + snapshot.assert_match(perform_error_test(client, data)) + + +def test_third_level_custom_array_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + 'headline': 'headline', + 'comments': [ + { + 'body': 'test comment', + 'attachments': [ + { + 'data': 'text' + } + ] + } + ] + } + } + } + + snapshot.assert_match(perform_error_test(client, data)) + + +def test_third_level_dict_error(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + 'headline': 'headline', + 'comments': [ + { + 'body': 'test comment', + 'attachment': {} + } + ] + } + } + } + + snapshot.assert_match(perform_error_test(client, data)) + + +def test_many_third_level_dict_errors(client, some_blog, snapshot): + data = { + 'data': { + 'type': 'entries', + 'attributes': { + 'blog': some_blog.pk, + 'bodyText': 'body_text', + 'headline': 'headline', + 'comments': [ + { + 'attachment': {} + } + ] + } + } + } + + snapshot.assert_match(perform_error_test(client, data)) + + +@pytest.mark.filterwarnings('default::DeprecationWarning:rest_framework_json_api.serializers') +def test_deprecation_warning(recwarn, settings, snapshot): + settings.JSON_API_SERIALIZE_NESTED_SERIALIZERS_AS_ATTRIBUTE = False + + class DummyNestedSerializer(serializers.Serializer): + field = serializers.CharField() + + class DummySerializer(serializers.Serializer): + nested = DummyNestedSerializer(many=True) + + assert len(recwarn) == 1 + warning = recwarn.pop(DeprecationWarning) + snapshot.assert_match(str(warning.message)) diff --git a/example/tests/test_generic_viewset.py b/example/tests/test_generic_viewset.py index a9abdd53..ba315740 100644 --- a/example/tests/test_generic_viewset.py +++ b/example/tests/test_generic_viewset.py @@ -95,11 +95,6 @@ def test_custom_validation_exceptions(self): """ expected = { 'errors': [ - { - 'id': 'armageddon101', - 'detail': 'Hey! You need a last name!', - 'meta': 'something', - }, { 'status': '400', 'source': { @@ -108,6 +103,12 @@ def test_custom_validation_exceptions(self): 'detail': 'Enter a valid email address.', 'code': 'invalid', }, + { + 'id': 'armageddon101', + 'detail': 'Hey! You need a last name!', + 'meta': 'something', + 'source': {'pointer': '/data/attributes/lastName'} + }, ] } response = self.client.post('/identities', { diff --git a/requirements/requirements-testing.txt b/requirements/requirements-testing.txt index 357bf12a..6f02437a 100644 --- a/requirements/requirements-testing.txt +++ b/requirements/requirements-testing.txt @@ -5,3 +5,4 @@ pytest==6.0.1 pytest-cov==2.10.1 pytest-django==3.9.0 pytest-factoryboy==2.0.3 +snapshottest==0.5.1 diff --git a/rest_framework_json_api/utils.py b/rest_framework_json_api/utils.py index b3932651..2443575f 100644 --- a/rest_framework_json_api/utils.py +++ b/rest_framework_json_api/utils.py @@ -312,29 +312,25 @@ def format_drf_errors(response, context, exc): # handle generic errors. ValidationError('test') in a view for example if isinstance(response.data, list): for message in response.data: - errors.append(format_error_object(message, '/data', response)) + errors.extend(format_error_object(message, '/data', response)) # handle all errors thrown from serializers else: for field, error in response.data.items(): field = format_value(field) pointer = '/data/attributes/{}'.format(field) - # see if they passed a dictionary to ValidationError manually - if isinstance(error, dict): - errors.append(error) - elif isinstance(exc, Http404) and isinstance(error, str): + if isinstance(exc, Http404) and isinstance(error, str): # 404 errors don't have a pointer - errors.append(format_error_object(error, None, response)) + errors.extend(format_error_object(error, None, response)) elif isinstance(error, str): classes = inspect.getmembers(exceptions, inspect.isclass) # DRF sets the `field` to 'detail' for its own exceptions if isinstance(exc, tuple(x[1] for x in classes)): pointer = '/data' - errors.append(format_error_object(error, pointer, response)) + errors.extend(format_error_object(error, pointer, response)) elif isinstance(error, list): - for message in error: - errors.append(format_error_object(message, pointer, response)) + errors.extend(format_error_object(error, pointer, response)) else: - errors.append(format_error_object(error, pointer, response)) + errors.extend(format_error_object(error, pointer, response)) context['view'].resource_name = 'errors' response.data = errors @@ -343,18 +339,49 @@ def format_drf_errors(response, context, exc): def format_error_object(message, pointer, response): - error_obj = { - 'detail': message, - 'status': encoding.force_str(response.status_code), - } - if pointer is not None: - error_obj['source'] = { - 'pointer': pointer, + errors = [] + if isinstance(message, dict): + + # as there is no required field in error object we check that all fields are string + # except links and source which might be a dict + is_custom_error = all([ + isinstance(value, str) + for key, value in message.items() if key not in ['links', 'source'] + ]) + + if is_custom_error: + if 'source' not in message: + message['source'] = {} + message['source'] = { + 'pointer': pointer, + } + errors.append(message) + else: + for k, v in message.items(): + errors.extend(format_error_object(v, pointer + '/{}'.format(k), response)) + elif isinstance(message, list): + for num, error in enumerate(message): + if isinstance(error, (list, dict)): + new_pointer = pointer + '/{}'.format(num) + else: + new_pointer = pointer + if error: + errors.extend(format_error_object(error, new_pointer, response)) + else: + error_obj = { + 'detail': message, + 'status': encoding.force_str(response.status_code), } - code = getattr(message, "code", None) - if code is not None: - error_obj['code'] = code - return error_obj + if pointer is not None: + error_obj['source'] = { + 'pointer': pointer, + } + code = getattr(message, "code", None) + if code is not None: + error_obj['code'] = code + errors.append(error_obj) + + return errors def format_errors(data): diff --git a/setup.cfg b/setup.cfg index ef25b7bd..d8247c1d 100644 --- a/setup.cfg +++ b/setup.cfg @@ -8,6 +8,7 @@ universal = 1 ignore = F405,W504 max-line-length = 100 exclude = + snapshots build/lib, docs/conf.py, migrations,