Opened 13 years ago
Closed 12 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)
Change History (9)
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 13 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()
.
follow-up: 4 comment:3 by , 13 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?
comment:4 by , 13 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 , 13 years ago
Attachment: | 17712.diff added |
---|
comment:5 by , 13 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 , 13 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 , 12 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 , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Confirmed in 1.4b1 also.