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.

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

Raising ValidationError from Serializer.to_internal_value() with error message(s) as list causes ValueError #3864

Closed
ryochiji opened this issue Jan 22, 2016 · 8 comments · Fixed by #5466

Comments

@ryochiji
Copy link

>>> from rest_framework.serializers import Serializer
>>> from rest_framework.exceptions import ValidationError
>>> class FooSerializer(Serializer):
...   def to_internal_value(self, data):
...     raise ValidationError(['foo error'])
... 
>>> foo = FooSerializer(data={})
>>> foo.is_valid(raise_exception=True)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/altschool/vishnu-backend/lib/python2.7/site-packages/rest_framework/serializers.py", line 221, in is_valid
    raise ValidationError(self.errors)
  File "/usr/local/altschool/vishnu-backend/lib/python2.7/site-packages/rest_framework/serializers.py", line 509, in errors
    return ReturnDict(ret, serializer=self)
  File "/usr/local/altschool/vishnu-backend/lib/python2.7/site-packages/rest_framework/utils/serializer_helpers.py", line 20, in __init__
    super(ReturnDict, self).__init__(*args, **kwargs)
  File "/Users/ryochiji/.pyenv/versions/2.7.8/lib/python2.7/collections.py", line 52, in __init__
    self.__update(*args, **kwds)
  File "/usr/local/altschool/vishnu-backend/lib/python2.7/_abcoll.py", line 566, in update
    for key, value in other:
ValueError: too many values to unpack

This seems to happen because Serializer.errors passes self._errors to ReturnDict without type-checking the way ListSerializer.errors does.

@laz2
Copy link

laz2 commented Mar 16, 2016

Breaking changes in usage self.errors instead self._errors in older versions.

From official documentation:

To create a read-write serializer we first need to implement a .to_internal_value() method. This method returns the validated values that will be used to construct the object instance, and may raise a ValidationError if the supplied data is in an incorrect format.

I think, line value = self.to_internal_value(data) should be under try-except to proper exception converting.

@laz2
Copy link

laz2 commented Mar 17, 2016

Original to_internal_value always raises exception in dict-format:
raise ValidationError({ api_settings.NON_FIELD_ERRORS_KEY: [message] })

@maryokhin
Copy link
Contributor

This should probably at least be mentioned in the docs somewhere?

@mikeengland
Copy link

I'm also experiencing this issue. Raising a validation error inside to_internal_value throws the same exception as above. This can be handled outside to_internal_value so it isn't a blocker by any means.

@practual
Copy link

This issue can also be seen if you pass data=None to a serializer, e.g.:

class MySerializer(serializers.Serializer):
    name = serializers.CharField()

    class Meta:
        fields = ('name',)

serializer = MySerializer(data=None)
serializer.is_valid()
serializer.errors
>>> ValueError: too many values to unpack (expected 2)

I believe this error could be avoided by wrapping self.validate_empty_values(data) with a check for self.parent to determine whether the serializer is nested or not. If the serializer is not nested, it should raise the error as a non-field error.

@carltongibson carltongibson removed this from the 3.6.5 Release milestone Sep 28, 2017
@carltongibson
Copy link
Collaborator

I'm going to de-milestone for now. We can reassess after v3.7

@rpkilby
Copy link
Member

rpkilby commented Sep 29, 2017

Hi all, I've opened #5466 with a docs clarification. In short, the recommended approach is:

  • Override .validate() when you want to perform object-level validation. You can raise ValidationError('msg') and ValidationError(['msg 1', 'msg 2']) as expected.
  • Override .to_internal_value() when you need to alter how data is deserialized. If deserialization fails, it's expected that you raise a ValidationError with a dictionary mapping field names / settings.NON_FIELD_ERRORS_KEY to a list of error messages.

@tomchristie
Copy link
Member

#5466 Is a good start, but I think we want to restructure that slightly.

The opening paragraphs for that section might be the right place make your validate vs to_internal_value point, tho I'm open to discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants