Opened 5 years ago

Closed 4 years ago

#17712 closed Bug (fixed)

<model>.objects.none() not always empty

Reported by: David Ventura Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: none(), orm, queryset, emptyqueryset
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


Doing Table.objects.none().values_list('id').order_by('id')
An example:

In [27]: Location.objects.none().values_list('id').order_by('id')
Out[27]: [(1L,), (4L,), (6L,)]

Django 1.3.1

Attachments (1)

17712.diff (2.6 KB) - added by Anssi Kääriäinen 5 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by Claude Paroz

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Confirmed in 1.4b1 also.

comment:2 Changed 5 years ago by Simon Charette

Haven't had the time to check if it's really working but it might be related to #17681 which trigger the same issue but with annotate() .

comment:3 Changed 5 years ago by Anssi Kääriäinen

I don't believe .none().annotate() has this problem. I haven't tried but it is hard to see how that could happen with the current code. Of course, if you have value/values_list in between, then you have problems.

I think the best way forward would be to remove the EmptyQuerySet altogether. Add a flag to sql.query and a couple of checks in the different compiler/as_sql() methods or into the models.query.QuerySet.iterator(). Or just add NothingNode into the query.where. The latter is done in the patch attached to #17681. There aren't many needed changes, at least not in the NothingNode case.

The downside of either of the above is that the operations on an empty queryset are going to be somewhat slower (they actually do something). The upside is that this kind of bug should be less common, and that the queryset actually behaves exactly like the "real" queryset, which is not the case at all now. In addition you get to remove around 200 lines of code.

If you want to be technically correct when fixing this ticket and avoid the EmptyQuerySet removal you should add EmptyValuesQuerySet and EmptyValuesListQuerySet classes. I don't like that at all. You could cheat and do "return self" from values and values_list however.

I wonder if there are something which I am missing about EmptyQuerySet. Backward compatibility perhaps?

Should we deal with the EmptyQuerySet removal in a different ticket, or hijack this one?

comment:4 in reply to:  3 Changed 5 years ago by Carl Meyer

Replying to akaariai:

I wonder if there are something which I am missing about EmptyQuerySet. Backward compatibility perhaps?

The EmptyQuerySet class is not part of the public API, so I don't think there's a concern about removing it. I am hesitant to make this change after the beta, however. So I'm wondering if we could make the quick "return self" fix for 1.4 (seems like it should be fine) and then do the EmptyQuerySet removal after 1.4 is released.

Should we deal with the EmptyQuerySet removal in a different ticket, or hijack this one?

If we go with the above plan, separate tickets I think.

Changed 5 years ago by Anssi Kääriäinen

Attachment: 17712.diff added

comment:5 Changed 5 years ago by Anssi Kääriäinen

I think the attached patch should be ready for checkin. Not checking that for my own patch...

I will open another ticket for the EQS removal.

comment:6 Changed 5 years ago by Craig de Stigter

Has patch: set
Patch needs improvement: set

Looks okay, tests pass, one minor suggestion:

Method docstrings shouldn't really reference each other if possible, since that only makes sense when browsing the django source, and not when inspecting docstrings in ipython etc.

comment:7 Changed 4 years ago by orblivion@…

I created what is probably a dupe of this issue. However my symptom was slightly different. Just in case, here it is, perhaps you'll want to confirm this doesn't happen:

In : User.objects.filter(id__in = Profile.objects.none().values_list('user_id', flat = True))
Out: [<User: xxx>, <User: yyy>...]

Perhaps this, along with the one from #17681, should go into the test suite? I could even add them myself if there's a good branch for failing tests.

comment:8 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

In 69a46c5ca7d4e6819096af88cd8d51174efd46df:

Tests for various emptyqs tickets

The tickets are either about different signature between qs.none() and
qs or problems with subclass types (either EmptyQS overrided the custom
qs class, or EmptyQS was overridden by another class - values() did

Fixed #15959, fixed #17271, fixed #17712, fixed #19426

Note: See TracTickets for help on using tickets.
Back to Top