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

Nested writable serializers validation in ModelSerializer.update method seems wrong #2194

Closed
mtschammer opened this issue Dec 3, 2014 · 4 comments · Fixed by #2196
Closed
Labels
Milestone

Comments

@mtschammer
Copy link

mtschammer commented Dec 3, 2014

The update method on ModelSerializer makes sure that the data doesn't contain nested writable fields of the BaseSerializer variety, as per the documentation. However, it doesn't actually assert this by looking at the passed validated_attrs. Instead, it does this by looking at self.fields:

assert not any(
            isinstance(field, BaseSerializer) and not field.read_only
            for field in self.fields.values()

This means that even if we pop() the data of the nested serializer (as shown in the changelog example), the assertion still fails. This makes it necessary to call explicitly delete the field from self.fields:

def update(self, instance, validated_attrs):
        try:
            # Removing user data - not wanted for an update.
            validated_attrs.pop('user')
            self.fields.__delitem__('user') # This shouldn't be necessary.
        except KeyError:
            pass

        return super(CustomerSerializer, self).update(instance=instance, validated_attrs=validated_attrs)

By the time it gets to update, the data has already been validated. So you should be able to assume that any fields not present can be ignored, no?

In short, I'd say that it shouldn't trigger the error if the field isn't in the validated_attrs.

@tomchristie
Copy link
Member

tomchristie commented Dec 3, 2014

Okay, didn't consider the case of making a change to validated_attrs and then calling into the implementation with super.

@mtschammer
Copy link
Author

mtschammer commented Dec 3, 2014

I'm just lazy and wanted to avoid having to write my own update code. Sorry!

@tomchristie
Copy link
Member

tomchristie commented Dec 3, 2014

It's a valid report :) the style in the reference pull request may be less brittle in any case?

@mtschammer
Copy link
Author

mtschammer commented Dec 3, 2014

Looks good!

@tomchristie tomchristie added this to the 3.0.1 Release milestone Dec 3, 2014
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 a pull request may close this issue.

2 participants