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

Fix nested validation error being rendered incorrectly. #3801

Merged
merged 1 commit into from Jan 7, 2016

Conversation

craigds
Copy link
Contributor

@craigds craigds commented Jan 6, 2016

Previously an extra list wrapped nested validation errors raised from serializer's validate() methods.
That was inconsistent with the format of validation errors raised by validate_<fieldname> methods.
i.e. these two resulted in different behaviour:

def validate_foo(self):
    raise ValidationError(['bar'])

...

def validate(self):
    raise ValidationError({'foo': ['bar']})

Previously an extra list wrapped nested validation errors raised from serializer's validate() methods.
That was inconsistent with the format of validation errors raised by validate_<fieldname> methods.
i.e. these two resulted in *different* behaviour:

    def validate_foo(self):
        raise ValidationError(['bar'])

    def validate(self):
        raise ValidationError({'foo': ['bar']})
@tomchristie
Copy link
Member

tomchristie commented Jan 6, 2016

i.e. these two resulted in different behavior

Looks good to me. For clarity, could you show me:

  • what those two cases currently result with?
  • which of the two case is now used consistently after your fix?

@craigds
Copy link
Contributor Author

craigds commented Jan 7, 2016

I went all-out and tried three different approaches to raising the same error. In the process I found my description of the problem was slightly incorrect. The issue occurs when raising a nested ValidationError from an outer serializer. It's not actually about validate vs validate_foo.

This code shows all three cases:

from rest_framework import serializers

# validation error is created by the outer serializer's validate() method
class Nested1(serializers.Serializer):
    foo = serializers.CharField()

class Outer1(serializers.Serializer):
    nested = Nested1()

    def validate(self, attrs):
        raise serializers.ValidationError({'nested': {'foo': ['bar']}})


# error is created by the inner serializer's validate() method
class Nested2(serializers.Serializer):
    foo = serializers.CharField()

    def validate(self, attrs):
        raise serializers.ValidationError({'foo': ['bar']})

class Outer2(serializers.Serializer):
    nested = Nested2()


# error is created by the inner serializer's validate_foo() method
class Nested3(serializers.Serializer):
    foo = serializers.CharField()

    def validate_foo(self, value):
        raise serializers.ValidationError(['bar'])

class Outer3(serializers.Serializer):
    nested = Nested3()


o1 = Outer1(data={'nested': {'foo': 'thing'}})
o1.is_valid()
o2 = Outer2(data={'nested': {'foo': 'thing'}})
o2.is_valid()
o3 = Outer3(data={'nested': {'foo': 'thing'}})
o3.is_valid()

print o1.errors
print o2.errors
print o3.errors

This results in:

{'nested': [{'foo': ['bar']}]}
{'nested': {'foo': ['bar']}}
{'nested': OrderedDict([('foo', ['bar'])])}

Case 1 is the weird one; cases 2 and 3 are the same as each other (OrderedDict notwithstanding)

My fix changes case 1, so the new output is:

{'nested': {'foo': ['bar']}}
{'nested': {'foo': ['bar']}}
{'nested': OrderedDict([('foo', ['bar'])])}

in which all three are equivalent.

@johnraz
Copy link
Contributor

johnraz commented Jan 7, 2016

In light of what I've been doing for the error codes on validation errors in #3716, this makes full sense and the test case will help too 👍

@tomchristie tomchristie added the Bug label Jan 7, 2016
@tomchristie tomchristie added this to the 3.3.3 Release milestone Jan 7, 2016
@tomchristie
Copy link
Member

tomchristie commented Jan 7, 2016

Thanks - nice work! 😄

tomchristie added a commit that referenced this pull request Jan 7, 2016
Fix nested validation error being rendered incorrectly.
@tomchristie tomchristie merged commit f01a3d9 into encode:master Jan 7, 2016
2 checks passed
@remik
Copy link

remik commented Mar 23, 2016

Is it possible to avoid it for DictField or JSONField? At this moment if I validate dict and want to raise with key in this value I am getting only list of keys. Eg. code

class SerializerWithDictField(Serializer):
    data = serializers.DictField()

    def validate_data(self, data):
        if not data.get('key'):
            raise ValidationError({'key': 'This key is required'})
        return data

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

Successfully merging this pull request may close these issues.

None yet

4 participants