Opened 11 years ago

Closed 11 years ago

#22817 closed Bug (invalid)

Missing custom methods on EmptyQuerySet

Reported by: benjaoming Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#19151 was basically reintroduced in 1.6 -- recap of the release notes:

The django.db.models.query.EmptyQuerySet can’t be instantiated any more - it is only usable as a marker class for checking if none() has been called: isinstance(qs.none(), EmptyQuerySet)

Thus, if we try the case of #19151 in 1.7:

>>> AnonymousUser().groups.value_list('name') 
Traceback (most recent call last):
  File "<input>", line 1, in <module>
AttributeError: 'EmptyManager' object has no attribute 'value_list'

I also have this issue in django-wiki and I don't know how to solve it: https://github.com/benjaoming/django-wiki/issues/216 (sorry it doesn't contain many details yet)

The problem with the above is that basically when we have a custom manager and we want to call a custom function, we have to always check that we do not have an instance of EmptyQuerySet. I mean, how else can we call a custom method on a custom QuerySet if it's always subject to be an instance of a non-custom EmptyQuerySet ? :)

I'm seeing behaviour as broken and should be reverted, but please correct me if I'm wrong and there's a way around this.

I've heard that 1.7 will have a QuerySet.as_manager utility method, but this doesn't fix heaps of broken managers that rely on their own instances of EmptyQuerySet and cannot simply stop instantiating them because they use none() to transparently implement the same call pattern for both empty and non-empty querysets.

In addition, maintaining compatibility of managers across 1.5 - 1.7 seems very painful without custom EmptyQuerySet instances.

Change History (11)

comment:1 by benjaoming, 11 years ago

See also #19184 for the reasoning behind removing EmptyQuerySet.

Can you perhaps imagine something like this:

class QuerySet():
    def __init__(self, is_empty=False):
         self.is_empty = is_empty

    # Same for exclude and other core functions...
    def filter(self, **kwargs):
         # Do not hit database when empty!
         if self.is_empty:
             return self

    def none(self):
         self.is_empty = True
         return self

class Manager():
    ...
    def get_empty_queryset(self):
        return QuerySet(empty=True)
    ...
    def none(self):
        return QuerySet(empty=True)

# Backwards compatible...
class EmptyQuerySet():
    def __init__(self, ...):
        return QuerySet(empty=True)

That way, EmptyQuerySet can continue to exist and still be deprecated?

comment:2 by Anssi Kääriäinen, 11 years ago

I'm probably missing some piece here - why doesn't using .none() work in 1.7? This should return a QuerySet that doesn't hit the database when executed.

I don't see any big problems in allowing creation of EmptyQuerySet classes for backward compatibility, but I'd like to get a good picture of what is happening before doing that. (The example above doesn't work, first, __init__ can't return anything, second it would return direct QuerySet without any custom methods which by my understanding isn't the wanted behaviour)

in reply to:  2 comment:3 by benjaoming, 11 years ago

Replying to akaariai:

I'm probably missing some piece here - why doesn't using .none() work in 1.7? This should return a QuerySet that doesn't hit the database when executed.

I want to be able to do:

MyModel.objects.all().none().my_custom_manager_method()

Now, as I understand it, that would raise AttributeError because it goes through EmptyManager in Django 1.7 and 1.6, the case of doing AnonymousUser().groups.value_list('name') is similar.

(The example above doesn't work, first, __init__ can't return anything, second it would return direct QuerySet without any custom methods which by my understanding isn't the wanted behaviour)

Sorry, yes :) So maybe this would work...

# Backwards compatible...
class EmptyQuerySet(QuerySet):
    def __init__(self, *args, **kwargs):
        kwargs['is_empty'] = True
        QuerySet(*args, **kwargs)

It's just a conceptual picture, so if you can review the idea and indicate if you see it happening, I'd like to work on it to fix these issues that I otherwise don't know how to work around (for instance, I'm not managing MPTT which is affected and affects django-wiki back).

Another fix could be if EmptyManager magically proxied all methods of the non-empty Manager instance and just returned self.

comment:4 by Anssi Kääriäinen, 11 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

OK, the problem is that EmptyManager is subclass of Manager. Due to that, it doesn't have same methods as the real Manager. I'll set this as release blocker.

One solution is to create dynamically a subclass of the real manager for EmptyManager. I don't like how much dynamic class creation we have now for querysets and managers, I have a feeling we will need to revisit this design again later on...

comment:5 by benjaoming, 11 years ago

Severity: Release blockerNormal
Triage Stage: AcceptedUnreviewed

Thanks. I will provide a running example, the AttributeError in the above is due to an embarrassing typo and not actually a missing method -- however, the point remains the same.

comment:6 by Tim Graham, 11 years ago

Component: UncategorizedDatabase layer (models, ORM)
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:7 by benjaoming, 11 years ago

I'm going to mark this as invalid very shortly because I cannot recreate the bug.

Django 1.7 style Manager...

from django.db import models
from django.db.models.query import QuerySet


class MyQuerySet(QuerySet):
    def custom_method(self):
        return self.filter(my_field=True)


class MyModel(models.Model):
    objects = MyQuerySet.as_manager()
    my_field = models.BooleanField(default=False)

Output:

>>> models.MyModel.objects.custom_method()
[]

>>> models.MyModel.objects.none().custom_method()
[]

This part works.

Also works with Django <=1.6 style managers:

from django.db import models
from django.db.models.query import QuerySet


class MyManager(models.Manager):
    def get_empty_query_set(self):
        return MyEmptyQuerySet(model=self.model)

    def get_query_set(self):
        return MyQuerySet(self.model, using=self._db)

    def custom_method(self):
        return self.filter(my_field=True)


class MyQuerySet(QuerySet):
    def custom_method(self):
        return self.filter(my_field=True)


class MyEmptyQuerySet(QuerySet):
    def custom_method(self):
        return self


class MyModel(models.Model):
    objects = MyManager()
    my_field = models.BooleanField(default=False)

Running on Django 1.7, the above code works:

>> from testapp import models
>>> models.MyModel.objects.none().custom_method()
[]
>>> models.MyModel.objects.custom_method()
[]
>>> models.MyModel.objects.all().none().custom_method()
[]

comment:8 by benjaoming, 11 years ago

Ah okay so this is the problem:

>>> from testapp import models
>>> from django.db.models.manager import EmptyManager
>>> emp=EmptyManager(models.MyModel)
>>> emp.all()
[]
>>> emp.custom_method()
Traceback (most recent call last):
  File "<input>", line 1, in <module>
AttributeError: 'EmptyManager' object has no attribute 'custom_method'

comment:9 by matt@…, 11 years ago

the only place I'm seeing that method called is AnonymousUser.

comment:10 by loic84, 11 years ago

I'm a little confused, we discussed on IRC having auth's EmptyManager inherit from the custom user's default manager. But the only place where EmptyManager is used is to simulate a M2M to Permission and Group; both of which cannot have their manager customized.

comment:11 by Marc Tamlyn, 11 years ago

Resolution: invalid
Status: newclosed

I agree with Loic's assessment. I would like to see an actionable failing test to allow this to move forwards.

This comment: https://code.djangoproject.com/ticket/22817#comment:8 is not in my mind a bug - just because a model has a custom manager with a custom method, if you create an EmptyManager for that model it should not work. For example this code clearly should behave as I've shown.

>>> from testapp import models
>>> from django.db.models.manager import Manager
>>> MyModel.objects.custom_method()
... [<some>, <qs>]
>>> manager=Manager(models.MyModel)
>>> manager.custom_method()
Traceback (most recent call last):
  File "<input>", line 1, in <module>
AttributeError: 'Manager' object has no attribute 'custom_method'

So EmptyManager should have the same issues.

I'm closing this as invalid for now. If there is a genuine bug here, please reopen or add a new report with a clearer description of the problem.

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