-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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.
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
Changes from 1 commit
8c29efe
1834760
c7351b3
42f4c55
7971343
24ba3b3
ba21a1e
12e4b8f
2344d22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
`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.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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='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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,8 @@ | |
| unicode_to_repr | ||
| ) | ||
| from rest_framework.exceptions import ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well keep that formatting as-is.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, 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): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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') | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
|
|
@@ -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)>' % ( | ||
|
|
||
| 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) |
There was a problem hiding this comment.
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
ValidationErrorMessageto me. This is the same as:Right? Or not?
There was a problem hiding this comment.
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
ValidationErrorMessagewithinValidationError.__init__().So, I could change the use of
ValidationErrorwithout breaking anything indeed. And that would also enforce thatValidationError.detailwill always be aValidationErrorMessage.There was a problem hiding this comment.
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
messageargument, rather than adding a new, separate, optionalcodeargument. 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/#ValidationErrorThere was a problem hiding this comment.
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
ValidationErrorin the forms API?There was a problem hiding this comment.
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
codeon the string-like object?Because right now I don't quite see how we could maintain backward compatibility without that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well there's two issues:
ValidationError- an optionalcodeargument here would be fine.