Opened 3 years ago

Closed 3 years ago

Last modified 2 years 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.

Change History (5)

comment:1 Changed 3 years 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 3 years 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 3 years 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 3 years ago by aaugustin

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

Closing the ticket per Anssi's comment.

comment:5 Changed 2 years 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.

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