From 8c29efef48378a9484289fa1ae3b076b65dc6a7f Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Mon, 7 Dec 2015 10:48:53 +0100 Subject: [PATCH 1/9] Introduce error code for validation errors. This patch is meant to fix #3111, regarding comments made to #3137 and #3169. The `ValidationError` will now contain a `code` attribute. Before this patch, `ValidationError.detail` only contained a `dict` with values equal to a `list` of string error messages or directly a `list` containing string error messages. Now, the string error messages are replaced with `ValidationError`. This means that, depending on the case, you will not only get a string back but a all object containing both the error message and the error code, respectively `ValidationError.detail` and `ValidationError.code`. It is important to note that the `code` attribute is not relevant when the `ValidationError` represents a combination of errors and hence is `None` in such cases. The main benefit of this change is that the error message and error code are now accessible the custom exception handler and can be used to format the error response. An custom exception handler example is available in the `TestValidationErrorWithCode` test. --- rest_framework/authtoken/serializers.py | 15 +++- rest_framework/exceptions.py | 17 ++++- rest_framework/fields.py | 12 ++-- rest_framework/renderers.py | 15 ++++ rest_framework/response.py | 1 - rest_framework/serializers.py | 21 ++++-- rest_framework/validators.py | 16 +++-- rest_framework/views.py | 13 +++- tests/test_bound_fields.py | 3 +- tests/test_fields.py | 14 +++- tests/test_model_serializer.py | 2 +- tests/test_relations_hyperlink.py | 6 +- tests/test_relations_pk.py | 7 +- tests/test_relations_slug.py | 6 +- tests/test_serializer.py | 33 ++++++++- tests/test_serializer_bulk_update.py | 33 +++++---- tests/test_serializer_lists.py | 5 +- tests/test_serializer_nested.py | 10 +-- tests/test_validation.py | 15 ++-- tests/test_validation_error.py | 95 +++++++++++++++++++++++++ tests/test_validators.py | 23 +++--- 21 files changed, 287 insertions(+), 75 deletions(-) create mode 100644 tests/test_validation_error.py diff --git a/rest_framework/authtoken/serializers.py b/rest_framework/authtoken/serializers.py index 8a295c03e9..caf5b64b3d 100644 --- a/rest_framework/authtoken/serializers.py +++ b/rest_framework/authtoken/serializers.py @@ -18,13 +18,22 @@ def validate(self, attrs): if user: if not user.is_active: msg = _('User account is disabled.') - raise serializers.ValidationError(msg) + raise serializers.ValidationError( + msg, + code='authorization' + ) else: msg = _('Unable to log in with provided credentials.') - raise serializers.ValidationError(msg) + raise serializers.ValidationError( + msg, + code='authorization' + ) else: msg = _('Must include "username" and "password".') - raise serializers.ValidationError(msg) + raise serializers.ValidationError( + msg, + code='authorization' + ) attrs['user'] = user return attrs diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 8447a9dedc..f8330bf170 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -58,6 +58,14 @@ def __str__(self): return self.detail +def build_error_from_django_validation_error(exc_info): + code = getattr(exc_info, 'code', None) or 'invalid' + return [ + ValidationError(msg, code=code) + for msg in exc_info.messages + ] + + # The recommended style for using `ValidationError` is to keep it namespaced # under `serializers`, in order to minimize potential confusion with Django's # built in `ValidationError`. For example: @@ -68,12 +76,17 @@ def __str__(self): class ValidationError(APIException): status_code = status.HTTP_400_BAD_REQUEST - def __init__(self, detail): + def __init__(self, detail, code=None): # For validation errors the 'detail' key is always required. # The details should always be coerced to a list if not already. if not isinstance(detail, dict) and not isinstance(detail, list): detail = [detail] - self.detail = _force_text_recursive(detail) + elif isinstance(detail, dict) or (detail and isinstance(detail[0], ValidationError)): + assert code is None, ( + 'The `code` argument must not be set for compound errors.') + + self.detail = detail + self.code = code def __str__(self): return six.text_type(self.detail) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 8541bc43a0..e9044513be 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -31,7 +31,9 @@ MinValueValidator, duration_string, parse_duration, unicode_repr, unicode_to_repr ) -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import ( + ValidationError, build_error_from_django_validation_error +) from rest_framework.settings import api_settings from rest_framework.utils import html, humanize_datetime, representation @@ -501,9 +503,11 @@ def run_validators(self, value): # attempting to accumulate a list of errors. if isinstance(exc.detail, dict): raise - errors.extend(exc.detail) + errors.append(ValidationError(exc.detail, code=exc.code)) except DjangoValidationError as exc: - errors.extend(exc.messages) + errors.extend( + build_error_from_django_validation_error(exc) + ) if errors: raise ValidationError(errors) @@ -541,7 +545,7 @@ def fail(self, key, **kwargs): msg = MISSING_ERROR_MESSAGE.format(class_name=class_name, key=key) raise AssertionError(msg) message_string = msg.format(**kwargs) - raise ValidationError(message_string) + raise ValidationError(message_string, code=key) @cached_property def root(self): diff --git a/rest_framework/renderers.py b/rest_framework/renderers.py index dafaf7de62..d52ebaea98 100644 --- a/rest_framework/renderers.py +++ b/rest_framework/renderers.py @@ -493,6 +493,21 @@ def get_rendered_html_form(self, data, view, method, request): if hasattr(serializer, 'initial_data'): serializer.is_valid() + # Convert ValidationError to unicode string + # This is mainly a hack to monkey patch the errors and make the form renderer happy... + errors = OrderedDict() + for field_name, values in serializer.errors.items(): + if isinstance(values, list): + errors[field_name] = values + continue + + errors[field_name] = [] + for value in values.detail: + for message in value.detail: + errors[field_name].append(message) + + serializer._errors = errors + form_renderer = self.form_renderer_class() return form_renderer.render( serializer.data, diff --git a/rest_framework/response.py b/rest_framework/response.py index 0e97668eb4..9942dd576c 100644 --- a/rest_framework/response.py +++ b/rest_framework/response.py @@ -38,7 +38,6 @@ def __init__(self, data=None, status=None, '`.error`. representation.' ) raise AssertionError(msg) - self.data = data self.template_name = template_name self.exception = exception diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 99d36a8a54..e3206793e2 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -20,6 +20,7 @@ from django.utils.functional import cached_property from django.utils.translation import ugettext_lazy as _ +from rest_framework import exceptions from rest_framework.compat import DurationField as ModelDurationField from rest_framework.compat import JSONField as ModelJSONField from rest_framework.compat import postgres_fields, unicode_to_repr @@ -300,7 +301,8 @@ def get_validation_error_detail(exc): # exception class as well for simpler compat. # Eg. Calling Model.clean() explicitly inside Serializer.validate() return { - api_settings.NON_FIELD_ERRORS_KEY: list(exc.messages) + api_settings.NON_FIELD_ERRORS_KEY: + exceptions.build_error_from_django_validation_error(exc) } elif isinstance(exc.detail, dict): # If errors may be a dict we use the standard {key: list of values}. @@ -422,8 +424,9 @@ def to_internal_value(self, data): message = self.error_messages['invalid'].format( datatype=type(data).__name__ ) + error = ValidationError(message, code='invalid') raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [message] + api_settings.NON_FIELD_ERRORS_KEY: [error] }) ret = OrderedDict() @@ -438,9 +441,11 @@ def to_internal_value(self, data): if validate_method is not None: validated_value = validate_method(validated_value) except ValidationError as exc: - errors[field.field_name] = exc.detail + errors[field.field_name] = exc except DjangoValidationError as exc: - errors[field.field_name] = list(exc.messages) + errors[field.field_name] = ( + exceptions.build_error_from_django_validation_error(exc) + ) except SkipField: pass else: @@ -575,14 +580,18 @@ def to_internal_value(self, data): message = self.error_messages['not_a_list'].format( input_type=type(data).__name__ ) + error = ValidationError( + message, + code='not_a_list' + ) raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [message] + api_settings.NON_FIELD_ERRORS_KEY: [error] }) if not self.allow_empty and len(data) == 0: message = self.error_messages['empty'] raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [message] + api_settings.NON_FIELD_ERRORS_KEY: [ValidationError(message, code='empty_not_allowed')] }) ret = [] diff --git a/rest_framework/validators.py b/rest_framework/validators.py index a21f67e60e..27148cedc9 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -60,7 +60,7 @@ def __call__(self, value): queryset = self.filter_queryset(value, queryset) queryset = self.exclude_current_instance(queryset) if queryset.exists(): - raise ValidationError(self.message) + raise ValidationError(self.message, code='unique') def __repr__(self): return unicode_to_repr('<%s(queryset=%s)>' % ( @@ -101,7 +101,9 @@ def enforce_required_fields(self, attrs): return missing = { - field_name: self.missing_message + field_name: ValidationError( + self.missing_message, + code='required') for field_name in self.fields if field_name not in attrs } @@ -147,7 +149,8 @@ def __call__(self, attrs): ] if None not in checked_values and queryset.exists(): field_names = ', '.join(self.fields) - raise ValidationError(self.message.format(field_names=field_names)) + raise ValidationError(self.message.format(field_names=field_names), + code='unique') def __repr__(self): return unicode_to_repr('<%s(queryset=%s, fields=%s)>' % ( @@ -185,7 +188,9 @@ def enforce_required_fields(self, attrs): 'required' state on the fields they are applied to. """ missing = { - field_name: self.missing_message + field_name: ValidationError( + self.missing_message, + code='required') for field_name in [self.field, self.date_field] if field_name not in attrs } @@ -211,7 +216,8 @@ def __call__(self, attrs): queryset = self.exclude_current_instance(attrs, queryset) if queryset.exists(): message = self.message.format(date_field=self.date_field) - raise ValidationError({self.field: message}) + error = ValidationError(message, code='unique') + raise ValidationError({self.field: error}) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, field=%s, date_field=%s)>' % ( diff --git a/rest_framework/views.py b/rest_framework/views.py index 56e3c4e499..926a3d00e9 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -15,6 +15,7 @@ from rest_framework import exceptions, status from rest_framework.compat import set_rollback +from rest_framework.exceptions import ValidationError, _force_text_recursive from rest_framework.request import Request from rest_framework.response import Response from rest_framework.settings import api_settings @@ -69,7 +70,17 @@ def exception_handler(exc, context): if getattr(exc, 'wait', None): headers['Retry-After'] = '%d' % exc.wait - if isinstance(exc.detail, (list, dict)): + if isinstance(exc.detail, list): + data = _force_text_recursive([item.detail if isinstance(item, ValidationError) else item + for item in exc.detai]) + elif isinstance(exc.detail, dict): + for field_name, e in exc.detail.items(): + if hasattr(e, 'detail') and isinstance(e.detail[0], ValidationError): + exc.detail[field_name] = e.detail[0].detail + elif isinstance(e, ValidationError): + exc.detail[field_name] = e.detail + else: + exc.detail[field_name] = e data = exc.detail else: data = {'detail': exc.detail} diff --git a/tests/test_bound_fields.py b/tests/test_bound_fields.py index f2fac8f0d2..0fd8e6f5d3 100644 --- a/tests/test_bound_fields.py +++ b/tests/test_bound_fields.py @@ -39,7 +39,8 @@ class ExampleSerializer(serializers.Serializer): serializer.is_valid() assert serializer['text'].value == 'x' * 1000 - assert serializer['text'].errors == ['Ensure this field has no more than 100 characters.'] + assert serializer['text'].errors.detail[0].detail == ['Ensure this field has no more than 100 characters.'] + assert serializer['text'].errors.detail[0].code == 'max_length' assert serializer['text'].name == 'text' assert serializer['amount'].value is 123 assert serializer['amount'].errors is None diff --git a/tests/test_fields.py b/tests/test_fields.py index 9cb59f7da0..0c1bbfc85c 100644 --- a/tests/test_fields.py +++ b/tests/test_fields.py @@ -10,6 +10,7 @@ import rest_framework from rest_framework import serializers +from rest_framework.exceptions import ValidationError # Tests for field keyword arguments and core functionality. @@ -426,7 +427,13 @@ def test_invalid_inputs(self): for input_value, expected_failure in get_items(self.invalid_inputs): with pytest.raises(serializers.ValidationError) as exc_info: self.field.run_validation(input_value) - assert exc_info.value.detail == expected_failure + + if isinstance(exc_info.value.detail[0], ValidationError): + failure = exc_info.value.detail[0].detail + else: + failure = exc_info.value.detail + + assert failure == expected_failure def test_outputs(self): for output_value, expected_output in get_items(self.outputs): @@ -1393,7 +1400,10 @@ class TestFieldFieldWithName(FieldValues): # call into it's regular validation, or require PIL for testing. class FailImageValidation(object): def to_python(self, value): - raise serializers.ValidationError(self.error_messages['invalid_image']) + raise serializers.ValidationError( + self.error_messages['invalid_image'], + code='invalid_image' + ) class PassImageValidation(object): diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index 57e540e7a5..fb786b01fa 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -374,7 +374,7 @@ class Meta: s = TestSerializer(data={'address': 'not an ip address'}) self.assertFalse(s.is_valid()) - self.assertEquals(1, len(s.errors['address']), + self.assertEquals(1, len(s.errors['address'].detail), 'Unexpected number of validation errors: ' '{0}'.format(s.errors)) diff --git a/tests/test_relations_hyperlink.py b/tests/test_relations_hyperlink.py index c0642eda26..c2fc7904f1 100644 --- a/tests/test_relations_hyperlink.py +++ b/tests/test_relations_hyperlink.py @@ -244,7 +244,8 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data, context={'request': request}) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'target': ['Incorrect type. Expected URL string, received int.']}) + self.assertEqual(serializer.errors['target'].detail, ['Incorrect type. Expected URL string, received int.']) + self.assertEqual(serializer.errors['target'].code, 'incorrect_type') def test_reverse_foreign_key_update(self): data = {'url': 'http://testserver/foreignkeytarget/2/', 'name': 'target-2', 'sources': ['http://testserver/foreignkeysource/1/', 'http://testserver/foreignkeysource/3/']} @@ -315,7 +316,8 @@ def test_foreign_key_update_with_invalid_null(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data, context={'request': request}) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) + self.assertEqual(serializer.errors['target'].detail, ['This field may not be null.']) + self.assertEqual(serializer.errors['target'].code, 'null') class HyperlinkedNullableForeignKeyTests(TestCase): diff --git a/tests/test_relations_pk.py b/tests/test_relations_pk.py index 169f7d9c5e..0ad31109d8 100644 --- a/tests/test_relations_pk.py +++ b/tests/test_relations_pk.py @@ -235,7 +235,9 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'target': ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]}) + self.assertEqual(serializer.errors['target'].detail, + ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]) + self.assertEqual(serializer.errors['target'].code, 'incorrect_type') def test_reverse_foreign_key_update(self): data = {'id': 2, 'name': 'target-2', 'sources': [1, 3]} @@ -306,7 +308,8 @@ def test_foreign_key_update_with_invalid_null(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) + self.assertEqual(serializer.errors['target'].detail, ['This field may not be null.']) + self.assertEqual(serializer.errors['target'].code, 'null') def test_foreign_key_with_unsaved(self): source = ForeignKeySource(name='source-unsaved') diff --git a/tests/test_relations_slug.py b/tests/test_relations_slug.py index 680aee4173..fb7910b694 100644 --- a/tests/test_relations_slug.py +++ b/tests/test_relations_slug.py @@ -104,7 +104,8 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'target': ['Object with name=123 does not exist.']}) + self.assertEqual(serializer.errors['target'].detail, ['Object with name=123 does not exist.']) + self.assertEqual(serializer.errors['target'].code, 'does_not_exist') def test_reverse_foreign_key_update(self): data = {'id': 2, 'name': 'target-2', 'sources': ['source-1', 'source-3']} @@ -176,7 +177,8 @@ def test_foreign_key_update_with_invalid_null(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) + self.assertEqual(serializer.errors['target'].detail, ['This field may not be null.']) + self.assertEqual(serializer.errors['target'].code, 'null') class SlugNullableForeignKeyTests(TestCase): diff --git a/tests/test_serializer.py b/tests/test_serializer.py index 741c6ab174..b2436ce4a4 100644 --- a/tests/test_serializer.py +++ b/tests/test_serializer.py @@ -7,6 +7,7 @@ from rest_framework import serializers from rest_framework.compat import unicode_repr +from rest_framework.fields import DjangoValidationError from .utils import MockObject @@ -31,7 +32,8 @@ def test_invalid_serializer(self): serializer = self.Serializer(data={'char': 'abc'}) assert not serializer.is_valid() assert serializer.validated_data == {} - assert serializer.errors == {'integer': ['This field is required.']} + assert serializer.errors['integer'].detail == ['This field is required.'] + assert serializer.errors['integer'].code == 'required' def test_partial_validation(self): serializer = self.Serializer(data={'char': 'abc'}, partial=True) @@ -69,7 +71,10 @@ class ExampleSerializer(serializers.Serializer): integer = serializers.IntegerField() def validate(self, attrs): - raise serializers.ValidationError('Non field error') + raise serializers.ValidationError( + 'Non field error', + code='test' + ) serializer = ExampleSerializer(data={'char': 'abc', 'integer': 123}) assert not serializer.is_valid() @@ -309,3 +314,27 @@ class ExampleSerializer(serializers.Serializer): pickled = pickle.dumps(serializer.data) data = pickle.loads(pickled) assert data == {'field1': 'a', 'field2': 'b'} + + +class TestGetValidationErrorDetail: + def test_get_validation_error_detail_converts_django_errors(self): + exc = DjangoValidationError("Missing field.", code='required') + detail = serializers.get_validation_error_detail(exc) + assert detail['non_field_errors'][0].detail == ['Missing field.'] + assert detail['non_field_errors'][0].code == 'required' + + +class TestCapturingDjangoValidationError: + def test_django_validation_error_on_a_field_is_converted(self): + class ExampleSerializer(serializers.Serializer): + field = serializers.CharField() + + def validate_field(self, value): + raise DjangoValidationError( + 'validation failed' + ) + + serializer = ExampleSerializer(data={'field': 'a'}) + assert not serializer.is_valid() + assert serializer.errors['field'][0].detail == ['validation failed'] + assert serializer.errors['field'][0].code == 'invalid' diff --git a/tests/test_serializer_bulk_update.py b/tests/test_serializer_bulk_update.py index 8d7240a7b0..b18d6200d8 100644 --- a/tests/test_serializer_bulk_update.py +++ b/tests/test_serializer_bulk_update.py @@ -67,15 +67,16 @@ def test_bulk_create_errors(self): 'author': 'Haruki Murakami' } ] - expected_errors = [ - {}, - {}, - {'id': ['A valid integer is required.']} - ] serializer = self.BookSerializer(data=data, many=True) self.assertEqual(serializer.is_valid(), False) - self.assertEqual(serializer.errors, expected_errors) + + for idx, error in enumerate(serializer.errors): + if idx < 2: + self.assertEqual(error, {}) + else: + self.assertEqual(error['id'].detail, ['A valid integer is required.']) + self.assertEqual(error['id'].code, 'invalid') def test_invalid_list_datatype(self): """ @@ -87,13 +88,10 @@ def test_invalid_list_datatype(self): text_type_string = six.text_type.__name__ message = 'Invalid data. Expected a dictionary, but got %s.' % text_type_string - expected_errors = [ - {'non_field_errors': [message]}, - {'non_field_errors': [message]}, - {'non_field_errors': [message]} - ] - self.assertEqual(serializer.errors, expected_errors) + for error in serializer.errors: + self.assertEqual(error['non_field_errors'][0].detail, [message]) + self.assertEqual(error['non_field_errors'][0].code, 'invalid') def test_invalid_single_datatype(self): """ @@ -103,9 +101,9 @@ def test_invalid_single_datatype(self): serializer = self.BookSerializer(data=data, many=True) self.assertEqual(serializer.is_valid(), False) - expected_errors = {'non_field_errors': ['Expected a list of items but got type "int".']} - - self.assertEqual(serializer.errors, expected_errors) + self.assertEqual(serializer.errors['non_field_errors'][0].detail, + ['Expected a list of items but got type "int".']) + self.assertEqual(serializer.errors['non_field_errors'][0].code, 'not_a_list') def test_invalid_single_object(self): """ @@ -120,6 +118,7 @@ def test_invalid_single_object(self): serializer = self.BookSerializer(data=data, many=True) self.assertEqual(serializer.is_valid(), False) - expected_errors = {'non_field_errors': ['Expected a list of items but got type "dict".']} + self.assertEqual(serializer.errors['non_field_errors'][0].detail, + ['Expected a list of items but got type "dict".']) - self.assertEqual(serializer.errors, expected_errors) + self.assertEqual(serializer.errors['non_field_errors'][0].code, 'not_a_list') diff --git a/tests/test_serializer_lists.py b/tests/test_serializer_lists.py index 607ddba04a..81aed35747 100644 --- a/tests/test_serializer_lists.py +++ b/tests/test_serializer_lists.py @@ -280,7 +280,10 @@ class TestListSerializerClass: def test_list_serializer_class_validate(self): class CustomListSerializer(serializers.ListSerializer): def validate(self, attrs): - raise serializers.ValidationError('Non field error') + raise serializers.ValidationError( + 'Non field error', + code='test' + ) class TestSerializer(serializers.Serializer): class Meta: diff --git a/tests/test_serializer_nested.py b/tests/test_serializer_nested.py index aeb092ee05..41d6724ac3 100644 --- a/tests/test_serializer_nested.py +++ b/tests/test_serializer_nested.py @@ -113,8 +113,8 @@ def test_null_is_not_allowed_if_allow_null_is_not_set(self): assert not serializer.is_valid() - expected_errors = {'not_allow_null': [serializer.error_messages['null']]} - assert serializer.errors == expected_errors + assert serializer.errors['not_allow_null'].detail == [serializer.error_messages['null']] + assert serializer.errors['not_allow_null'].code == 'null' def test_run_the_field_validation_even_if_the_field_is_null(self): class TestSerializer(self.Serializer): @@ -165,5 +165,7 @@ def test_empty_not_allowed_if_allow_empty_is_set_to_false(self): assert not serializer.is_valid() - expected_errors = {'not_allow_empty': {'non_field_errors': [serializers.ListSerializer.default_error_messages['empty']]}} - assert serializer.errors == expected_errors + assert serializer.errors['not_allow_empty'].detail['non_field_errors'][0].detail == \ + [serializers.ListSerializer.default_error_messages['empty']] + + assert serializer.errors['not_allow_empty'].detail['non_field_errors'][0].code == 'empty_not_allowed' diff --git a/tests/test_validation.py b/tests/test_validation.py index 855ff20e01..a644a3d5ff 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -41,7 +41,8 @@ class ShouldValidateModelSerializer(serializers.ModelSerializer): def validate_renamed(self, value): if len(value) < 3: - raise serializers.ValidationError('Minimum 3 characters.') + raise serializers.ValidationError('Minimum 3 characters.', + code='min_length') return value class Meta: @@ -91,11 +92,9 @@ class TestAvoidValidation(TestCase): def test_serializer_errors_has_only_invalid_data_error(self): serializer = ValidationSerializer(data='invalid data') self.assertFalse(serializer.is_valid()) - self.assertDictEqual(serializer.errors, { - 'non_field_errors': [ - 'Invalid data. Expected a dictionary, but got %s.' % type('').__name__ - ] - }) + self.assertEqual(serializer.errors['non_field_errors'][0].detail, + ['Invalid data. Expected a dictionary, but got %s.' % type('').__name__]) + self.assertEqual(serializer.errors['non_field_errors'][0].code, 'invalid') # regression tests for issue: 1493 @@ -123,7 +122,9 @@ def test_max_value_validation_serializer_success(self): def test_max_value_validation_serializer_fails(self): serializer = ValidationMaxValueValidatorModelSerializer(data={'number_value': 101}) self.assertFalse(serializer.is_valid()) - self.assertDictEqual({'number_value': ['Ensure this value is less than or equal to 100.']}, serializer.errors) + self.assertEqual(['Ensure this value is less than or equal to 100.'], serializer.errors['number_value'].detail[0].detail) + self.assertEqual(None, serializer.errors['number_value'].code) + self.assertEqual('max_value', serializer.errors['number_value'].detail[0].code) def test_max_value_validation_success(self): obj = ValidationMaxValueValidatorModel.objects.create(number_value=100) diff --git a/tests/test_validation_error.py b/tests/test_validation_error.py new file mode 100644 index 0000000000..7e65131275 --- /dev/null +++ b/tests/test_validation_error.py @@ -0,0 +1,95 @@ +import pytest +from django.test import TestCase + +from rest_framework import serializers, status +from rest_framework.decorators import api_view +from rest_framework.exceptions import ValidationError +from rest_framework.response import Response +from rest_framework.settings import api_settings +from rest_framework.test import APIRequestFactory +from rest_framework.views import APIView + +factory = APIRequestFactory() + + +class ExampleSerializer(serializers.Serializer): + char = serializers.CharField() + integer = serializers.IntegerField() + + +class ErrorView(APIView): + def get(self, request, *args, **kwargs): + ExampleSerializer(data={}).is_valid(raise_exception=True) + + +@api_view(['GET']) +def error_view(request): + ExampleSerializer(data={}).is_valid(raise_exception=True) + + +class TestValidationErrorWithCode(TestCase): + def setUp(self): + self.DEFAULT_HANDLER = api_settings.EXCEPTION_HANDLER + + def exception_handler(exc, request): + if not exc.code: + errors = { + field_name: { + 'code': e.code, + 'message': e.detail + } for field_name, e in exc.detail.items() + } + else: + errors = { + 'code': exc.code, + 'detail': exc.detail + } + return Response(errors, status=status.HTTP_400_BAD_REQUEST) + + api_settings.EXCEPTION_HANDLER = exception_handler + + self.expected_response_data = { + 'char': { + 'message': ['This field is required.'], + 'code': 'required', + }, + 'integer': { + 'message': ['This field is required.'], + 'code': 'required' + }, + } + + def tearDown(self): + api_settings.EXCEPTION_HANDLER = self.DEFAULT_HANDLER + + def test_validation_error_requires_no_code_for_structured_errors(self): + """ + ValidationError can hold a list or dictionary of simple errors, in + which case the code is no longer meaningful and should not be + specified. + """ + with pytest.raises(AssertionError): + serializers.ValidationError([ValidationError("test-detail", "test-code")], code='min_value') + + with pytest.raises(AssertionError): + serializers.ValidationError({}, code='min_value') + + def test_validation_error_stores_error_code(self): + exception = serializers.ValidationError("", code='min_value') + assert exception.code == 'min_value' + + def test_class_based_view_exception_handler(self): + view = ErrorView.as_view() + + request = factory.get('/', content_type='application/json') + response = view(request) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data, self.expected_response_data) + + def test_function_based_view_exception_handler(self): + view = error_view + + request = factory.get('/', content_type='application/json') + response = view(request) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data, self.expected_response_data) diff --git a/tests/test_validators.py b/tests/test_validators.py index acaaf57432..b86f1ed165 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -47,8 +47,11 @@ def test_repr(self): def test_is_not_unique(self): data = {'username': 'existing'} serializer = UniquenessSerializer(data=data) + assert not serializer.is_valid() - assert serializer.errors == {'username': ['UniquenessModel with this username already exists.']} + assert serializer.errors['username'].code is None + assert serializer.errors['username'].detail[0].code == 'unique' + assert serializer.errors['username'].detail[0].detail == ['UniquenessModel with this username already exists.'] def test_is_unique(self): data = {'username': 'other'} @@ -150,11 +153,9 @@ def test_is_not_unique_together(self): data = {'race_name': 'example', 'position': 2} serializer = UniquenessTogetherSerializer(data=data) assert not serializer.is_valid() - assert serializer.errors == { - 'non_field_errors': [ - 'The fields race_name, position must make a unique set.' - ] - } + assert serializer.errors['non_field_errors'][0].code == 'unique' + assert serializer.errors['non_field_errors'][0].detail == [ + 'The fields race_name, position must make a unique set.'] def test_is_unique_together(self): """ @@ -189,9 +190,8 @@ def test_unique_together_is_required(self): data = {'position': 2} serializer = UniquenessTogetherSerializer(data=data, partial=True) assert not serializer.is_valid() - assert serializer.errors == { - 'race_name': ['This field is required.'] - } + assert serializer.errors['race_name'][0].code == 'required' + assert serializer.errors['race_name'][0].detail == ['This field is required.'] def test_ignore_excluded_fields(self): """ @@ -278,9 +278,8 @@ def test_is_not_unique_for_date(self): data = {'slug': 'existing', 'published': '2000-01-01'} serializer = UniqueForDateSerializer(data=data) assert not serializer.is_valid() - assert serializer.errors == { - 'slug': ['This field must be unique for the "published" date.'] - } + assert serializer.errors['slug'][0].code == 'unique' + assert serializer.errors['slug'][0].detail == ['This field must be unique for the "published" date.'] def test_is_unique_for_date(self): """ From 18347601489f4f868adc82f5cccc9929186513d6 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Thu, 10 Dec 2015 14:46:51 +0100 Subject: [PATCH 2/9] Dont change return type of `Serializer.errors` We should keep `Serializer.errors`'s return type unchanged in order to maintain backward compatibility. The error codes will only be propagated to the `exception_handler` or accessible through the `Serializer._errors` private attribute. --- rest_framework/renderers.py | 15 ----------- rest_framework/serializers.py | 40 ++++++++++++++++++++++++++-- rest_framework/views.py | 15 ++--------- tests/test_bound_fields.py | 7 +++-- tests/test_model_serializer.py | 2 +- tests/test_relations_hyperlink.py | 10 ++++--- tests/test_relations_pk.py | 11 +++++--- tests/test_relations_slug.py | 10 ++++--- tests/test_serializer.py | 5 ++-- tests/test_serializer_bulk_update.py | 10 ++++++- tests/test_serializer_nested.py | 15 ++++++++--- tests/test_validation.py | 10 ++++--- tests/test_validators.py | 9 ++++--- 13 files changed, 101 insertions(+), 58 deletions(-) diff --git a/rest_framework/renderers.py b/rest_framework/renderers.py index d52ebaea98..dafaf7de62 100644 --- a/rest_framework/renderers.py +++ b/rest_framework/renderers.py @@ -493,21 +493,6 @@ def get_rendered_html_form(self, data, view, method, request): if hasattr(serializer, 'initial_data'): serializer.is_valid() - # Convert ValidationError to unicode string - # This is mainly a hack to monkey patch the errors and make the form renderer happy... - errors = OrderedDict() - for field_name, values in serializer.errors.items(): - if isinstance(values, list): - errors[field_name] = values - continue - - errors[field_name] = [] - for value in values.detail: - for message in value.detail: - errors[field_name].append(message) - - serializer._errors = errors - form_renderer = self.form_renderer_class() return form_renderer.render( serializer.data, diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index e3206793e2..d67f047c25 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -219,7 +219,13 @@ def is_valid(self, raise_exception=False): self._errors = {} if self._errors and raise_exception: - raise ValidationError(self.errors) + return_errors = None + if isinstance(self._errors, list): + return_errors = ReturnList(self._errors, serializer=self) + elif isinstance(self._errors, dict): + return_errors = ReturnDict(self._errors, serializer=self) + + raise ValidationError(return_errors) return not bool(self._errors) @@ -244,12 +250,42 @@ def data(self): self._data = self.get_initial() return self._data + def _transform_to_legacy_errors(self, errors_to_transform): + # Do not mutate `errors_to_transform` here. + errors = ReturnDict(serializer=self) + for field_name, values in errors_to_transform.items(): + if isinstance(values, list): + errors[field_name] = values + continue + + if isinstance(values.detail, list): + errors[field_name] = [] + for value in values.detail: + if isinstance(value, ValidationError): + errors[field_name].extend(value.detail) + elif isinstance(value, list): + errors[field_name].extend(value) + else: + errors[field_name].append(value) + + elif isinstance(values.detail, dict): + errors[field_name] = {} + for sub_field_name, value in values.detail.items(): + errors[field_name][sub_field_name] = [] + for validation_error in value: + errors[field_name][sub_field_name].extend(validation_error.detail) + return errors + @property def errors(self): if not hasattr(self, '_errors'): msg = 'You must call `.is_valid()` before accessing `.errors`.' raise AssertionError(msg) - return self._errors + + if isinstance(self._errors, list): + return map(self._transform_to_legacy_errors, self._errors) + else: + return self._transform_to_legacy_errors(self._errors) @property def validated_data(self): diff --git a/rest_framework/views.py b/rest_framework/views.py index 926a3d00e9..915bacbc6d 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -15,7 +15,6 @@ from rest_framework import exceptions, status from rest_framework.compat import set_rollback -from rest_framework.exceptions import ValidationError, _force_text_recursive from rest_framework.request import Request from rest_framework.response import Response from rest_framework.settings import api_settings @@ -70,18 +69,8 @@ def exception_handler(exc, context): if getattr(exc, 'wait', None): headers['Retry-After'] = '%d' % exc.wait - if isinstance(exc.detail, list): - data = _force_text_recursive([item.detail if isinstance(item, ValidationError) else item - for item in exc.detai]) - elif isinstance(exc.detail, dict): - for field_name, e in exc.detail.items(): - if hasattr(e, 'detail') and isinstance(e.detail[0], ValidationError): - exc.detail[field_name] = e.detail[0].detail - elif isinstance(e, ValidationError): - exc.detail[field_name] = e.detail - else: - exc.detail[field_name] = e - data = exc.detail + if isinstance(exc.detail, (list, dict)): + data = exc.detail.serializer.errors else: data = {'detail': exc.detail} diff --git a/tests/test_bound_fields.py b/tests/test_bound_fields.py index 0fd8e6f5d3..e1f6cd88b8 100644 --- a/tests/test_bound_fields.py +++ b/tests/test_bound_fields.py @@ -38,9 +38,12 @@ class ExampleSerializer(serializers.Serializer): serializer = ExampleSerializer(data={'text': 'x' * 1000, 'amount': 123}) serializer.is_valid() + # TODO Should we add the _errors with ValidationError to the bound_field.errors to get acces to the error code? + assert serializer._errors['text'].detail[0].detail == ['Ensure this field has no more than 100 characters.'] + assert serializer._errors['text'].detail[0].code == 'max_length' + assert serializer['text'].value == 'x' * 1000 - assert serializer['text'].errors.detail[0].detail == ['Ensure this field has no more than 100 characters.'] - assert serializer['text'].errors.detail[0].code == 'max_length' + assert serializer['text'].errors == ['Ensure this field has no more than 100 characters.'] assert serializer['text'].name == 'text' assert serializer['amount'].value is 123 assert serializer['amount'].errors is None diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index fb786b01fa..57e540e7a5 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -374,7 +374,7 @@ class Meta: s = TestSerializer(data={'address': 'not an ip address'}) self.assertFalse(s.is_valid()) - self.assertEquals(1, len(s.errors['address'].detail), + self.assertEquals(1, len(s.errors['address']), 'Unexpected number of validation errors: ' '{0}'.format(s.errors)) diff --git a/tests/test_relations_hyperlink.py b/tests/test_relations_hyperlink.py index c2fc7904f1..2bf311d8ec 100644 --- a/tests/test_relations_hyperlink.py +++ b/tests/test_relations_hyperlink.py @@ -244,8 +244,9 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data, context={'request': request}) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['target'].detail, ['Incorrect type. Expected URL string, received int.']) - self.assertEqual(serializer.errors['target'].code, 'incorrect_type') + self.assertEqual(serializer.errors, {'target': ['Incorrect type. Expected URL string, received int.']}) + self.assertEqual(serializer._errors['target'].detail, ['Incorrect type. Expected URL string, received int.']) + self.assertEqual(serializer._errors['target'].code, 'incorrect_type') def test_reverse_foreign_key_update(self): data = {'url': 'http://testserver/foreignkeytarget/2/', 'name': 'target-2', 'sources': ['http://testserver/foreignkeysource/1/', 'http://testserver/foreignkeysource/3/']} @@ -316,8 +317,9 @@ def test_foreign_key_update_with_invalid_null(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data, context={'request': request}) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['target'].detail, ['This field may not be null.']) - self.assertEqual(serializer.errors['target'].code, 'null') + self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) + self.assertEqual(serializer._errors['target'].detail, ['This field may not be null.']) + self.assertEqual(serializer._errors['target'].code, 'null') class HyperlinkedNullableForeignKeyTests(TestCase): diff --git a/tests/test_relations_pk.py b/tests/test_relations_pk.py index 0ad31109d8..49110d2de8 100644 --- a/tests/test_relations_pk.py +++ b/tests/test_relations_pk.py @@ -235,9 +235,11 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['target'].detail, + self.assertEqual(serializer.errors, + {'target': ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]}) + self.assertEqual(serializer._errors['target'].detail, ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]) - self.assertEqual(serializer.errors['target'].code, 'incorrect_type') + self.assertEqual(serializer._errors['target'].code, 'incorrect_type') def test_reverse_foreign_key_update(self): data = {'id': 2, 'name': 'target-2', 'sources': [1, 3]} @@ -308,8 +310,9 @@ def test_foreign_key_update_with_invalid_null(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['target'].detail, ['This field may not be null.']) - self.assertEqual(serializer.errors['target'].code, 'null') + self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) + self.assertEqual(serializer._errors['target'].detail, ['This field may not be null.']) + self.assertEqual(serializer._errors['target'].code, 'null') def test_foreign_key_with_unsaved(self): source = ForeignKeySource(name='source-unsaved') diff --git a/tests/test_relations_slug.py b/tests/test_relations_slug.py index fb7910b694..936546b8dd 100644 --- a/tests/test_relations_slug.py +++ b/tests/test_relations_slug.py @@ -104,8 +104,9 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['target'].detail, ['Object with name=123 does not exist.']) - self.assertEqual(serializer.errors['target'].code, 'does_not_exist') + self.assertEqual(serializer.errors, {'target': ['Object with name=123 does not exist.']}) + self.assertEqual(serializer._errors['target'].detail, ['Object with name=123 does not exist.']) + self.assertEqual(serializer._errors['target'].code, 'does_not_exist') def test_reverse_foreign_key_update(self): data = {'id': 2, 'name': 'target-2', 'sources': ['source-1', 'source-3']} @@ -177,8 +178,9 @@ def test_foreign_key_update_with_invalid_null(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['target'].detail, ['This field may not be null.']) - self.assertEqual(serializer.errors['target'].code, 'null') + self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) + self.assertEqual(serializer._errors['target'].detail, ['This field may not be null.']) + self.assertEqual(serializer._errors['target'].code, 'null') class SlugNullableForeignKeyTests(TestCase): diff --git a/tests/test_serializer.py b/tests/test_serializer.py index b2436ce4a4..97a2c642eb 100644 --- a/tests/test_serializer.py +++ b/tests/test_serializer.py @@ -32,8 +32,9 @@ def test_invalid_serializer(self): serializer = self.Serializer(data={'char': 'abc'}) assert not serializer.is_valid() assert serializer.validated_data == {} - assert serializer.errors['integer'].detail == ['This field is required.'] - assert serializer.errors['integer'].code == 'required' + assert serializer.errors == {'integer': ['This field is required.']} + assert serializer._errors['integer'].detail == ['This field is required.'] + assert serializer._errors['integer'].code == 'required' def test_partial_validation(self): serializer = self.Serializer(data={'char': 'abc'}, partial=True) diff --git a/tests/test_serializer_bulk_update.py b/tests/test_serializer_bulk_update.py index b18d6200d8..2b67ff5c78 100644 --- a/tests/test_serializer_bulk_update.py +++ b/tests/test_serializer_bulk_update.py @@ -71,7 +71,15 @@ def test_bulk_create_errors(self): serializer = self.BookSerializer(data=data, many=True) self.assertEqual(serializer.is_valid(), False) - for idx, error in enumerate(serializer.errors): + expected_errors = [ + {}, + {}, + {'id': ['A valid integer is required.']} + ] + + self.assertEqual(serializer.errors, expected_errors) + + for idx, error in enumerate(serializer._errors): if idx < 2: self.assertEqual(error, {}) else: diff --git a/tests/test_serializer_nested.py b/tests/test_serializer_nested.py index 41d6724ac3..870d66c548 100644 --- a/tests/test_serializer_nested.py +++ b/tests/test_serializer_nested.py @@ -113,8 +113,11 @@ def test_null_is_not_allowed_if_allow_null_is_not_set(self): assert not serializer.is_valid() - assert serializer.errors['not_allow_null'].detail == [serializer.error_messages['null']] - assert serializer.errors['not_allow_null'].code == 'null' + expected_errors = {'not_allow_null': [serializer.error_messages['null']]} + assert serializer.errors == expected_errors + + assert serializer._errors['not_allow_null'].detail == [serializer.error_messages['null']] + assert serializer._errors['not_allow_null'].code == 'null' def test_run_the_field_validation_even_if_the_field_is_null(self): class TestSerializer(self.Serializer): @@ -165,7 +168,11 @@ def test_empty_not_allowed_if_allow_empty_is_set_to_false(self): assert not serializer.is_valid() - assert serializer.errors['not_allow_empty'].detail['non_field_errors'][0].detail == \ + expected_errors = { + 'not_allow_empty': {'non_field_errors': [serializers.ListSerializer.default_error_messages['empty']]}} + assert serializer.errors == expected_errors + + assert serializer._errors['not_allow_empty'].detail['non_field_errors'][0].detail == \ [serializers.ListSerializer.default_error_messages['empty']] - assert serializer.errors['not_allow_empty'].detail['non_field_errors'][0].code == 'empty_not_allowed' + assert serializer._errors['not_allow_empty'].detail['non_field_errors'][0].code == 'empty_not_allowed' diff --git a/tests/test_validation.py b/tests/test_validation.py index a644a3d5ff..eb4b6be64a 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -122,9 +122,13 @@ def test_max_value_validation_serializer_success(self): def test_max_value_validation_serializer_fails(self): serializer = ValidationMaxValueValidatorModelSerializer(data={'number_value': 101}) self.assertFalse(serializer.is_valid()) - self.assertEqual(['Ensure this value is less than or equal to 100.'], serializer.errors['number_value'].detail[0].detail) - self.assertEqual(None, serializer.errors['number_value'].code) - self.assertEqual('max_value', serializer.errors['number_value'].detail[0].code) + + self.assertDictEqual({'number_value': ['Ensure this value is less than or equal to 100.']}, serializer.errors) + + self.assertEqual(['Ensure this value is less than or equal to 100.'], + serializer._errors['number_value'].detail[0].detail) + self.assertEqual(None, serializer._errors['number_value'].code) + self.assertEqual('max_value', serializer._errors['number_value'].detail[0].code) def test_max_value_validation_success(self): obj = ValidationMaxValueValidatorModel.objects.create(number_value=100) diff --git a/tests/test_validators.py b/tests/test_validators.py index b86f1ed165..ff28908c3c 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -49,9 +49,12 @@ def test_is_not_unique(self): serializer = UniquenessSerializer(data=data) assert not serializer.is_valid() - assert serializer.errors['username'].code is None - assert serializer.errors['username'].detail[0].code == 'unique' - assert serializer.errors['username'].detail[0].detail == ['UniquenessModel with this username already exists.'] + + assert serializer.errors == {'username': ['UniquenessModel with this username already exists.']} + + assert serializer._errors['username'].code is None + assert serializer._errors['username'].detail[0].code == 'unique' + assert serializer._errors['username'].detail[0].detail == ['UniquenessModel with this username already exists.'] def test_is_unique(self): data = {'username': 'other'} From c7351b38321f2cb56ad662b0fef3fb4401751ec3 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Wed, 16 Dec 2015 16:55:19 +0100 Subject: [PATCH 3/9] Revert change on existing tests --- tests/test_bound_fields.py | 4 -- tests/test_fields.py | 14 +--- tests/test_relations_hyperlink.py | 4 -- tests/test_relations_pk.py | 8 +-- tests/test_relations_slug.py | 4 -- tests/test_serializer.py | 32 +--------- tests/test_serializer_bulk_update.py | 33 ++++------ tests/test_serializer_lists.py | 5 +- tests/test_serializer_nested.py | 11 +--- tests/test_validation.py | 17 ++--- tests/test_validation_error.py | 95 ---------------------------- tests/test_validators.py | 24 ++++--- 12 files changed, 36 insertions(+), 215 deletions(-) delete mode 100644 tests/test_validation_error.py diff --git a/tests/test_bound_fields.py b/tests/test_bound_fields.py index e1f6cd88b8..f2fac8f0d2 100644 --- a/tests/test_bound_fields.py +++ b/tests/test_bound_fields.py @@ -38,10 +38,6 @@ class ExampleSerializer(serializers.Serializer): serializer = ExampleSerializer(data={'text': 'x' * 1000, 'amount': 123}) serializer.is_valid() - # TODO Should we add the _errors with ValidationError to the bound_field.errors to get acces to the error code? - assert serializer._errors['text'].detail[0].detail == ['Ensure this field has no more than 100 characters.'] - assert serializer._errors['text'].detail[0].code == 'max_length' - assert serializer['text'].value == 'x' * 1000 assert serializer['text'].errors == ['Ensure this field has no more than 100 characters.'] assert serializer['text'].name == 'text' diff --git a/tests/test_fields.py b/tests/test_fields.py index 0c1bbfc85c..9cb59f7da0 100644 --- a/tests/test_fields.py +++ b/tests/test_fields.py @@ -10,7 +10,6 @@ import rest_framework from rest_framework import serializers -from rest_framework.exceptions import ValidationError # Tests for field keyword arguments and core functionality. @@ -427,13 +426,7 @@ def test_invalid_inputs(self): for input_value, expected_failure in get_items(self.invalid_inputs): with pytest.raises(serializers.ValidationError) as exc_info: self.field.run_validation(input_value) - - if isinstance(exc_info.value.detail[0], ValidationError): - failure = exc_info.value.detail[0].detail - else: - failure = exc_info.value.detail - - assert failure == expected_failure + assert exc_info.value.detail == expected_failure def test_outputs(self): for output_value, expected_output in get_items(self.outputs): @@ -1400,10 +1393,7 @@ class TestFieldFieldWithName(FieldValues): # call into it's regular validation, or require PIL for testing. class FailImageValidation(object): def to_python(self, value): - raise serializers.ValidationError( - self.error_messages['invalid_image'], - code='invalid_image' - ) + raise serializers.ValidationError(self.error_messages['invalid_image']) class PassImageValidation(object): diff --git a/tests/test_relations_hyperlink.py b/tests/test_relations_hyperlink.py index 2bf311d8ec..c0642eda26 100644 --- a/tests/test_relations_hyperlink.py +++ b/tests/test_relations_hyperlink.py @@ -245,8 +245,6 @@ def test_foreign_key_update_incorrect_type(self): serializer = ForeignKeySourceSerializer(instance, data=data, context={'request': request}) self.assertFalse(serializer.is_valid()) self.assertEqual(serializer.errors, {'target': ['Incorrect type. Expected URL string, received int.']}) - self.assertEqual(serializer._errors['target'].detail, ['Incorrect type. Expected URL string, received int.']) - self.assertEqual(serializer._errors['target'].code, 'incorrect_type') def test_reverse_foreign_key_update(self): data = {'url': 'http://testserver/foreignkeytarget/2/', 'name': 'target-2', 'sources': ['http://testserver/foreignkeysource/1/', 'http://testserver/foreignkeysource/3/']} @@ -318,8 +316,6 @@ def test_foreign_key_update_with_invalid_null(self): serializer = ForeignKeySourceSerializer(instance, data=data, context={'request': request}) self.assertFalse(serializer.is_valid()) self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) - self.assertEqual(serializer._errors['target'].detail, ['This field may not be null.']) - self.assertEqual(serializer._errors['target'].code, 'null') class HyperlinkedNullableForeignKeyTests(TestCase): diff --git a/tests/test_relations_pk.py b/tests/test_relations_pk.py index 49110d2de8..169f7d9c5e 100644 --- a/tests/test_relations_pk.py +++ b/tests/test_relations_pk.py @@ -235,11 +235,7 @@ def test_foreign_key_update_incorrect_type(self): instance = ForeignKeySource.objects.get(pk=1) serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, - {'target': ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]}) - self.assertEqual(serializer._errors['target'].detail, - ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]) - self.assertEqual(serializer._errors['target'].code, 'incorrect_type') + self.assertEqual(serializer.errors, {'target': ['Incorrect type. Expected pk value, received %s.' % six.text_type.__name__]}) def test_reverse_foreign_key_update(self): data = {'id': 2, 'name': 'target-2', 'sources': [1, 3]} @@ -311,8 +307,6 @@ def test_foreign_key_update_with_invalid_null(self): serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) - self.assertEqual(serializer._errors['target'].detail, ['This field may not be null.']) - self.assertEqual(serializer._errors['target'].code, 'null') def test_foreign_key_with_unsaved(self): source = ForeignKeySource(name='source-unsaved') diff --git a/tests/test_relations_slug.py b/tests/test_relations_slug.py index 936546b8dd..680aee4173 100644 --- a/tests/test_relations_slug.py +++ b/tests/test_relations_slug.py @@ -105,8 +105,6 @@ def test_foreign_key_update_incorrect_type(self): serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) self.assertEqual(serializer.errors, {'target': ['Object with name=123 does not exist.']}) - self.assertEqual(serializer._errors['target'].detail, ['Object with name=123 does not exist.']) - self.assertEqual(serializer._errors['target'].code, 'does_not_exist') def test_reverse_foreign_key_update(self): data = {'id': 2, 'name': 'target-2', 'sources': ['source-1', 'source-3']} @@ -179,8 +177,6 @@ def test_foreign_key_update_with_invalid_null(self): serializer = ForeignKeySourceSerializer(instance, data=data) self.assertFalse(serializer.is_valid()) self.assertEqual(serializer.errors, {'target': ['This field may not be null.']}) - self.assertEqual(serializer._errors['target'].detail, ['This field may not be null.']) - self.assertEqual(serializer._errors['target'].code, 'null') class SlugNullableForeignKeyTests(TestCase): diff --git a/tests/test_serializer.py b/tests/test_serializer.py index 97a2c642eb..741c6ab174 100644 --- a/tests/test_serializer.py +++ b/tests/test_serializer.py @@ -7,7 +7,6 @@ from rest_framework import serializers from rest_framework.compat import unicode_repr -from rest_framework.fields import DjangoValidationError from .utils import MockObject @@ -33,8 +32,6 @@ def test_invalid_serializer(self): assert not serializer.is_valid() assert serializer.validated_data == {} assert serializer.errors == {'integer': ['This field is required.']} - assert serializer._errors['integer'].detail == ['This field is required.'] - assert serializer._errors['integer'].code == 'required' def test_partial_validation(self): serializer = self.Serializer(data={'char': 'abc'}, partial=True) @@ -72,10 +69,7 @@ class ExampleSerializer(serializers.Serializer): integer = serializers.IntegerField() def validate(self, attrs): - raise serializers.ValidationError( - 'Non field error', - code='test' - ) + raise serializers.ValidationError('Non field error') serializer = ExampleSerializer(data={'char': 'abc', 'integer': 123}) assert not serializer.is_valid() @@ -315,27 +309,3 @@ class ExampleSerializer(serializers.Serializer): pickled = pickle.dumps(serializer.data) data = pickle.loads(pickled) assert data == {'field1': 'a', 'field2': 'b'} - - -class TestGetValidationErrorDetail: - def test_get_validation_error_detail_converts_django_errors(self): - exc = DjangoValidationError("Missing field.", code='required') - detail = serializers.get_validation_error_detail(exc) - assert detail['non_field_errors'][0].detail == ['Missing field.'] - assert detail['non_field_errors'][0].code == 'required' - - -class TestCapturingDjangoValidationError: - def test_django_validation_error_on_a_field_is_converted(self): - class ExampleSerializer(serializers.Serializer): - field = serializers.CharField() - - def validate_field(self, value): - raise DjangoValidationError( - 'validation failed' - ) - - serializer = ExampleSerializer(data={'field': 'a'}) - assert not serializer.is_valid() - assert serializer.errors['field'][0].detail == ['validation failed'] - assert serializer.errors['field'][0].code == 'invalid' diff --git a/tests/test_serializer_bulk_update.py b/tests/test_serializer_bulk_update.py index 2b67ff5c78..8d7240a7b0 100644 --- a/tests/test_serializer_bulk_update.py +++ b/tests/test_serializer_bulk_update.py @@ -67,25 +67,16 @@ def test_bulk_create_errors(self): 'author': 'Haruki Murakami' } ] - - serializer = self.BookSerializer(data=data, many=True) - self.assertEqual(serializer.is_valid(), False) - expected_errors = [ {}, {}, {'id': ['A valid integer is required.']} ] + serializer = self.BookSerializer(data=data, many=True) + self.assertEqual(serializer.is_valid(), False) self.assertEqual(serializer.errors, expected_errors) - for idx, error in enumerate(serializer._errors): - if idx < 2: - self.assertEqual(error, {}) - else: - self.assertEqual(error['id'].detail, ['A valid integer is required.']) - self.assertEqual(error['id'].code, 'invalid') - def test_invalid_list_datatype(self): """ Data containing list of incorrect data type should return errors. @@ -96,10 +87,13 @@ def test_invalid_list_datatype(self): text_type_string = six.text_type.__name__ message = 'Invalid data. Expected a dictionary, but got %s.' % text_type_string + expected_errors = [ + {'non_field_errors': [message]}, + {'non_field_errors': [message]}, + {'non_field_errors': [message]} + ] - for error in serializer.errors: - self.assertEqual(error['non_field_errors'][0].detail, [message]) - self.assertEqual(error['non_field_errors'][0].code, 'invalid') + self.assertEqual(serializer.errors, expected_errors) def test_invalid_single_datatype(self): """ @@ -109,9 +103,9 @@ def test_invalid_single_datatype(self): serializer = self.BookSerializer(data=data, many=True) self.assertEqual(serializer.is_valid(), False) - self.assertEqual(serializer.errors['non_field_errors'][0].detail, - ['Expected a list of items but got type "int".']) - self.assertEqual(serializer.errors['non_field_errors'][0].code, 'not_a_list') + expected_errors = {'non_field_errors': ['Expected a list of items but got type "int".']} + + self.assertEqual(serializer.errors, expected_errors) def test_invalid_single_object(self): """ @@ -126,7 +120,6 @@ def test_invalid_single_object(self): serializer = self.BookSerializer(data=data, many=True) self.assertEqual(serializer.is_valid(), False) - self.assertEqual(serializer.errors['non_field_errors'][0].detail, - ['Expected a list of items but got type "dict".']) + expected_errors = {'non_field_errors': ['Expected a list of items but got type "dict".']} - self.assertEqual(serializer.errors['non_field_errors'][0].code, 'not_a_list') + self.assertEqual(serializer.errors, expected_errors) diff --git a/tests/test_serializer_lists.py b/tests/test_serializer_lists.py index 81aed35747..607ddba04a 100644 --- a/tests/test_serializer_lists.py +++ b/tests/test_serializer_lists.py @@ -280,10 +280,7 @@ class TestListSerializerClass: def test_list_serializer_class_validate(self): class CustomListSerializer(serializers.ListSerializer): def validate(self, attrs): - raise serializers.ValidationError( - 'Non field error', - code='test' - ) + raise serializers.ValidationError('Non field error') class TestSerializer(serializers.Serializer): class Meta: diff --git a/tests/test_serializer_nested.py b/tests/test_serializer_nested.py index 870d66c548..aeb092ee05 100644 --- a/tests/test_serializer_nested.py +++ b/tests/test_serializer_nested.py @@ -116,9 +116,6 @@ def test_null_is_not_allowed_if_allow_null_is_not_set(self): expected_errors = {'not_allow_null': [serializer.error_messages['null']]} assert serializer.errors == expected_errors - assert serializer._errors['not_allow_null'].detail == [serializer.error_messages['null']] - assert serializer._errors['not_allow_null'].code == 'null' - def test_run_the_field_validation_even_if_the_field_is_null(self): class TestSerializer(self.Serializer): validation_was_run = False @@ -168,11 +165,5 @@ def test_empty_not_allowed_if_allow_empty_is_set_to_false(self): assert not serializer.is_valid() - expected_errors = { - 'not_allow_empty': {'non_field_errors': [serializers.ListSerializer.default_error_messages['empty']]}} + expected_errors = {'not_allow_empty': {'non_field_errors': [serializers.ListSerializer.default_error_messages['empty']]}} assert serializer.errors == expected_errors - - assert serializer._errors['not_allow_empty'].detail['non_field_errors'][0].detail == \ - [serializers.ListSerializer.default_error_messages['empty']] - - assert serializer._errors['not_allow_empty'].detail['non_field_errors'][0].code == 'empty_not_allowed' diff --git a/tests/test_validation.py b/tests/test_validation.py index eb4b6be64a..855ff20e01 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -41,8 +41,7 @@ class ShouldValidateModelSerializer(serializers.ModelSerializer): def validate_renamed(self, value): if len(value) < 3: - raise serializers.ValidationError('Minimum 3 characters.', - code='min_length') + raise serializers.ValidationError('Minimum 3 characters.') return value class Meta: @@ -92,9 +91,11 @@ class TestAvoidValidation(TestCase): def test_serializer_errors_has_only_invalid_data_error(self): serializer = ValidationSerializer(data='invalid data') self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors['non_field_errors'][0].detail, - ['Invalid data. Expected a dictionary, but got %s.' % type('').__name__]) - self.assertEqual(serializer.errors['non_field_errors'][0].code, 'invalid') + self.assertDictEqual(serializer.errors, { + 'non_field_errors': [ + 'Invalid data. Expected a dictionary, but got %s.' % type('').__name__ + ] + }) # regression tests for issue: 1493 @@ -122,14 +123,8 @@ def test_max_value_validation_serializer_success(self): def test_max_value_validation_serializer_fails(self): serializer = ValidationMaxValueValidatorModelSerializer(data={'number_value': 101}) self.assertFalse(serializer.is_valid()) - self.assertDictEqual({'number_value': ['Ensure this value is less than or equal to 100.']}, serializer.errors) - self.assertEqual(['Ensure this value is less than or equal to 100.'], - serializer._errors['number_value'].detail[0].detail) - self.assertEqual(None, serializer._errors['number_value'].code) - self.assertEqual('max_value', serializer._errors['number_value'].detail[0].code) - def test_max_value_validation_success(self): obj = ValidationMaxValueValidatorModel.objects.create(number_value=100) request = factory.patch('/{0}'.format(obj.pk), {'number_value': 98}, format='json') diff --git a/tests/test_validation_error.py b/tests/test_validation_error.py deleted file mode 100644 index 7e65131275..0000000000 --- a/tests/test_validation_error.py +++ /dev/null @@ -1,95 +0,0 @@ -import pytest -from django.test import TestCase - -from rest_framework import serializers, status -from rest_framework.decorators import api_view -from rest_framework.exceptions import ValidationError -from rest_framework.response import Response -from rest_framework.settings import api_settings -from rest_framework.test import APIRequestFactory -from rest_framework.views import APIView - -factory = APIRequestFactory() - - -class ExampleSerializer(serializers.Serializer): - char = serializers.CharField() - integer = serializers.IntegerField() - - -class ErrorView(APIView): - def get(self, request, *args, **kwargs): - ExampleSerializer(data={}).is_valid(raise_exception=True) - - -@api_view(['GET']) -def error_view(request): - ExampleSerializer(data={}).is_valid(raise_exception=True) - - -class TestValidationErrorWithCode(TestCase): - def setUp(self): - self.DEFAULT_HANDLER = api_settings.EXCEPTION_HANDLER - - def exception_handler(exc, request): - if not exc.code: - errors = { - field_name: { - 'code': e.code, - 'message': e.detail - } for field_name, e in exc.detail.items() - } - else: - errors = { - 'code': exc.code, - 'detail': exc.detail - } - return Response(errors, status=status.HTTP_400_BAD_REQUEST) - - api_settings.EXCEPTION_HANDLER = exception_handler - - self.expected_response_data = { - 'char': { - 'message': ['This field is required.'], - 'code': 'required', - }, - 'integer': { - 'message': ['This field is required.'], - 'code': 'required' - }, - } - - def tearDown(self): - api_settings.EXCEPTION_HANDLER = self.DEFAULT_HANDLER - - def test_validation_error_requires_no_code_for_structured_errors(self): - """ - ValidationError can hold a list or dictionary of simple errors, in - which case the code is no longer meaningful and should not be - specified. - """ - with pytest.raises(AssertionError): - serializers.ValidationError([ValidationError("test-detail", "test-code")], code='min_value') - - with pytest.raises(AssertionError): - serializers.ValidationError({}, code='min_value') - - def test_validation_error_stores_error_code(self): - exception = serializers.ValidationError("", code='min_value') - assert exception.code == 'min_value' - - def test_class_based_view_exception_handler(self): - view = ErrorView.as_view() - - request = factory.get('/', content_type='application/json') - response = view(request) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data, self.expected_response_data) - - def test_function_based_view_exception_handler(self): - view = error_view - - request = factory.get('/', content_type='application/json') - response = view(request) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data, self.expected_response_data) diff --git a/tests/test_validators.py b/tests/test_validators.py index ff28908c3c..acaaf57432 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -47,15 +47,9 @@ def test_repr(self): def test_is_not_unique(self): data = {'username': 'existing'} serializer = UniquenessSerializer(data=data) - assert not serializer.is_valid() - assert serializer.errors == {'username': ['UniquenessModel with this username already exists.']} - assert serializer._errors['username'].code is None - assert serializer._errors['username'].detail[0].code == 'unique' - assert serializer._errors['username'].detail[0].detail == ['UniquenessModel with this username already exists.'] - def test_is_unique(self): data = {'username': 'other'} serializer = UniquenessSerializer(data=data) @@ -156,9 +150,11 @@ def test_is_not_unique_together(self): data = {'race_name': 'example', 'position': 2} serializer = UniquenessTogetherSerializer(data=data) assert not serializer.is_valid() - assert serializer.errors['non_field_errors'][0].code == 'unique' - assert serializer.errors['non_field_errors'][0].detail == [ - 'The fields race_name, position must make a unique set.'] + assert serializer.errors == { + 'non_field_errors': [ + 'The fields race_name, position must make a unique set.' + ] + } def test_is_unique_together(self): """ @@ -193,8 +189,9 @@ def test_unique_together_is_required(self): data = {'position': 2} serializer = UniquenessTogetherSerializer(data=data, partial=True) assert not serializer.is_valid() - assert serializer.errors['race_name'][0].code == 'required' - assert serializer.errors['race_name'][0].detail == ['This field is required.'] + assert serializer.errors == { + 'race_name': ['This field is required.'] + } def test_ignore_excluded_fields(self): """ @@ -281,8 +278,9 @@ def test_is_not_unique_for_date(self): data = {'slug': 'existing', 'published': '2000-01-01'} serializer = UniqueForDateSerializer(data=data) assert not serializer.is_valid() - assert serializer.errors['slug'][0].code == 'unique' - assert serializer.errors['slug'][0].detail == ['This field must be unique for the "published" date.'] + assert serializer.errors == { + 'slug': ['This field must be unique for the "published" date.'] + } def test_is_unique_for_date(self): """ From 42f4c5549d00a5b4ceda744503b177c849546ef7 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Wed, 16 Dec 2015 19:09:03 +0100 Subject: [PATCH 4/9] Introduce ValidationErrorMessage `ValidationErrorMessage` is a string-like object that holds a code attribute. The code attribute has been removed from ValidationError to be able to maintain better backward compatibility. What this means is that `ValidationError` can accept either a regular string or a `ValidationErrorMessage` for its `detail` attribute. --- rest_framework/authtoken/serializers.py | 16 ++++-- rest_framework/exceptions.py | 22 +++++--- rest_framework/fields.py | 7 ++- rest_framework/response.py | 1 + rest_framework/serializers.py | 53 +++--------------- rest_framework/validators.py | 22 +++++--- rest_framework/views.py | 2 +- tests/test_validation_error.py | 74 +++++++++++++++++++++++++ 8 files changed, 127 insertions(+), 70 deletions(-) create mode 100644 tests/test_validation_error.py diff --git a/rest_framework/authtoken/serializers.py b/rest_framework/authtoken/serializers.py index caf5b64b3d..99ea86e860 100644 --- a/rest_framework/authtoken/serializers.py +++ b/rest_framework/authtoken/serializers.py @@ -2,6 +2,7 @@ from django.utils.translation import ugettext_lazy as _ from rest_framework import serializers +from rest_framework.exceptions import ValidationErrorMessage class AuthTokenSerializer(serializers.Serializer): @@ -19,20 +20,23 @@ def validate(self, attrs): if not user.is_active: msg = _('User account is disabled.') raise serializers.ValidationError( - msg, - code='authorization' + ValidationErrorMessage( + msg, + code='authorization') ) else: msg = _('Unable to log in with provided credentials.') raise serializers.ValidationError( - msg, - code='authorization' + ValidationErrorMessage( + msg, + code='authorization') ) else: msg = _('Must include "username" and "password".') raise serializers.ValidationError( - msg, - code='authorization' + ValidationErrorMessage( + msg, + code='authorization') ) attrs['user'] = user diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index f8330bf170..7bb12b773b 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -61,7 +61,7 @@ def __str__(self): def build_error_from_django_validation_error(exc_info): code = getattr(exc_info, 'code', None) or 'invalid' return [ - ValidationError(msg, code=code) + ValidationErrorMessage(msg, code=code) for msg in exc_info.messages ] @@ -73,20 +73,26 @@ def build_error_from_django_validation_error(exc_info): # from rest_framework import serializers # raise serializers.ValidationError('Value was invalid') +class ValidationErrorMessage(six.text_type): + code = None + + def __new__(cls, string, code=None, *args, **kwargs): + self = super(ValidationErrorMessage, cls).__new__( + cls, string, *args, **kwargs) + + self.code = code + return self + + class ValidationError(APIException): status_code = status.HTTP_400_BAD_REQUEST - def __init__(self, detail, code=None): + def __init__(self, detail): # For validation errors the 'detail' key is always required. # The details should always be coerced to a list if not already. if not isinstance(detail, dict) and not isinstance(detail, list): detail = [detail] - elif isinstance(detail, dict) or (detail and isinstance(detail[0], ValidationError)): - assert code is None, ( - 'The `code` argument must not be set for compound errors.') - - self.detail = detail - self.code = code + self.detail = _force_text_recursive(detail) def __str__(self): return six.text_type(self.detail) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index e9044513be..d3bb326293 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -32,7 +32,8 @@ unicode_to_repr ) from rest_framework.exceptions import ( - ValidationError, build_error_from_django_validation_error + ValidationError, ValidationErrorMessage, + build_error_from_django_validation_error ) from rest_framework.settings import api_settings from rest_framework.utils import html, humanize_datetime, representation @@ -503,7 +504,7 @@ def run_validators(self, value): # attempting to accumulate a list of errors. if isinstance(exc.detail, dict): raise - errors.append(ValidationError(exc.detail, code=exc.code)) + errors.extend(exc.detail) except DjangoValidationError as exc: errors.extend( build_error_from_django_validation_error(exc) @@ -545,7 +546,7 @@ def fail(self, key, **kwargs): msg = MISSING_ERROR_MESSAGE.format(class_name=class_name, key=key) raise AssertionError(msg) message_string = msg.format(**kwargs) - raise ValidationError(message_string, code=key) + raise ValidationError(ValidationErrorMessage(message_string, code=key)) @cached_property def root(self): diff --git a/rest_framework/response.py b/rest_framework/response.py index 9942dd576c..0e97668eb4 100644 --- a/rest_framework/response.py +++ b/rest_framework/response.py @@ -38,6 +38,7 @@ def __init__(self, data=None, status=None, '`.error`. representation.' ) raise AssertionError(msg) + self.data = data self.template_name = template_name self.exception = exception diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index d67f047c25..db07df0a35 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -219,14 +219,7 @@ def is_valid(self, raise_exception=False): self._errors = {} if self._errors and raise_exception: - return_errors = None - if isinstance(self._errors, list): - return_errors = ReturnList(self._errors, serializer=self) - elif isinstance(self._errors, dict): - return_errors = ReturnDict(self._errors, serializer=self) - - raise ValidationError(return_errors) - + raise ValidationError(self.errors) return not bool(self._errors) @property @@ -250,42 +243,12 @@ def data(self): self._data = self.get_initial() return self._data - def _transform_to_legacy_errors(self, errors_to_transform): - # Do not mutate `errors_to_transform` here. - errors = ReturnDict(serializer=self) - for field_name, values in errors_to_transform.items(): - if isinstance(values, list): - errors[field_name] = values - continue - - if isinstance(values.detail, list): - errors[field_name] = [] - for value in values.detail: - if isinstance(value, ValidationError): - errors[field_name].extend(value.detail) - elif isinstance(value, list): - errors[field_name].extend(value) - else: - errors[field_name].append(value) - - elif isinstance(values.detail, dict): - errors[field_name] = {} - for sub_field_name, value in values.detail.items(): - errors[field_name][sub_field_name] = [] - for validation_error in value: - errors[field_name][sub_field_name].extend(validation_error.detail) - return errors - @property def errors(self): if not hasattr(self, '_errors'): msg = 'You must call `.is_valid()` before accessing `.errors`.' raise AssertionError(msg) - - if isinstance(self._errors, list): - return map(self._transform_to_legacy_errors, self._errors) - else: - return self._transform_to_legacy_errors(self._errors) + return self._errors @property def validated_data(self): @@ -460,7 +423,7 @@ def to_internal_value(self, data): message = self.error_messages['invalid'].format( datatype=type(data).__name__ ) - error = ValidationError(message, code='invalid') + error = ValidationErrorMessage(message, code='invalid') raise ValidationError({ api_settings.NON_FIELD_ERRORS_KEY: [error] }) @@ -477,7 +440,7 @@ def to_internal_value(self, data): if validate_method is not None: validated_value = validate_method(validated_value) except ValidationError as exc: - errors[field.field_name] = exc + errors[field.field_name] = exc.detail except DjangoValidationError as exc: errors[field.field_name] = ( exceptions.build_error_from_django_validation_error(exc) @@ -616,7 +579,7 @@ def to_internal_value(self, data): message = self.error_messages['not_a_list'].format( input_type=type(data).__name__ ) - error = ValidationError( + error = ValidationErrorMessage( message, code='not_a_list' ) @@ -625,9 +588,11 @@ def to_internal_value(self, data): }) if not self.allow_empty and len(data) == 0: - message = self.error_messages['empty'] + message = ValidationErrorMessage( + self.error_messages['empty'], + code='empty_not_allowed') raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [ValidationError(message, code='empty_not_allowed')] + api_settings.NON_FIELD_ERRORS_KEY: [message] }) ret = [] diff --git a/rest_framework/validators.py b/rest_framework/validators.py index 27148cedc9..86a8d75fcb 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -11,7 +11,7 @@ from django.utils.translation import ugettext_lazy as _ from rest_framework.compat import unicode_to_repr -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import ValidationError, ValidationErrorMessage from rest_framework.utils.representation import smart_repr @@ -60,7 +60,8 @@ def __call__(self, value): queryset = self.filter_queryset(value, queryset) queryset = self.exclude_current_instance(queryset) if queryset.exists(): - raise ValidationError(self.message, code='unique') + raise ValidationError(ValidationErrorMessage(self.message, + code='unique')) def __repr__(self): return unicode_to_repr('<%s(queryset=%s)>' % ( @@ -101,9 +102,10 @@ def enforce_required_fields(self, attrs): return missing = { - field_name: ValidationError( + field_name: ValidationErrorMessage( self.missing_message, code='required') + for field_name in self.fields if field_name not in attrs } @@ -149,8 +151,11 @@ def __call__(self, attrs): ] if None not in checked_values and queryset.exists(): field_names = ', '.join(self.fields) - raise ValidationError(self.message.format(field_names=field_names), - code='unique') + raise ValidationError( + ValidationErrorMessage( + self.message.format(field_names=field_names), + code='unique') + ) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, fields=%s)>' % ( @@ -188,7 +193,7 @@ def enforce_required_fields(self, attrs): 'required' state on the fields they are applied to. """ missing = { - field_name: ValidationError( + field_name: ValidationErrorMessage( self.missing_message, code='required') for field_name in [self.field, self.date_field] @@ -216,8 +221,9 @@ def __call__(self, attrs): queryset = self.exclude_current_instance(attrs, queryset) if queryset.exists(): message = self.message.format(date_field=self.date_field) - error = ValidationError(message, code='unique') - raise ValidationError({self.field: error}) + raise ValidationError({ + self.field: ValidationErrorMessage(message, code='unique'), + }) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, field=%s, date_field=%s)>' % ( diff --git a/rest_framework/views.py b/rest_framework/views.py index 915bacbc6d..56e3c4e499 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -70,7 +70,7 @@ def exception_handler(exc, context): headers['Retry-After'] = '%d' % exc.wait if isinstance(exc.detail, (list, dict)): - data = exc.detail.serializer.errors + data = exc.detail else: data = {'detail': exc.detail} diff --git a/tests/test_validation_error.py b/tests/test_validation_error.py new file mode 100644 index 0000000000..a9d244176d --- /dev/null +++ b/tests/test_validation_error.py @@ -0,0 +1,74 @@ +from django.test import TestCase + +from rest_framework import serializers, status +from rest_framework.decorators import api_view +from rest_framework.response import Response +from rest_framework.settings import api_settings +from rest_framework.test import APIRequestFactory +from rest_framework.views import APIView + +factory = APIRequestFactory() + + +class ExampleSerializer(serializers.Serializer): + char = serializers.CharField() + integer = serializers.IntegerField() + + +class ErrorView(APIView): + def get(self, request, *args, **kwargs): + ExampleSerializer(data={}).is_valid(raise_exception=True) + + +@api_view(['GET']) +def error_view(request): + ExampleSerializer(data={}).is_valid(raise_exception=True) + + +class TestValidationErrorWithCode(TestCase): + def setUp(self): + self.DEFAULT_HANDLER = api_settings.EXCEPTION_HANDLER + + def exception_handler(exc, request): + return_errors = {} + for field_name, errors in exc.detail.items(): + return_errors[field_name] = [] + for error in errors: + return_errors[field_name].append({ + 'code': error.code, + 'message': error + }) + + return Response(return_errors, status=status.HTTP_400_BAD_REQUEST) + + api_settings.EXCEPTION_HANDLER = exception_handler + + self.expected_response_data = { + 'char': [{ + 'message': 'This field is required.', + 'code': 'required', + }], + 'integer': [{ + 'message': 'This field is required.', + 'code': 'required' + }], + } + + def tearDown(self): + api_settings.EXCEPTION_HANDLER = self.DEFAULT_HANDLER + + def test_class_based_view_exception_handler(self): + view = ErrorView.as_view() + + request = factory.get('/', content_type='application/json') + response = view(request) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data, self.expected_response_data) + + def test_function_based_view_exception_handler(self): + view = error_view + + request = factory.get('/', content_type='application/json') + response = view(request) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data, self.expected_response_data) From 7971343a6457c7c72bf48c9c0a3838f01ea67cb3 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Thu, 17 Dec 2015 15:44:17 +0100 Subject: [PATCH 5/9] Modify ValidationError api `ValidationErrorMessage` is now abstracted in `ValidationError`'s constructor --- rest_framework/authtoken/serializers.py | 20 +++++++------------- rest_framework/exceptions.py | 6 +++++- rest_framework/fields.py | 5 ++--- rest_framework/serializers.py | 1 + rest_framework/validators.py | 9 +++------ 5 files changed, 18 insertions(+), 23 deletions(-) diff --git a/rest_framework/authtoken/serializers.py b/rest_framework/authtoken/serializers.py index 99ea86e860..7c49f33056 100644 --- a/rest_framework/authtoken/serializers.py +++ b/rest_framework/authtoken/serializers.py @@ -2,7 +2,6 @@ from django.utils.translation import ugettext_lazy as _ from rest_framework import serializers -from rest_framework.exceptions import ValidationErrorMessage class AuthTokenSerializer(serializers.Serializer): @@ -20,24 +19,19 @@ def validate(self, attrs): if not user.is_active: msg = _('User account is disabled.') raise serializers.ValidationError( - ValidationErrorMessage( - msg, - code='authorization') - ) + msg, + code='authorization') else: msg = _('Unable to log in with provided credentials.') raise serializers.ValidationError( - ValidationErrorMessage( - msg, - code='authorization') - ) + msg, + code='authorization') + else: msg = _('Must include "username" and "password".') raise serializers.ValidationError( - ValidationErrorMessage( - msg, - code='authorization') - ) + msg, + code='authorization') attrs['user'] = user return attrs diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 7bb12b773b..7465415ea4 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -87,7 +87,11 @@ def __new__(cls, string, code=None, *args, **kwargs): class ValidationError(APIException): status_code = status.HTTP_400_BAD_REQUEST - def __init__(self, detail): + def __init__(self, detail, code=None): + # If code is there, this means we are dealing with a message. + if code and not isinstance(detail, ValidationErrorMessage): + detail = ValidationErrorMessage(detail, code=code) + # For validation errors the 'detail' key is always required. # The details should always be coerced to a list if not already. if not isinstance(detail, dict) and not isinstance(detail, list): diff --git a/rest_framework/fields.py b/rest_framework/fields.py index d3bb326293..2932de4fe4 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -32,8 +32,7 @@ unicode_to_repr ) from rest_framework.exceptions import ( - ValidationError, ValidationErrorMessage, - build_error_from_django_validation_error + ValidationError, build_error_from_django_validation_error ) from rest_framework.settings import api_settings from rest_framework.utils import html, humanize_datetime, representation @@ -546,7 +545,7 @@ def fail(self, key, **kwargs): msg = MISSING_ERROR_MESSAGE.format(class_name=class_name, key=key) raise AssertionError(msg) message_string = msg.format(**kwargs) - raise ValidationError(ValidationErrorMessage(message_string, code=key)) + raise ValidationError(message_string, code=key) @cached_property def root(self): diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index db07df0a35..79f80e30a1 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -24,6 +24,7 @@ from rest_framework.compat import DurationField as ModelDurationField from rest_framework.compat import JSONField as ModelJSONField from rest_framework.compat import postgres_fields, unicode_to_repr +from rest_framework.exceptions import ValidationErrorMessage from rest_framework.utils import model_meta from rest_framework.utils.field_mapping import ( ClassLookupDict, get_field_kwargs, get_nested_relation_kwargs, diff --git a/rest_framework/validators.py b/rest_framework/validators.py index 86a8d75fcb..11e986232f 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -60,8 +60,7 @@ def __call__(self, value): queryset = self.filter_queryset(value, queryset) queryset = self.exclude_current_instance(queryset) if queryset.exists(): - raise ValidationError(ValidationErrorMessage(self.message, - code='unique')) + raise ValidationError(self.message, code='unique') def __repr__(self): return unicode_to_repr('<%s(queryset=%s)>' % ( @@ -152,10 +151,8 @@ def __call__(self, attrs): if None not in checked_values and queryset.exists(): field_names = ', '.join(self.fields) raise ValidationError( - ValidationErrorMessage( - self.message.format(field_names=field_names), - code='unique') - ) + self.message.format(field_names=field_names), + code='unique') def __repr__(self): return unicode_to_repr('<%s(queryset=%s, fields=%s)>' % ( From 24ba3b3743bfee94e86c711b2bd03087d6bd51d6 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Fri, 18 Dec 2015 13:24:45 +0100 Subject: [PATCH 6/9] Review step 1 - simplify to clarify --- rest_framework/exceptions.py | 25 +------------------------ rest_framework/fields.py | 6 ++---- rest_framework/serializers.py | 23 ++++++----------------- rest_framework/validators.py | 13 ++++--------- 4 files changed, 13 insertions(+), 54 deletions(-) diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 7465415ea4..7cee4468c7 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -58,45 +58,22 @@ def __str__(self): return self.detail -def build_error_from_django_validation_error(exc_info): - code = getattr(exc_info, 'code', None) or 'invalid' - return [ - ValidationErrorMessage(msg, code=code) - for msg in exc_info.messages - ] - - # The recommended style for using `ValidationError` is to keep it namespaced # under `serializers`, in order to minimize potential confusion with Django's # built in `ValidationError`. For example: # # from rest_framework import serializers # raise serializers.ValidationError('Value was invalid') - -class ValidationErrorMessage(six.text_type): - code = None - - def __new__(cls, string, code=None, *args, **kwargs): - self = super(ValidationErrorMessage, cls).__new__( - cls, string, *args, **kwargs) - - self.code = code - return self - - class ValidationError(APIException): status_code = status.HTTP_400_BAD_REQUEST def __init__(self, detail, code=None): - # If code is there, this means we are dealing with a message. - if code and not isinstance(detail, ValidationErrorMessage): - detail = ValidationErrorMessage(detail, code=code) - # For validation errors the 'detail' key is always required. # The details should always be coerced to a list if not already. if not isinstance(detail, dict) and not isinstance(detail, list): detail = [detail] self.detail = _force_text_recursive(detail) + self.code = code def __str__(self): return six.text_type(self.detail) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 2932de4fe4..469c573f78 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -32,7 +32,7 @@ unicode_to_repr ) from rest_framework.exceptions import ( - ValidationError, build_error_from_django_validation_error + ValidationError ) from rest_framework.settings import api_settings from rest_framework.utils import html, humanize_datetime, representation @@ -505,9 +505,7 @@ def run_validators(self, value): raise errors.extend(exc.detail) except DjangoValidationError as exc: - errors.extend( - build_error_from_django_validation_error(exc) - ) + errors.extend(exc.messages) if errors: raise ValidationError(errors) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 79f80e30a1..99d36a8a54 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -20,11 +20,9 @@ from django.utils.functional import cached_property from django.utils.translation import ugettext_lazy as _ -from rest_framework import exceptions from rest_framework.compat import DurationField as ModelDurationField from rest_framework.compat import JSONField as ModelJSONField from rest_framework.compat import postgres_fields, unicode_to_repr -from rest_framework.exceptions import ValidationErrorMessage from rest_framework.utils import model_meta from rest_framework.utils.field_mapping import ( ClassLookupDict, get_field_kwargs, get_nested_relation_kwargs, @@ -221,6 +219,7 @@ def is_valid(self, raise_exception=False): if self._errors and raise_exception: raise ValidationError(self.errors) + return not bool(self._errors) @property @@ -301,8 +300,7 @@ def get_validation_error_detail(exc): # exception class as well for simpler compat. # Eg. Calling Model.clean() explicitly inside Serializer.validate() return { - api_settings.NON_FIELD_ERRORS_KEY: - exceptions.build_error_from_django_validation_error(exc) + api_settings.NON_FIELD_ERRORS_KEY: list(exc.messages) } elif isinstance(exc.detail, dict): # If errors may be a dict we use the standard {key: list of values}. @@ -424,9 +422,8 @@ def to_internal_value(self, data): message = self.error_messages['invalid'].format( datatype=type(data).__name__ ) - error = ValidationErrorMessage(message, code='invalid') raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [error] + api_settings.NON_FIELD_ERRORS_KEY: [message] }) ret = OrderedDict() @@ -443,9 +440,7 @@ def to_internal_value(self, data): except ValidationError as exc: errors[field.field_name] = exc.detail except DjangoValidationError as exc: - errors[field.field_name] = ( - exceptions.build_error_from_django_validation_error(exc) - ) + errors[field.field_name] = list(exc.messages) except SkipField: pass else: @@ -580,18 +575,12 @@ def to_internal_value(self, data): message = self.error_messages['not_a_list'].format( input_type=type(data).__name__ ) - error = ValidationErrorMessage( - message, - code='not_a_list' - ) raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [error] + api_settings.NON_FIELD_ERRORS_KEY: [message] }) if not self.allow_empty and len(data) == 0: - message = ValidationErrorMessage( - self.error_messages['empty'], - code='empty_not_allowed') + message = self.error_messages['empty'] raise ValidationError({ api_settings.NON_FIELD_ERRORS_KEY: [message] }) diff --git a/rest_framework/validators.py b/rest_framework/validators.py index 11e986232f..53d5479ce7 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -11,7 +11,7 @@ from django.utils.translation import ugettext_lazy as _ from rest_framework.compat import unicode_to_repr -from rest_framework.exceptions import ValidationError, ValidationErrorMessage +from rest_framework.exceptions import ValidationError from rest_framework.utils.representation import smart_repr @@ -101,10 +101,7 @@ def enforce_required_fields(self, attrs): return missing = { - field_name: ValidationErrorMessage( - self.missing_message, - code='required') - + field_name: self.missing_message for field_name in self.fields if field_name not in attrs } @@ -190,9 +187,7 @@ def enforce_required_fields(self, attrs): 'required' state on the fields they are applied to. """ missing = { - field_name: ValidationErrorMessage( - self.missing_message, - code='required') + field_name: self.missing_message for field_name in [self.field, self.date_field] if field_name not in attrs } @@ -219,7 +214,7 @@ def __call__(self, attrs): if queryset.exists(): message = self.message.format(date_field=self.date_field) raise ValidationError({ - self.field: ValidationErrorMessage(message, code='unique'), + self.field: message, }) def __repr__(self): From ba21a1e551c047dc7cd5563b3c7d1fe0ec5b94ad Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Fri, 18 Dec 2015 14:15:34 +0100 Subject: [PATCH 7/9] review cleanup --- rest_framework/authtoken/serializers.py | 15 ++++++--------- rest_framework/fields.py | 4 +--- rest_framework/validators.py | 6 +++--- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/rest_framework/authtoken/serializers.py b/rest_framework/authtoken/serializers.py index 7c49f33056..42d67c2d3c 100644 --- a/rest_framework/authtoken/serializers.py +++ b/rest_framework/authtoken/serializers.py @@ -18,20 +18,17 @@ def validate(self, attrs): if user: if not user.is_active: msg = _('User account is disabled.') - raise serializers.ValidationError( - msg, - code='authorization') + code = 'authorization' + raise serializers.ValidationError(msg, code=code) else: msg = _('Unable to log in with provided credentials.') - raise serializers.ValidationError( - msg, - code='authorization') + code = 'authorization' + raise serializers.ValidationError(msg, code=code) else: msg = _('Must include "username" and "password".') - raise serializers.ValidationError( - msg, - code='authorization') + code = 'authorization' + raise serializers.ValidationError(msg, code=code) attrs['user'] = user return attrs diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 469c573f78..27e064040f 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -31,9 +31,7 @@ MinValueValidator, duration_string, parse_duration, unicode_repr, unicode_to_repr ) -from rest_framework.exceptions import ( - ValidationError -) +from rest_framework.exceptions import ValidationError from rest_framework.settings import api_settings from rest_framework.utils import html, humanize_datetime, representation diff --git a/rest_framework/validators.py b/rest_framework/validators.py index 53d5479ce7..d964215425 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -147,9 +147,9 @@ def __call__(self, attrs): ] if None not in checked_values and queryset.exists(): field_names = ', '.join(self.fields) - raise ValidationError( - self.message.format(field_names=field_names), - code='unique') + message = self.message.format(field_names=field_names) + code = 'unique' + raise ValidationError(message, code=code) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, fields=%s)>' % ( From 12e4b8f3b17b07481b77e4c7cf4cee6ab3c16c78 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Fri, 18 Dec 2015 23:03:14 +0100 Subject: [PATCH 8/9] WIP - Integrate 2 tuples solution for error codes This is still a wip, the code is uggly and will need to be refactored and reviewed a lot! --- rest_framework/exceptions.py | 60 ++++++++++++++++++++++++++++-- rest_framework/fields.py | 8 ++-- rest_framework/serializers.py | 67 ++++++++++++++++++++++++++-------- rest_framework/validators.py | 11 +++--- tests/test_validation_error.py | 8 ++-- 5 files changed, 122 insertions(+), 32 deletions(-) diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 7cee4468c7..2d1474050c 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -58,22 +58,74 @@ def __str__(self): return self.detail +def build_error_from_django_validation_error(exc_info): + code = getattr(exc_info, 'code', None) or 'invalid' + return [ + (msg, code) + for msg in exc_info.messages + ] + # The recommended style for using `ValidationError` is to keep it namespaced # under `serializers`, in order to minimize potential confusion with Django's # built in `ValidationError`. For example: # # from rest_framework import serializers # raise serializers.ValidationError('Value was invalid') + + class ValidationError(APIException): status_code = status.HTTP_400_BAD_REQUEST + code = None def __init__(self, detail, code=None): # For validation errors the 'detail' key is always required. # The details should always be coerced to a list if not already. - if not isinstance(detail, dict) and not isinstance(detail, list): - detail = [detail] - self.detail = _force_text_recursive(detail) - self.code = code + + if code: + self.full_details = [(detail, code)] + else: + self.full_details = detail + + if isinstance(self.full_details, tuple): + self.detail, self.code = self.full_details + self.detail = [self.detail] + + elif isinstance(self.full_details, list): + if isinstance(self.full_details, ReturnList): + self.detail = ReturnList(serializer=self.full_details.serializer) + else: + self.detail = [] + for error in self.full_details: + if isinstance(error, tuple): + message, code = error + self.detail.append(message) + elif isinstance(error, dict): + self.detail = self.full_details + break + + elif isinstance(self.full_details, dict): + if isinstance(self.full_details, ReturnDict): + self.detail = ReturnDict(serializer=self.full_details.serializer) + else: + self.detail = {} + + for field_name, errors in self.full_details.items(): + self.detail[field_name] = [] + if isinstance(errors, tuple): + message, code = errors + self.detail[field_name].append(message) + elif isinstance(errors, list): + for error in errors: + if isinstance(error, tuple): + message, code = error + else: + message = error + if message: + self.detail[field_name].append(message) + else: + self.detail = [self.full_details] + + self.detail = _force_text_recursive(self.detail) def __str__(self): return six.text_type(self.detail) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 27e064040f..35df9225cb 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -31,7 +31,9 @@ MinValueValidator, duration_string, parse_duration, unicode_repr, unicode_to_repr ) -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import ( + ValidationError, build_error_from_django_validation_error +) from rest_framework.settings import api_settings from rest_framework.utils import html, humanize_datetime, representation @@ -501,9 +503,9 @@ def run_validators(self, value): # attempting to accumulate a list of errors. if isinstance(exc.detail, dict): raise - errors.extend(exc.detail) + errors.append((exc.detail, exc.code)) except DjangoValidationError as exc: - errors.extend(exc.messages) + errors.extend(build_error_from_django_validation_error(exc)) if errors: raise ValidationError(errors) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 99d36a8a54..27e764da34 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -23,6 +23,7 @@ from rest_framework.compat import DurationField as ModelDurationField from rest_framework.compat import JSONField as ModelJSONField from rest_framework.compat import postgres_fields, unicode_to_repr +from rest_framework.exceptions import build_error_from_django_validation_error from rest_framework.utils import model_meta from rest_framework.utils.field_mapping import ( ClassLookupDict, get_field_kwargs, get_nested_relation_kwargs, @@ -213,12 +214,12 @@ def is_valid(self, raise_exception=False): self._validated_data = self.run_validation(self.initial_data) except ValidationError as exc: self._validated_data = {} - self._errors = exc.detail + self._errors = exc.full_details else: self._errors = {} if self._errors and raise_exception: - raise ValidationError(self.errors) + raise ValidationError(self._errors) return not bool(self._errors) @@ -248,7 +249,36 @@ def errors(self): if not hasattr(self, '_errors'): msg = 'You must call `.is_valid()` before accessing `.errors`.' raise AssertionError(msg) - return self._errors + + if isinstance(self._errors, dict): + errors = ReturnDict(serializer=self) + for key, value in self._errors.items(): + if isinstance(value, dict): + errors[key] = {} + for key_, value_ in value.items(): + message, code = value_[0] + errors[key][key_] = [message] + elif isinstance(value, list): + if isinstance(value[0], tuple): + message, code = value[0] + else: + message = value[0] + if isinstance(message, list): + errors[key] = message + else: + errors[key] = [message] + elif isinstance(value, tuple): + message, code = value + errors[key] = [message] + else: + errors[key] = [value] + elif isinstance(self._errors, list): + errors = ReturnList(self._errors, serializer=self) + else: + # This shouldn't ever happen. + errors = self._errors + + return errors @property def validated_data(self): @@ -299,24 +329,25 @@ def get_validation_error_detail(exc): # inside your codebase, but we handle Django's validation # exception class as well for simpler compat. # Eg. Calling Model.clean() explicitly inside Serializer.validate() + error = build_error_from_django_validation_error(exc) return { - api_settings.NON_FIELD_ERRORS_KEY: list(exc.messages) + api_settings.NON_FIELD_ERRORS_KEY: error } - elif isinstance(exc.detail, dict): + elif isinstance(exc.full_details, dict): # If errors may be a dict we use the standard {key: list of values}. # Here we ensure that all the values are *lists* of errors. return { key: value if isinstance(value, list) else [value] - for key, value in exc.detail.items() + for key, value in exc.full_details.items() } - elif isinstance(exc.detail, list): + elif isinstance(exc.full_details, list): # Errors raised as a list are non-field errors. return { - api_settings.NON_FIELD_ERRORS_KEY: exc.detail + api_settings.NON_FIELD_ERRORS_KEY: exc.full_details } # Errors raised as a string are non-field errors. return { - api_settings.NON_FIELD_ERRORS_KEY: [exc.detail] + api_settings.NON_FIELD_ERRORS_KEY: [exc.full_details] } @@ -422,12 +453,13 @@ def to_internal_value(self, data): message = self.error_messages['invalid'].format( datatype=type(data).__name__ ) + code = 'invalid' raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [message] + api_settings.NON_FIELD_ERRORS_KEY: [(message, code)] }) - ret = OrderedDict() - errors = OrderedDict() + ret = ReturnDict(serializer=self) + errors = ReturnDict(serializer=self) fields = self._writable_fields for field in fields: @@ -438,9 +470,10 @@ def to_internal_value(self, data): if validate_method is not None: validated_value = validate_method(validated_value) except ValidationError as exc: - errors[field.field_name] = exc.detail + errors[field.field_name] = exc.full_details except DjangoValidationError as exc: - errors[field.field_name] = list(exc.messages) + error = build_error_from_django_validation_error(exc) + errors[field.field_name] = error except SkipField: pass else: @@ -575,14 +608,16 @@ def to_internal_value(self, data): message = self.error_messages['not_a_list'].format( input_type=type(data).__name__ ) + code = 'not_a_list' raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [message] + api_settings.NON_FIELD_ERRORS_KEY: [(message, code)] }) if not self.allow_empty and len(data) == 0: message = self.error_messages['empty'] + code = 'empty_not_allowed' raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [message] + api_settings.NON_FIELD_ERRORS_KEY: [(message, code)] }) ret = [] diff --git a/rest_framework/validators.py b/rest_framework/validators.py index d964215425..8fe629c72c 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -100,8 +100,9 @@ def enforce_required_fields(self, attrs): if self.instance is not None: return + code = 'required' missing = { - field_name: self.missing_message + field_name: [(self.missing_message, code)] for field_name in self.fields if field_name not in attrs } @@ -186,8 +187,9 @@ def enforce_required_fields(self, attrs): The `UniqueForValidator` classes always force an implied 'required' state on the fields they are applied to. """ + code = 'required' missing = { - field_name: self.missing_message + field_name: [(self.missing_message, code)] for field_name in [self.field, self.date_field] if field_name not in attrs } @@ -213,9 +215,8 @@ def __call__(self, attrs): queryset = self.exclude_current_instance(attrs, queryset) if queryset.exists(): message = self.message.format(date_field=self.date_field) - raise ValidationError({ - self.field: message, - }) + code = 'unique' + raise ValidationError({self.field: [(message, code)]}) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, field=%s, date_field=%s)>' % ( diff --git a/tests/test_validation_error.py b/tests/test_validation_error.py index a9d244176d..fbde190220 100644 --- a/tests/test_validation_error.py +++ b/tests/test_validation_error.py @@ -31,12 +31,12 @@ def setUp(self): def exception_handler(exc, request): return_errors = {} - for field_name, errors in exc.detail.items(): + for field_name, errors in exc.full_details.items(): return_errors[field_name] = [] - for error in errors: + for message, code in errors: return_errors[field_name].append({ - 'code': error.code, - 'message': error + 'code': code, + 'message': message }) return Response(return_errors, status=status.HTTP_400_BAD_REQUEST) From 2344d2270a851acea6a3089b9d70b92df3fd7ee2 Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Tue, 22 Dec 2015 16:02:24 +0100 Subject: [PATCH 9/9] Deal with ValidationError instantiation --- rest_framework/exceptions.py | 76 +++++++++++++++++----------------- rest_framework/fields.py | 2 +- rest_framework/serializers.py | 61 ++++++++------------------- rest_framework/validators.py | 10 ++--- tests/test_validation_error.py | 2 +- 5 files changed, 63 insertions(+), 88 deletions(-) diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 2d1474050c..5f1b27c766 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -7,6 +7,7 @@ from __future__ import unicode_literals import math +from collections import namedtuple from django.utils import six from django.utils.encoding import force_text @@ -61,7 +62,7 @@ def __str__(self): def build_error_from_django_validation_error(exc_info): code = getattr(exc_info, 'code', None) or 'invalid' return [ - (msg, code) + ErrorDetails(msg, code) for msg in exc_info.messages ] @@ -72,60 +73,61 @@ def build_error_from_django_validation_error(exc_info): # from rest_framework import serializers # raise serializers.ValidationError('Value was invalid') +ErrorDetails = namedtuple('ErrorDetails', ['message', 'code']) + class ValidationError(APIException): status_code = status.HTTP_400_BAD_REQUEST code = None def __init__(self, detail, code=None): - # For validation errors the 'detail' key is always required. - # The details should always be coerced to a list if not already. - if code: - self.full_details = [(detail, code)] + self.full_details = ErrorDetails(detail, code) else: self.full_details = detail - if isinstance(self.full_details, tuple): - self.detail, self.code = self.full_details - self.detail = [self.detail] + if not isinstance(self.full_details, dict) \ + and not isinstance(self.full_details, list): + self.full_details = [self.full_details] + self.full_details = _force_text_recursive(self.full_details) - elif isinstance(self.full_details, list): + self.detail = detail + if isinstance(self.full_details, list): if isinstance(self.full_details, ReturnList): - self.detail = ReturnList(serializer=self.full_details.serializer) + self.detail = ReturnList( + serializer=self.full_details.serializer) else: self.detail = [] - for error in self.full_details: - if isinstance(error, tuple): - message, code = error - self.detail.append(message) - elif isinstance(error, dict): - self.detail = self.full_details - break - + for full_detail in self.full_details: + if isinstance(full_detail, ErrorDetails): + self.detail.append(full_detail.message) + elif isinstance(full_detail, dict): + if not full_detail: + self.detail.append(full_detail) + for key, value in full_detail.items(): + if isinstance(value, list): + self.detail.append( + {key: [item.message] + if isinstance(item, ErrorDetails) + else [item] for item in value}) + elif isinstance(full_detail, list): + self.detail.extend(full_detail) + else: + self.detail.append(full_detail) elif isinstance(self.full_details, dict): if isinstance(self.full_details, ReturnDict): - self.detail = ReturnDict(serializer=self.full_details.serializer) + self.detail = ReturnDict( + serializer=self.full_details.serializer) else: self.detail = {} - - for field_name, errors in self.full_details.items(): - self.detail[field_name] = [] - if isinstance(errors, tuple): - message, code = errors - self.detail[field_name].append(message) - elif isinstance(errors, list): - for error in errors: - if isinstance(error, tuple): - message, code = error - else: - message = error - if message: - self.detail[field_name].append(message) - else: - self.detail = [self.full_details] - - self.detail = _force_text_recursive(self.detail) + for field_name, full_detail in self.full_details.items(): + if isinstance(full_detail, list): + self.detail[field_name] = [ + item.message if isinstance(item, ErrorDetails) else item + for item in full_detail + ] + else: + self.detail[field_name] = full_detail def __str__(self): return six.text_type(self.detail) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 35df9225cb..1f588e990a 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -503,7 +503,7 @@ def run_validators(self, value): # attempting to accumulate a list of errors. if isinstance(exc.detail, dict): raise - errors.append((exc.detail, exc.code)) + errors.append(exc.full_details) except DjangoValidationError as exc: errors.extend(build_error_from_django_validation_error(exc)) if errors: diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 27e764da34..77c5cd182f 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -23,7 +23,9 @@ from rest_framework.compat import DurationField as ModelDurationField from rest_framework.compat import JSONField as ModelJSONField from rest_framework.compat import postgres_fields, unicode_to_repr -from rest_framework.exceptions import build_error_from_django_validation_error +from rest_framework.exceptions import ( + ErrorDetails, build_error_from_django_validation_error +) from rest_framework.utils import model_meta from rest_framework.utils.field_mapping import ( ClassLookupDict, get_field_kwargs, get_nested_relation_kwargs, @@ -214,12 +216,12 @@ def is_valid(self, raise_exception=False): self._validated_data = self.run_validation(self.initial_data) except ValidationError as exc: self._validated_data = {} - self._errors = exc.full_details + self._errors = exc.detail else: self._errors = {} if self._errors and raise_exception: - raise ValidationError(self._errors) + raise ValidationError(self.errors) return not bool(self._errors) @@ -249,36 +251,7 @@ def errors(self): if not hasattr(self, '_errors'): msg = 'You must call `.is_valid()` before accessing `.errors`.' raise AssertionError(msg) - - if isinstance(self._errors, dict): - errors = ReturnDict(serializer=self) - for key, value in self._errors.items(): - if isinstance(value, dict): - errors[key] = {} - for key_, value_ in value.items(): - message, code = value_[0] - errors[key][key_] = [message] - elif isinstance(value, list): - if isinstance(value[0], tuple): - message, code = value[0] - else: - message = value[0] - if isinstance(message, list): - errors[key] = message - else: - errors[key] = [message] - elif isinstance(value, tuple): - message, code = value - errors[key] = [message] - else: - errors[key] = [value] - elif isinstance(self._errors, list): - errors = ReturnList(self._errors, serializer=self) - else: - # This shouldn't ever happen. - errors = self._errors - - return errors + return self._errors @property def validated_data(self): @@ -333,21 +306,21 @@ def get_validation_error_detail(exc): return { api_settings.NON_FIELD_ERRORS_KEY: error } - elif isinstance(exc.full_details, dict): + elif isinstance(exc.detail, dict): # If errors may be a dict we use the standard {key: list of values}. # Here we ensure that all the values are *lists* of errors. return { key: value if isinstance(value, list) else [value] - for key, value in exc.full_details.items() + for key, value in exc.detail.items() } - elif isinstance(exc.full_details, list): + elif isinstance(exc.detail, list): # Errors raised as a list are non-field errors. return { - api_settings.NON_FIELD_ERRORS_KEY: exc.full_details + api_settings.NON_FIELD_ERRORS_KEY: exc.detail } # Errors raised as a string are non-field errors. return { - api_settings.NON_FIELD_ERRORS_KEY: [exc.full_details] + api_settings.NON_FIELD_ERRORS_KEY: [exc.detail] } @@ -455,11 +428,11 @@ def to_internal_value(self, data): ) code = 'invalid' raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [(message, code)] + api_settings.NON_FIELD_ERRORS_KEY: [ErrorDetails(message, code)] }) - ret = ReturnDict(serializer=self) - errors = ReturnDict(serializer=self) + ret = OrderedDict() + errors = OrderedDict() fields = self._writable_fields for field in fields: @@ -470,7 +443,7 @@ def to_internal_value(self, data): if validate_method is not None: validated_value = validate_method(validated_value) except ValidationError as exc: - errors[field.field_name] = exc.full_details + errors[field.field_name] = exc.detail except DjangoValidationError as exc: error = build_error_from_django_validation_error(exc) errors[field.field_name] = error @@ -610,14 +583,14 @@ def to_internal_value(self, data): ) code = 'not_a_list' raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [(message, code)] + api_settings.NON_FIELD_ERRORS_KEY: [ErrorDetails(message, code)] }) if not self.allow_empty and len(data) == 0: message = self.error_messages['empty'] code = 'empty_not_allowed' raise ValidationError({ - api_settings.NON_FIELD_ERRORS_KEY: [(message, code)] + api_settings.NON_FIELD_ERRORS_KEY: [ErrorDetails(message, code)] }) ret = [] diff --git a/rest_framework/validators.py b/rest_framework/validators.py index 8fe629c72c..07c8eb464e 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -11,7 +11,7 @@ from django.utils.translation import ugettext_lazy as _ from rest_framework.compat import unicode_to_repr -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import ErrorDetails, ValidationError from rest_framework.utils.representation import smart_repr @@ -102,7 +102,7 @@ def enforce_required_fields(self, attrs): code = 'required' missing = { - field_name: [(self.missing_message, code)] + field_name: ErrorDetails(self.missing_message, code) for field_name in self.fields if field_name not in attrs } @@ -150,7 +150,7 @@ def __call__(self, attrs): field_names = ', '.join(self.fields) message = self.message.format(field_names=field_names) code = 'unique' - raise ValidationError(message, code=code) + raise ValidationError(ErrorDetails(message, code=code)) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, fields=%s)>' % ( @@ -189,7 +189,7 @@ def enforce_required_fields(self, attrs): """ code = 'required' missing = { - field_name: [(self.missing_message, code)] + field_name: ErrorDetails(self.missing_message, code) for field_name in [self.field, self.date_field] if field_name not in attrs } @@ -216,7 +216,7 @@ def __call__(self, attrs): if queryset.exists(): message = self.message.format(date_field=self.date_field) code = 'unique' - raise ValidationError({self.field: [(message, code)]}) + raise ValidationError({self.field: ErrorDetails(message, code)}) def __repr__(self): return unicode_to_repr('<%s(queryset=%s, field=%s, date_field=%s)>' % ( diff --git a/tests/test_validation_error.py b/tests/test_validation_error.py index fbde190220..7d2ec1f8d3 100644 --- a/tests/test_validation_error.py +++ b/tests/test_validation_error.py @@ -31,7 +31,7 @@ def setUp(self): def exception_handler(exc, request): return_errors = {} - for field_name, errors in exc.full_details.items(): + for field_name, errors in exc.detail.items(): return_errors[field_name] = [] for message, code in errors: return_errors[field_name].append({