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 partial update for the ListSerializer. #4222

Merged
merged 3 commits into from May 2, 2017

Conversation

NewVadim
Copy link
Contributor

@NewVadim NewVadim commented Jun 24, 2016

I have two serializers:

class ItemSerializer(serializers.ModelSerializer):
    class Meta:
        model = Item

class ObjectSerializer(serializers.ModelSerializer):
    class Meta:
        model = Object
        fields = ['id', 'title', 'items']

    items = ItemSerializer(many=True, allow_empty=False)

When I make partial update for ObjectSerializer without items, I get the error:

{'items': {'non_field_errors': ['This list may not be empty.']}}

@codecov-io
Copy link

codecov-io commented Jun 24, 2016

Current coverage is 91.54%

No coverage report found for master at 3a7bfdf.

Powered by Codecov. Last updated by 3a7bfdf...c752e96

@tomchristie tomchristie added the Bug label May 2, 2017
@tomchristie tomchristie added this to the 3.6.3 Release milestone May 2, 2017
@tomchristie
Copy link
Member

Looks good to me, nice work!

@tomchristie tomchristie merged commit dea601d into encode:master May 2, 2017
@gabn88
Copy link
Contributor

gabn88 commented May 15, 2017

This commit make the update from DRF 3.5.3 to 3.6.3 fail with:


  File "/lib/python3.5/site-packages/rest_framework/serials
    self._fields[key] = value
  File "/lib/python3.5/site-packages/rest_framework/utils/_
    field.bind(field_name=key, parent=self.serializer)
  File "/lib/python3.5/site-packages/rest_framework/seriald
    self.partial = self.parent.partial
AttributeError: 'YourSerializer' object has no attribute 'partial'

Cause is an entry in YourSerializer that nests another serializer:

class YourGroupSerializer(ModelValidatorMixin, SecureSerializerMixin, serializers.HyperlinkedModelSerializer):
    
    # Mostly used for SETTING
    your_set = YourSerializer(many=True, read_only=True)

Which then fails on the SecureSerializerMixin that filters nested entries on allowed entrys for that user:

class SecureSerializerMixin(object):
    """
    Filters the queryset for a related field to those items the user has permissions for.
    
    Does work with all serializers, but in general on nested serializers you don't use the queryset method, so then it does not work.
    
    IN THE SERIALIZER
    
    Without this mixin, the browsable api would show all options, not only those of the user.
    
    Also without this mixin, it would be possible to post (=create) a horse in another stable, etc.
    
    """
    def filter_queryset(self, field, kwargs):
        try: 

            request = kwargs['context']['request']
            # Not only for GET requests, but also for posts filter!
            if not kwargs['context'].get('do_not_filter_api', False):  # disables uneccesary filtering of serializers for performance
                self.fields.get(field).queryset = filter_queryset(request, self.fields.get(field).queryset)
            
            
        except AttributeError:
            pass
        except KeyError:

            # No context. For safety use empty queryset!
            try:
                self.fields.get(field).queryset = self.field.get(field).queryset.none()
            except AttributeError:
                pass

    def __init__(self, *args, **kwargs):

        for field in self.fields:
            self.filter_queryset(field, kwargs)

        
        super(SecureSerializerMixin, self).__init__(*args, **kwargs)

The SecureSerializerMixin fails on for field in self.fields.

@xordoquy
Copy link
Collaborator

That's because you are calling self.fields before the serializer gets initialized. BaseSerializer.__init__ sets this.

@gabn88
Copy link
Contributor

gabn88 commented May 16, 2017

@xordoquy my hero 👍 💯

@xordoquy
Copy link
Collaborator

@gabn88 glad you got it working

@kronion
Copy link

kronion commented Sep 17, 2017

Sorry to revive an old issue, but then is there a recommended way to dynamically control the set of fields on a serializer instance? In 3.5.3 I was passing kwargs to my serializers' __init__() methods to add, swap, or remove fields from self.fields, which I thought was more elegant than having potentially large numbers of (sub)classes for every combination of fields I might want to return for a particular view.

@xordoquy
Copy link
Collaborator

@kronion I don't have the time to go through the backlog but altering fields should be fine, shouldn't it ?

@kronion
Copy link

kronion commented Sep 18, 2017

Do you mean that I need to alter the fields after __init__()? Here's an example of what I was doing before:

class ExampleSerializer(ExampleBaseSerializer):
    bar = BarSerializer()

    def __init__(self, *args, **kwargs):
        if "foo" in kwargs and kwargs.pop("foo"):
            self.fields["foo"] = FooSerializer()
        if "no_bar" in kwargs and kwargs.pop("no_bar"):
            del self.fields["bar"]
        super(ExampleSerializer, self).__init__(*args, **kwargs)

@xordoquy
Copy link
Collaborator

That's because you are calling self.fields before the serializer gets initialized. BaseSerializer.init sets this.

reupen added a commit to reupen/django-rest-framework that referenced this pull request Mar 17, 2019
Refs encode#6509

This enforces allow_empty=True when a ListSerializer is a child of another serializer and partial validation is being performed on the parent serializer.

This is because partial validation should allow fields to be omitted, but should not cause values that are invalid without partial validation to become valid.

This effectively reverts encode#4222. None of the tests added in that PR fail if the associated change is removed, so I‘m not sure what that PR was trying to fix.
reupen added a commit to reupen/django-rest-framework that referenced this pull request May 2, 2019
Refs encode#6509

This enforces allow_empty=True when a ListSerializer is a child of another serializer and partial validation is being performed on the parent serializer.

This is because partial validation should allow fields to be omitted, but should not cause values that are invalid without partial validation to become valid.

This effectively reverts encode#4222. None of the tests added in that PR fail if the associated change is removed, so I‘m not sure what that PR was trying to fix.
tomchristie pushed a commit that referenced this pull request Jul 1, 2019
…zer (#6512)

Refs #6509

This enforces allow_empty=True when a ListSerializer is a child of another serializer and partial validation is being performed on the parent serializer.

This is because partial validation should allow fields to be omitted, but should not cause values that are invalid without partial validation to become valid.

This effectively reverts #4222. None of the tests added in that PR fail if the associated change is removed, so I‘m not sure what that PR was trying to fix.
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
…zer (encode#6512)

Refs encode#6509

This enforces allow_empty=True when a ListSerializer is a child of another serializer and partial validation is being performed on the parent serializer.

This is because partial validation should allow fields to be omitted, but should not cause values that are invalid without partial validation to become valid.

This effectively reverts encode#4222. None of the tests added in that PR fail if the associated change is removed, so I‘m not sure what that PR was trying to fix.
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
…zer (encode#6512)

Refs encode#6509

This enforces allow_empty=True when a ListSerializer is a child of another serializer and partial validation is being performed on the parent serializer.

This is because partial validation should allow fields to be omitted, but should not cause values that are invalid without partial validation to become valid.

This effectively reverts encode#4222. None of the tests added in that PR fail if the associated change is removed, so I‘m not sure what that PR was trying to fix.
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

6 participants