Opened 12 years ago

Closed 11 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

Description

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 12 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by Claude Paroz, 12 years ago

Triage Stage: UnreviewedAccepted

Confirmed in 1.4b1 also.

comment:2 by Simon Charette, 12 years ago

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 by Anssi Kääriäinen, 12 years ago

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?

in reply to:  3 comment:4 by Carl Meyer, 12 years ago

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.

by Anssi Kääriäinen, 12 years ago

Attachment: 17712.diff added

comment:5 by Anssi Kääriäinen, 12 years ago

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 by Craig de Stigter, 12 years ago

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 by orblivion@…, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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
this).

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

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