Skip to content

Introduce error code for validation errors. #3716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
johnraz wants to merge 9 commits into from
+171 −24
Closed

Introduce error code for validation errors. #3716

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8c29efe
Introduce error code for validation errors.
johnraz Dec 7, 2015
1834760
Dont change return type of `Serializer.errors`
johnraz Dec 10, 2015
c7351b3
Revert change on existing tests
johnraz Dec 16, 2015
42f4c55
Introduce ValidationErrorMessage
johnraz Dec 16, 2015
7971343
Modify ValidationError api
johnraz Dec 17, 2015
24ba3b3
Review step 1 - simplify to clarify
johnraz Dec 18, 2015
ba21a1e
review cleanup
johnraz Dec 18, 2015
12e4b8f
WIP - Integrate 2 tuples solution for error codes
johnraz Dec 18, 2015
2344d22
Deal with ValidationError instantiation
johnraz Dec 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Review step 1 - simplify to clarify
  • Loading branch information
@johnraz
johnraz committed Dec 18, 2015
commit 24ba3b3743bfee94e86c711b2bd03087d6bd51d6
25 changes: 1 addition & 24 deletions rest_framework/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 2 additions & 4 deletions rest_framework/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
unicode_to_repr
)
from rest_framework.exceptions import (
Copy link
Contributor

@lovelydinosaur lovelydinosaur Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well keep that formatting as-is.

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed blame my IDE XD

ValidationError, build_error_from_django_validation_error
ValidationError
)
from rest_framework.settings import api_settings
from rest_framework.utils import html, humanize_datetime, representation
Expand Down Expand Up @@ -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)

Expand Down
23 changes: 6 additions & 17 deletions rest_framework/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}.
Expand Down Expand Up @@ -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()
Expand All @@ -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:
Expand Down Expand Up @@ -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]
})
Expand Down
13 changes: 4 additions & 9 deletions rest_framework/validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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,
})
Copy link
Contributor

@lovelydinosaur lovelydinosaur Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this missing a code or not?

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If message is always a single value, then the code is missing yes.

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why ValidationError is taking a dict here and not a simple message ? It is really not clear to me why, even by looking at the code base.

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my first thought was wrong, the code is not missing but will have to be exposed in the message somehow once we expose the code to the serializer.

Copy link
Contributor

@lovelydinosaur lovelydinosaur Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why ValidationError is taking a dict here and not a simple message

This is a serializer-level validator, but needs to indicate that the validation error corresponded to a particular field.

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to my previous comment, this code shows why having the code here is useless:
https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/fields.py#L502

Copy link
Contributor

@lovelydinosaur lovelydinosaur Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not really the issue at this point in time. Yes we still need to figure out if and how we'd bridge "these exceptions can have an associated code" with "and here's how we expose that in the serializer API", but the exceptions themselves should include the code.

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This the touchy part.
I don't think we should be adding the code to a ValidationError that takes a list or a dict as its first argument.
The code should only mean something directly attached to a single error. Not to a list or a dictionary, even if they only contain one error.

Copy link
Contributor Author

@johnraz johnraz Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is, whatever we do to construct or expose it later on, this structure looks wrong:

{
    'field': 'error', 
    'code': 'the code'
}

while this looks better

{
    'field': {
        'message': 'error',
        'code': 'the code'
    }
}


def __repr__(self):
Expand Down