Skip to content

DictField and ListField error if child is a serializer with many=True #5384

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

Closed
5 of 6 tasks
warrd opened this issue Sep 1, 2017 · 11 comments
Closed
5 of 6 tasks

DictField and ListField error if child is a serializer with many=True #5384

warrd opened this issue Sep 1, 2017 · 11 comments

Comments

@warrd
Copy link

warrd commented Sep 1, 2017

The following example fails during the bind method:

from rest_framework import serializers

class ChildSerializer(serializers.Serializer):
    pass

class ParentSerializer(serializers.Serializer):
    child = serializers.DictField(child=ChildSerializer(many=True))

Here is a partial stacktrace:

/usr/local/lib/python3.5/dist-packages/rest_framework/serializers.py in bind(self, field_name, parent)
    568     def bind(self, field_name, parent):
    569         super(ListSerializer, self).bind(field_name, parent)
--> 570         self.partial = self.parent.partial
    571 

AttributeError: 'DictField' object has no attribute 'partial'

When many=False this serializer works as expected.

The problem is introduced by #4222, which assumes the parent of ListSerializer is a serializer instance, which would mean the field was declared on a serializer class rather than as a child argument to a DictField / ListField. Is this a fair assumption to make?

The documentation for DictField and ListField only mention a Field instance is expected for the child argument. Serializer is a subclass of Field so I think it is fair to assume a serializer can be used here.

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)
@warrd warrd changed the title DictField and ListField fail if child is a serializer with many=True DictField and ListField error if child is a serializer with many=True Sep 1, 2017
@carltongibson
Copy link
Collaborator

Hi @warrd is the issue here not with adding the many=True?

A ListField has a child. Let’s say it’s a CharField for example. Here what sense does the many add? I can’t see any. (List/Dict fields are already many.)

So why are you trying to add the many? What are you looking for there?

My first thought then is perhaps we need a guard raising an error if child has the many=True?

@NickStefan
Copy link

Just hit this as well when upgrading from 3.6.0 --> 3.6.3.

The issue is that its raising an error that it previously did not raise :). I changed all the many=True to many=False and this did raise quite a few errors, so there definitely is at least some breaking change going on here.

@NickStefan
Copy link

A ListField has a child. Let’s say it’s a CharField for example. Here what sense does the many add? I can’t see any. (List/Dict fields are already many.)

In our case we have definitions like this:

class NodeTreeSerializer(TreeSerializer):
    trees = serializers.ListField(
        child = NodeSerializer(many=True)
    )

which translate into roughly this:

{ 
   trees: [
      [<node>, <node>], 
      [<node>, <node>]
   ]
}

@NickStefan
Copy link

was able to get it to work by changing to this:

class NodeTreeSerializer(TreeSerializer):
    trees = serializers.ListField(
        child = serializers.ListField(
            child = NodeSerializer(many=True)
         )
    )

So my guess is that @warrd could do this:

class ParentSerializer(serializers.Serializer):
    child = serializers.DictField(child=
          serializers.ListField(
                child = ChildSerializer()
          )
    )

still not good for a minor version bump, but workable.

@carltongibson
Copy link
Collaborator

@NickStefan Any chance you could git bisect this to work out where the change was introduced?

@warrd
Copy link
Author

warrd commented Sep 15, 2017

@NickStefan cheers for the suggestion, that's exactly what we ended up doing :)

@carltongibson I linked to where the change was introduced in my original comment, here it is again: https://github.com/encode/django-rest-framework/pull/4222/files#diff-80f43677c94659fbd60c8687cce68eafR538

@carltongibson
Copy link
Collaborator

@warrd Sorry, yes. Thanks!

@josert
Copy link

josert commented Nov 7, 2017

Hi, same problem with DictField. I have tried with the ListField approach but getting the same error.
Is there any date planned in the roadmap where the issue could be fixed?
Thanks in advance!
(version used 3.7.3)

@philipforget
Copy link
Contributor

Dragging up a bit of an old issue here but I had this same issue recently when upgrading from an older version of drf. This was happening on a nested serializer that was re-defining only a subset of it's fields as class attributes and also specifying the fields attribute in the Meta class. eg:

# Throws the error documented in this issue
class SomeSerializer(serializers.ModelSerializer):
    some_many_field = SomeManySerializer(many=True)
    
    class Meta:
        model = SomeModel
        fields = ['some_many_field', 'some_other_field']

It hadn't been a problem in earlier versions of drf without the explicit bind of the parent being called. The fix for me was to include an explicit definition of the some_other_field as an attribute, eg:

# Works
class SomeSerializer(serializers.ModelSerializer):
    some_many_field = SomeManySerializer(many=True)
    some_other_field = serializers.CharField(required=False)

    class Meta:
        model = SomeModel
        fields = ['some_many_field', 'some_other_field']

I haven't done a deep dive to see why this is the case.

@carltongibson
Copy link
Collaborator

self.partial = self.parent.partial

It looks like this could be addressed adding a partial property to DictField/ListField that proxied to the parent serialiser. A PR with test cases would be welcome.

@tomchristie
Copy link
Member

Closing as (very) stale.

@tomchristie tomchristie closed this as not planned Won't fix, can't repro, duplicate, stale Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants