Opened 10 years ago
Closed 10 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 , 10 years ago
follow-up: 3 comment:2 by , 10 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)
comment:3 by , 10 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 , 10 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
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 , 10 years ago
Severity: | Release blocker → Normal |
---|---|
Triage Stage: | Accepted → Unreviewed |
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 , 10 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Severity: | Normal → Release blocker |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:7 by , 10 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 , 10 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:10 by , 10 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 , 10 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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.
See also #19184 for the reasoning behind removing EmptyQuerySet.
Can you perhaps imagine something like this:
That way, EmptyQuerySet can continue to exist and still be deprecated?