Code

Opened 18 months ago

Closed 18 months ago

Last modified 16 months ago

#19173 closed Uncategorized (fixed)

"query" param for Model.objects.none() queryset has unexpected value.

Reported by: joshua.fialkoff@… Owned by: nobody
Component: Uncategorized Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

  1. Create Django model:
class Location(models.Model):
  name = models.CharField(max_length=100)
  1. Get a null set
res = Location.objects.none()
  1. Check the SQL relating to this set:
print(res.query.sql_with_params())
>> ('SELECT "name" FROM "location", ())

The SQL above would actually return all the records in the database. My expectation is that I'd get the same result here as when I do the following:

query = Location.objects.filter(id__in=[])
print(query.sql_with_params())

This, in fact, throws an EmptyResultSet error which seems reasonable as there's really no SQL statement to go along with a purposefully empty set.

Attachments (0)

Change History (5)

comment:1 Changed 18 months ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

.query isn't a public API, so I'm not sure we care.

comment:2 Changed 18 months ago by akaariai

We really should get rid of the whole none() -> EmptyQuerySet. We would get 150 lines worth of DRY code removal. The code would be more robust (EmptyQS has had its problems, see the altered tests...). Still one more bonus point is that a query which has had .none() applied works exactly like normal queries (for example qs.none().filter(nonexistingfield=someval) would raise exception correctly.

The cost of this is speed penalty from QuerySet cloning. The fix for this is making cloning cheaper.

The biggest problem might be that .none() documentation explicitly mentions EmptyQuerySet. There is no reason the docs need to mention EQS. Seems like leaking implementation detail to me. My opinion is to not make a backwards compatibility issue out of this. Just noting that EQS has been removed should be enough IMO.

Implementation available here: https://github.com/akaariai/django/commit/915cf9e495f033a07feb6c9aaece44d735d7fe70

comment:3 Changed 18 months ago by akaariai

I created another ticket for emptyqs removal - see #19184.

I am going to close this ticket. As Alex said qs.query isn't part of public API, so we don't have to guarantee it works like in qs.filter(pk__in=[]). If we want to fix this IMO the proper fix is to just remove the emptyqs altogether which ensures qs.none() will work like qs.filter(pk__in=[]) works in every case - not just str(qs.query) case.

comment:4 Changed 18 months ago by aaugustin

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

Closing the ticket per Anssi's comment.

comment:5 Changed 16 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution changed from wontfix to fixed

In a2396a4c8f2ccd7f91adee6d8c2e9c31f13f0e3f:

Fixed #19173 -- Made EmptyQuerySet a marker class only

The guarantee that no queries will be made when accessing results is
done by new EmptyWhere class which is used for query.where and having.

Thanks to Simon Charette for reviewing and valuable suggestions.

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.