clokep / django-querysetsequence

Chain multiple (disparate) QuerySets in Django
https://django-querysetsequence.readthedocs.io/
ISC License
108 stars 25 forks source link

AttributeError: 'QuerySetSequence' object has no attribute '_prefetch_related_lookups' #67

Closed jpic closed 4 years ago

jpic commented 4 years ago

Any idea why ModelChoiceIterator does not work with QSS anymore ? Will look further into this anyway, apparently it broke django-autocomplete-light

self = <django.forms.models.ModelChoiceIterator object at 0x7f801689ced0>

    def __iter__(self):

        if self.field.empty_label is not None:

            yield ("", self.field.empty_label)

        queryset = self.queryset

        # Can't use iterator() when queryset uses prefetch_related()

>       if not queryset._prefetch_related_lookups:

E       AttributeError: 'QuerySetSequence' object has no attribute '_prefetch_related_lookups'

../.tox/base-py37-django21-sqlite/lib/python3.7/site-packages/django/forms/models.py:1137: AttributeError
clokep commented 4 years ago

@jpic What version is this with? I haven't changed anything in a while besides removing some Python 2 compatibility, but that was just some future imports and such I believe.

Did a change to django-autocomplete-light break this?

jpic commented 4 years ago

No change on my side, seems to have broken for the first time in DAL's pipeline on December 17th. It went un-noticed because it tests a commit that's in a branch and which broke a bunch of selenium tests and was abandoned.

ModelChoiceIterator comes from Django, and apparently it's checks if the qs has _prefetch_related_lookups to False to decide if it is going to call queryset.iterator().

        # Can't use iterator() when queryset uses prefetch_related()
        if not queryset._prefetch_related_lookups:
            queryset = queryset.iterator()

TBH, it looks pretty hacky, I would rather see Django do queryset.iterator() in a try/except, and see iterator() raise an exception if the queryset uses prefetch_related() which seems to be the problem in the comment.

Anyway, I could trace usage of that attribute to as far as 2015, in a commit that's in Django 1.10, when everything was definitely working.

Still digging, I am trying to reproduce it in a test in QSS, where tox runs fine with Python 3.8 as far as with Django 1.10, with Django 1.11 I get a couple of failures, is it your case too or is it something wrong perhaps with my environment ?

Also, I've asked the django-developers mailing list if they an effort to make django.forms.models rely only the public API of QuerySet would be acceptable, to make projects like QSS easier.

jpic commented 4 years ago

Also, I've asked the django-developers mailing list if they an effort to make django.forms.models rely only the public API of QuerySet would be acceptable, to make projects like QSS more easy to use in forms.

I mean, if QSS had to implement a method that says if yes or no calling iterator() is fine, it would be pretty easy for you right ?

Meanwhile, I would understand that you would not want to add a _prefetch_related_lookups attribute, and will try to monkey patch the QSS object with that to get things to work again for DAL users.

Have a great day

clokep commented 4 years ago

I mean, if QSS had to implement a method that says if yes or no calling iterator() is fine, it would be pretty easy for you right ?

I usually try to update this with changes to the public API, yes. 👍

It looks like this is a compatibility issue with Django, which is rather unfortunate. I wouldn't be 100% against adding a _prefetch_related_lookups, but would need to ensure that the definition of it is sane. For a situation where prefetch_related is called on the QuerySetSequence this is easy-ish to do, but you can also create a QuerySetSequence with a QuerySet that already has prefetch_related called on it.

From the way it is used, it might be OK to define it as:

@property
def _prefetch_related_lookups(self):
    return any(map(lambda qs: qs._prefetch_related_lookups, self._querysets))
jpic commented 4 years ago

There's been no reply on the django-developpers thread, so we have two options:

clokep commented 4 years ago

@jpic Or both -- we add the property for now and file a ticket with Django?

If adding the property, please add a comment about why it is necessary (and add some unit tests!)

jpic commented 4 years ago

All right then, thank you for your support @clokep

jpic commented 4 years ago

We just got a reply from charettes, linking to ongoing changes on the QuerySet API that relate to the code we're having issues with: https://groups.google.com/g/django-developers/c/ADgUd6jRvdw/m/_ZnETNlcAAAJ Can't dig right now though

In another recent thread, they are also talking about adding a contains() method to QuerySet, thought you might want to know about it. QuerySet.contains() thread: https://groups.google.com/forum/#!msg/django-developers/NZaMq9BALrs/OCNTh6QyCAAJ

Our thread: https://groups.google.com/g/django-developers/c/gpQROT-kimE/m/qPNPMUi8AAAJ

jpic commented 4 years ago

Ok, seems like that code is probably going away from django forms in 3.2 or 3.3, we can implement the private property without worrying about django, and just remove it in a few years :joy:

clokep commented 4 years ago

This is fixed in 0.13 of django-querysetsequence.