-
-
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
This is still a wip, the code is uggly and will need to be refactored and reviewed a lot!
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
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. There is obviously something wrong with all those isinstance checks and if you have any idea how to get rid of them or at least improve this code, I'm listening ;-) |
||
|
|
||
| def __str__(self): | ||
| return six.text_type(self.detail) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 ( | ||
|
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 | ||
| ) | ||
| 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) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
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. We should find a way to get access to the exception here and just return exc.detail instead of doing all this. I thought about putting the exception directly in self._errors, I don't know how you feel about that ? |
||
|
|
||
| @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 = [] | ||
|
|
||
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.
We don't really want this stuff in. At least at this point.
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.
What do you mean ? Above or below ?
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.
The population of
.full_details. Should just be storing the message and code at this point.(Let me know if I'm being braindead tho - I only get a brief amount of time on each review)
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'm not sure to follow.
It seems to me that this is what I'm doing right now:
It does store message and code for single items, a list when a list is provided and a dict when a dict is provided
Basically it does store what is passed to the
__init__method.We then need to transform whatever is given as the
detailparameter to the formerself.detailformat which is what this code does (quite painfully).