From df0d814665f3b0dab5e3782363abca2b89160e7f Mon Sep 17 00:00:00 2001 From: Jonathan Liuti Date: Mon, 7 Dec 2015 10:48:53 +0100 Subject: [PATCH] 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. We 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/authtoken/serializers.py | 15 ++++-- rest_framework/exceptions.py | 17 ++++++- rest_framework/fields.py | 12 +++-- rest_framework/response.py | 1 - rest_framework/serializers.py | 61 +++++++++++++++++++++---- rest_framework/validators.py | 16 +++++-- rest_framework/views.py | 2 +- 7 files changed, 100 insertions(+), 24 deletions(-) diff --git a/rest_framework/authtoken/serializers.py b/rest_framework/authtoken/serializers.py index df0c48b86..ce6bb9c79 100644 --- a/rest_framework/authtoken/serializers.py +++ b/rest_framework/authtoken/serializers.py @@ -18,13 +18,22 @@ class AuthTokenSerializer(serializers.Serializer): 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 29afaffe0..e23b7cd31 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -58,6 +58,14 @@ class APIException(Exception): 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 @@ class APIException(Exception): 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 f76e4e801..39a5e3395 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -34,7 +34,9 @@ from rest_framework import ISO_8601 from rest_framework.compat import ( get_remote_field, unicode_repr, unicode_to_repr, value_from_object ) -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 @@ -507,9 +509,11 @@ class Field(object): # 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) @@ -547,7 +551,7 @@ class Field(object): 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/response.py b/rest_framework/response.py index 4b863cb99..e9ceb2741 100644 --- a/rest_framework/response.py +++ b/rest_framework/response.py @@ -38,7 +38,6 @@ class Response(SimpleTemplateResponse): '`.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 4d1ed63ae..a4dbc6449 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -22,6 +22,7 @@ from django.db.models.fields import FieldDoesNotExist 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 JSONField as ModelJSONField from rest_framework.compat import postgres_fields, unicode_to_repr from rest_framework.utils import model_meta @@ -219,7 +220,13 @@ class BaseSerializer(Field): 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 +251,42 @@ class BaseSerializer(Field): 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): @@ -301,7 +338,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}. @@ -423,8 +461,9 @@ class Serializer(BaseSerializer): 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() @@ -439,9 +478,11 @@ class Serializer(BaseSerializer): 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: @@ -580,14 +621,18 @@ class ListSerializer(BaseSerializer): 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 ef23b9bd7..90483eeeb 100644 --- a/rest_framework/validators.py +++ b/rest_framework/validators.py @@ -79,7 +79,7 @@ class UniqueValidator(object): queryset = self.filter_queryset(value, queryset) queryset = self.exclude_current_instance(queryset) if qs_exists(queryset): - raise ValidationError(self.message) + raise ValidationError(self.message, code='unique') def __repr__(self): return unicode_to_repr('<%s(queryset=%s)>' % ( @@ -120,7 +120,9 @@ class UniqueTogetherValidator(object): 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 } @@ -166,7 +168,8 @@ class UniqueTogetherValidator(object): ] if None not in checked_values and qs_exists(queryset): 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)>' % ( @@ -204,7 +207,9 @@ class BaseUniqueForValidator(object): '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 } @@ -230,7 +235,8 @@ class BaseUniqueForValidator(object): queryset = self.exclude_current_instance(attrs, queryset) if qs_exists(queryset): 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 15d8c6cde..8b6f060d4 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -71,7 +71,7 @@ def exception_handler(exc, context): headers['Retry-After'] = '%d' % exc.wait if isinstance(exc.detail, (list, dict)): - data = exc.detail + data = exc.detail.serializer.errors else: data = {'detail': exc.detail}