Code

Opened 2 years ago

Closed 18 months ago

#17712 closed Bug (fixed)

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

Reported by: ventur40 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 akaariai 2 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Confirmed in 1.4b1 also.

comment:2 Changed 2 years ago by charettes

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 follow-up: Changed 2 years ago by akaariai

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 2 years ago by carljm

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 2 years ago by akaariai

comment:5 Changed 2 years ago by akaariai

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 2 years ago by cdestigter

  • 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 20 months 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 18 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.