Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#19653 closed Cleanup/optimization (fixed)

`Manager.get_empty_query_set` should call `get_query_set`

Reported by: Simon Charette Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Now that EmptyQuerySet has been removed calling none() on a QuerySet subclass returns an instance that subclass:

>>> class MyQuerySet(models.query.QuerySet): pass
>>> isinstance(MyQuerySet(MyModel).none(), MyQuerySet)
>>> True # On django >= 1.6

This is quite useful and less error prone.

However, when one subclass models.Manager to return a QuerySet subclass they must override both get_empty_query_set and get_query_set to have consistent behavior.

class MyManager(models.Manager):
    def get_query_set(self):
        return MyQuerySet(self.model, using=self._db)

class MyModel(models.Model):
    objects = MyManager()

>>> isinstance(MyModel.objects.none(), MyQuerySet) # Manager.none() calls get_empty_query_set under the hood
>>> False
>>> isinstance(MyModel.objects.all().none(), MyQuerySet)
>>> True

Another alternative (better IMHO) is to completely remove get_empty_query_set since it's not documented and only used by Manager.none and EmptyManager.get_query_set. We could make Manager.none proxy QuerySet.none and EmptyManager.get_query_set returns QuerySet.get_query_self(self).none().

Attaching two patches that both pass all tests on Python 2.7.3 SQlite.

Attachments (2)

0001-Fixed-19653-Make-sure-get_empty_query_set-calls-get_.patch (1.0 KB) - added by Simon Charette 5 years ago.
0001-Fixed-19653-Removed-Manager.get_empty_query_set.patch (1.3 KB) - added by Simon Charette 5 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 5 years ago by Anssi Kääriäinen

Triage Stage: UnreviewedReady for checkin

Looks good to me. Marking as ready for checkin, I like the full removal approach more, it looks like it will not be needed any more.

comment:2 Changed 5 years ago by Anssi Kääriäinen

Anybody know of any real use cases get_empty_query_set()? If not, I will go with the remove get_empty_query_set() approach.

I will commit this in a day or two if nobody objects...

comment:3 Changed 5 years ago by Simon Charette

Looks like get_empty_query_set was introduced at the same time as EmptyQuerySet.

comment:4 Changed 5 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: newclosed

In fb606e10ac07af6014cae838c21bf30cf50e8b40:

Fixed #19653 -- Removed Manager.get_empty_query_set.

comment:5 Changed 5 years ago by Anssi Kääriäinen

Component: ORM aggregationDatabase layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top