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

ListSerializer.to_representation cuts data and KeyError is raised if the queryset used has been modified #2727

Closed
MattBlack85 opened this issue Mar 19, 2015 · 10 comments

Comments

@MattBlack85
Copy link

I discovered this problem after upgrading to 3.1, after 86c5fa2 if a QuerySet is passed as iterable data.all() will be called. That's fine until we start to add extra data to the QS and use normal serializers for output.
Case:

objs = Model.objects.filter(x=y)
for obj in objs:
    obj.update(extra_dict)
serializer = MySerializer(objs, many=True)

serializer.data will contain only data from QS, ignoring totally the other portion of data added later and ending with a KeyError because we're missing fields that are declared on serializer, this wasn't happening before since data.all() wasn't called while passing QS around.

@xordoquy
Copy link
Collaborator

It's the second time we are bitten by the QS reevaluation in the ListSerializer (previous was #2704).
I'm really not convinced 86c5fa2 was a good idea after all.

@MattBlack85 MattBlack85 changed the title ListSerializer.to_representation cuts data and KeyError is raised if the queyset used has been modified ListSerializer.to_representation cuts data and KeyError is raised if the queryset used has been modified Mar 19, 2015
@mcbhenwood
Copy link
Contributor

Seconded. This is currently causing some grief to me as well. @bakaat suggested this quick fix:

Serializer(instance=list(objs), many=True)

which "works" in terms of the data you get out, but isn't desirable for large querysets

@bakaat
Copy link

bakaat commented Apr 21, 2015

Indeed it's not. And it would possibly require you to rewrite all calls to serializer.

While that's what I suggested, I actually worked around it by subclassing Serializer (We've got our own base class extending the ModelSerializer anyways)

from rest_framework import serializers

class ModelSerializer(serializers.ModelSerializer):
    def __new__(cls, *args, **kwargs):
        if 'many' in kwargs and isinstance(
                kwargs.get('instance', None), QuerySet):
            kwargs['instance'] = (x for x in kwargs['instance'])
        return super(ModelSerializer, cls).__new__(cls, *args, **kwargs)

Generator should be more efficient than list and still prevents reevalution. This said it still remains a quick fix

@tomchristie
Copy link
Member

That's fine until we start to add extra data to the QS

@MattBlack85 (or anyone else)

On further review I didn't fully understand this.
What exactly did you mean here - what kind of extra data, and added how? could you give an example?

@tomchristie
Copy link
Member

Closing as a duplicate of #2704, but it'd be useful to know if there are other types of "extra data" along with prefetch_related that also demonstrate the issue.

@bakaat
Copy link

bakaat commented Jun 24, 2015

@tomchristie In my situation it was something like

qs = Abc.objects.all()[:10]
for o in qs:
    o.additional_attr = 'abc'
Serializer(instance=qs, many=True)

This actually causes the items in qs to be re-fetched from the db and additional_attr to be lost in the process (as far as I recall at this moment)

@tomchristie
Copy link
Member

Okay. That's (arguably) a slightly quirky thing to do, but it is at least a very simple demonstration of the issue! :) Thank you.

@bakaat
Copy link

bakaat commented Jun 24, 2015

Yup :)

@MattBlack85
Copy link
Author

My case regards fetching data from another service, having then those data in a dict I was trying to update every object in my QS with that dictionary I got from the service:

fetched_data = {'x': {'attr_a': 'some', ...},...}
qs = Model.objects.all().values()
for obj in qs:
    obj.update(fetched_data.get(obj['id']), {})

almost the same

@tomchristie
Copy link
Member

Grand, thanks both.
I've commented here #2704 (comment) on what's required to progress the issue.

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

No branches or pull requests

5 participants