Skip to content
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

Introduce error code for validation errors. #3716

Closed
wants to merge 9 commits into from
Closed

Introduce error code for validation errors. #3716

Show file tree
Hide file tree
Changes from 4 commits
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
19 changes: 16 additions & 3 deletions rest_framework/authtoken/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -18,13 +19,25 @@ def validate(self, attrs):
if user:
if not user.is_active:
msg = _('User account is disabled.')
raise serializers.ValidationError(msg)
raise serializers.ValidationError(
ValidationErrorMessage(
msg,
code='authorization')
Copy link
Member

Choose a reason for hiding this comment

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

These lines demonstrate a bit of the weirdness of ValidationErrorMessage to me. This is the same as:

raise serializers.ValidationError(msg, code='authorization')

Right? Or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it could be the same if I'd move the logic that actually constructs the ValidationErrorMessage within ValidationError.__init__().

So, I could change the use of ValidationError without breaking anything indeed. And that would also enforce that ValidationError.detail will always be a ValidationErrorMessage.

Copy link
Member

Choose a reason for hiding this comment

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

This is just me not quite mentally parsing that this change
modify the existing message argument, rather than adding a new, separate, optional code argument. Apologies for continuing to be a thorn in this, but can't say I'm terribly keen on this API. I'd rather see us eg mirror Django here... https://docs.djangoproject.com/en/1.9/_modules/django/core/exceptions/#ValidationError

Copy link
Member

Choose a reason for hiding this comment

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

It'd be interesting to take a look at how Django exposes the 'code' information of ValidationError in the forms API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a change on the api to make it more like the django api.
Good point about the forms API, i'll look into that.

Just to clarify, are you against the fact that we put the code on the string-like object?
Because right now I don't quite see how we could maintain backward compatibility without that.

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, are you against the fact that we put the code on the string-like object?
Because right now I don't quite see how we could maintain backward compatibility without that.

Well there's two issues:

  1. Where it goes in the ValidationError - an optional code argument here would be fine.
  2. How we present it on the serializer - seeing what Django does in the Forms API would be useful here.

)
else:
msg = _('Unable to log in with provided credentials.')
raise serializers.ValidationError(msg)
raise serializers.ValidationError(
Copy link
Member

Choose a reason for hiding this comment

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

I guess these would be marginally cleaner reformatted as three lines...

msg = ...
code = ...
raise serializers.ValidationError(msg, code)

ValidationErrorMessage(
msg,
code='authorization')
)
else:
msg = _('Must include "username" and "password".')
raise serializers.ValidationError(msg)
raise serializers.ValidationError(
ValidationErrorMessage(
msg,
code='authorization')
)

attrs['user'] = user
return attrs
19 changes: 19 additions & 0 deletions rest_framework/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,32 @@ 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

Expand Down
11 changes: 8 additions & 3 deletions rest_framework/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@
MinValueValidator, duration_string, parse_duration, unicode_repr,
unicode_to_repr
)
from rest_framework.exceptions import ValidationError
from rest_framework.exceptions import (
Copy link
Member

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

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, ValidationErrorMessage,
build_error_from_django_validation_error
)
from rest_framework.settings import api_settings
from rest_framework.utils import html, humanize_datetime, representation

Expand Down Expand Up @@ -503,7 +506,9 @@ def run_validators(self, value):
raise
errors.extend(exc.detail)
except DjangoValidationError as exc:
errors.extend(exc.messages)
errors.extend(
build_error_from_django_validation_error(exc)
)
if errors:
raise ValidationError(errors)

Expand Down Expand Up @@ -541,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)
raise ValidationError(ValidationErrorMessage(message_string, code=key))

@cached_property
def root(self):
Expand Down
22 changes: 16 additions & 6 deletions rest_framework/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -219,7 +220,6 @@ 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 @@ -300,7 +300,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}.
Expand Down Expand Up @@ -422,8 +423,9 @@ 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: [message]
api_settings.NON_FIELD_ERRORS_KEY: [error]
})

ret = OrderedDict()
Expand All @@ -440,7 +442,9 @@ def to_internal_value(self, data):
except ValidationError as exc:
errors[field.field_name] = exc.detail
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:
Expand Down Expand Up @@ -575,12 +579,18 @@ 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: [message]
api_settings.NON_FIELD_ERRORS_KEY: [error]
})

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: [message]
})
Expand Down
24 changes: 18 additions & 6 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
from rest_framework.exceptions import ValidationError, ValidationErrorMessage
from rest_framework.utils.representation import smart_repr


Expand Down Expand Up @@ -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)
raise ValidationError(ValidationErrorMessage(self.message,
code='unique'))

def __repr__(self):
return unicode_to_repr('<%s(queryset=%s)>' % (
Expand Down Expand Up @@ -101,7 +102,10 @@ def enforce_required_fields(self, attrs):
return

missing = {
field_name: self.missing_message
field_name: ValidationErrorMessage(
self.missing_message,
code='required')

Copy link
Member

Choose a reason for hiding this comment

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

Let's drop this whitespace.

for field_name in self.fields
if field_name not in attrs
}
Expand Down Expand Up @@ -147,7 +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))
raise ValidationError(
ValidationErrorMessage(
self.message.format(field_names=field_names),
code='unique')
)

def __repr__(self):
return unicode_to_repr('<%s(queryset=%s, fields=%s)>' % (
Expand Down Expand Up @@ -185,7 +193,9 @@ def enforce_required_fields(self, attrs):
'required' state on the fields they are applied to.
"""
missing = {
field_name: self.missing_message
field_name: ValidationErrorMessage(
self.missing_message,
code='required')
for field_name in [self.field, self.date_field]
if field_name not in attrs
}
Expand All @@ -211,7 +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)
raise ValidationError({self.field: message})
raise ValidationError({
self.field: ValidationErrorMessage(message, code='unique'),
Copy link
Member

Choose a reason for hiding this comment

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

Not 100% convinced by the new ValidationErrorMessage and the change in the error argument for nested cases here. It ends up being a bit odd because in the nested case these's a different way of indicating the error code, than is used in the simple case.

Alternatives might include (1) constraining the code argument to only support simple cases, or (2) support code as a nested structure.

We might need to do some more thinking in this area. Unsure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain how the error argument for nested cases changes here ? I'm not following that part.

From what I get, everything stayed the same, it's just that the message became a little bit smarter than before and it is opt-in so you'd only have to change anything if you actually want to use error codes.

})
Copy link
Member

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

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

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

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
Member

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

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
Member

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

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

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):
return unicode_to_repr('<%s(queryset=%s, field=%s, date_field=%s)>' % (
Expand Down
74 changes: 74 additions & 0 deletions tests/test_validation_error.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)