[Django] #22817: Missing custom methods on EmptyQuerySet
Django
-------------------------------+--------------------
Reporter: benjaoming | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
#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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22817>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: benjaoming | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by benjaoming):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:1>
Django
Reporter: benjaoming | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:2>
Django
Reporter: benjaoming | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by benjaoming):
Replying to [comment:2 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:3>
Django
Reporter: benjaoming | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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...
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:4>
Django
Reporter: benjaoming | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by benjaoming):
* severity: Release blocker => Normal
* stage: Accepted => Unreviewed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:5>
Django
Reporter: benjaoming | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* type: Uncategorized => Bug
* component: Uncategorized => Database layer (models, ORM)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:6>
Django
Reporter: benjaoming | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by benjaoming):
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()
[]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:7>
Django
Reporter: benjaoming | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by benjaoming):
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'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:8>
Django
Reporter: benjaoming | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by matt@…):
the only place I'm seeing that method called is AnonymousUser.
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:9>
Django
Reporter: benjaoming | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:10>
Django
Reporter: benjaoming | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22817#comment:11>