#7154 closed (fixed)
Not all managers gets copied from abstract models.
Reported by: | Sebastian Noack | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Keywords: | qsrf-cleanup 1.0-blocker | |
Cc: | elsdoerfer@…, post@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
When deriving from an abstract Model, only the default manager gets copied. There is no reason why not all custom managers should become copied. I wrote a patch that does exactly this.
Attachments (7)
Change History (39)
by , 17 years ago
Attachment: | 0001-Now-all-managers-not-only-the-default-manager-gets.patch added |
---|
by , 17 years ago
Attachment: | 0002-Now-all-managers-not-only-the-default-manager-gets.patch added |
---|
comment:1 by , 17 years ago
Owner: | changed from | to
---|
comment:2 by , 17 years ago
Needs tests: | set |
---|
by , 17 years ago
Attachment: | 0003-Now-all-managers-not-only-the-default-manager-gets.patch added |
---|
comment:3 by , 17 years ago
Needs tests: | unset |
---|
comment:4 by , 17 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Patch works for me.
comment:5 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
by , 16 years ago
Attachment: | 0004-Now-all-managers-not-only-the-default-manager-gets.patch added |
---|
follow-up: 7 comment:6 by , 16 years ago
Patch works for me, although Options.base_managers
should probably be a SortedDict - order does matter here.
Style questions:
I'd encapsulate assignment to Options.base_managers
in an Options.add_manager()
method (analogous to Options.add_field()
).
Why is base_managers not called managers?
In base.py:
if mgr_attr in new_class.__dict__: continue new_class.add_to_class(mgr_attr, copy.copy(mgr)) # equivalent to: if not mgr_attr in new_class.__dict__: new_class.add_to_class(mgr_attr, copy.copy(mgr))
comment:7 by , 16 years ago
Patch works for me, although
Options.base_managers
should probably be a SortedDict - order does matter here.
It was a SortedDict but I replaced it with a default dict, in the latest patch. Order does not matter here, otherwise the model_inheritance tests, wouldn't pass. The metaclass gets the attrs already as a dict, so we don't need a SortedDict to keep a arbitrary order. To determine the order of defined managers a creation counter is used. This is the only way you can do this.
I'd encapsulate assignment to
Options.base_managers
in anOptions.add_manager()
method (analogous toOptions.add_field()
).
I don't think this is required, because of in contrast to adding fields (if you look in to add_field()'s definition), adding managers does not require any special code. Just add a manager to the dict, that's it.
Why is base_managers not called managers?
It is called !base_managers, because of it makes managers from base classes available in derived classes. But I don't care much about whether it is called !base_managers or just !managers.
In base.py:
if mgr_attr in new_class.__dict__: continue new_class.add_to_class(mgr_attr, copy.copy(mgr)) # equivalent to: if not mgr_attr in new_class.__dict__: new_class.add_to_class(mgr_attr, copy.copy(mgr))
This is just a matter of style. I think using continue and early returns are just more readable, than indented code.
comment:8 by , 16 years ago
Replying to sebastian_noack:
Order does not matter here, ...
I should have had a closer look, of course you're right.
comment:9 by , 16 years ago
Keywords: | qsrf-cleanup added |
---|---|
milestone: | → 1.0 |
follow-up: 11 comment:10 by , 16 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
This looks reasonable. However, some of the code you've removed was handling the special case of removing the automatically added default manager if a manager was explicitly specified on the derived class (since the explicitly specified manager would thus be the "first named" manager and hence the default). You're not testing this case, since you've added explicit managers to every model now in the tests. So please include a test that shows that if the abstract base class has no managers specified at all and there is a manager on a child class, the first manager of the child becomes the default.
Also, as a stylistic issue, you can save a level of indentation in managers.contribute_to_class()
by just returning if model._meta.abstract == True
. A manager on an abstract base class makes no sense (since an ABC is never instantiated in a way that using the manager makes sense), so you can just bail out in that case. It makes the code a little easier to read.
After those changes, feel free to promote to "ready for checkin".
by , 16 years ago
Attachment: | 7154.copy-managers.r7787.diff added |
---|
comment:11 by , 16 years ago
Replying to mtredinnick:
Done.
There are still issues with model inheritance and _defaul_manager resolution, but those shouldn't block this fix for the obvious problems:
class AManager(Manager): pass class BManager(Manager): pass class A(Model): a_objects = AManager() class Meta: abstract = True class B(Model): b_objects = BManager() class Meta: abstract = True class C(A,B): objects = Manager() class D(B,A): objects = Manager()
Now C._default_manager == C.a_objects and D._default_manager ==
D.a_objects.
If A and B come from different modules _default_manager will be
picked depending on import order.
And you cannot use a different default manager in subclasses.
The whole creation_counter magic is ugly.
by , 16 years ago
Attachment: | 7154.copy-managers.r7818.diff added |
---|
follow-up: 13 comment:12 by , 16 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
It's been a week, the patch still applies, the tests still pass.
follow-up: 14 comment:13 by , 16 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Replying to emulbreh:
It's been a week, the patch still applies, the tests still pass.
None of which are the only pre-conditions for moving to "ready for checkin". Please don't triage your own patches like this, since it skips a review step. Particularly on something tricky like this.
I'm not completely happy with the patch, since I suspect it might be missing some cases and I want to look at the case of using the right default manager in the multi-inheritance case. So I've read it through a couple of times and am thinking about it (which is why it's assigned to me).
comment:14 by , 16 years ago
Replying to mtredinnick:
Replying to emulbreh:
It's been a week, the patch still applies, the tests still pass.
None of which are the only pre-conditions for moving to "ready for checkin". Please don't triage your own patches like this, since it skips a review step. Particularly on something tricky like this.
I'm sorry, seems like I misunderstood 'After those changes, feel free to promote to "ready for checkin".'.
I'm not completely happy with the patch, since I suspect it might be missing some cases and I want to look at the case of using the right default manager in the multi-inheritance case. So I've read it through a couple of times and am thinking about it (which is why it's assigned to me).
I recently tried to discuss this: http://groups.google.at/group/django-developers/browse_thread/thread/3e1a668ac84328b6/ce36cbe46276d807
That was probably the wrong way to bring it up.
However, there are at least two tickets related to problems with the _default_manager
concept: #7566 and #2698.
comment:15 by , 16 years ago
Sorry for snapping in the earlier comment. I had forgotten that I encouraged you to move it and that you cannot read my mind or know that I'd semi-reviewed the patch. Completely my fault. I apologise.
My current intention is to fix this stuff "properly" in one go, rather than moving it from one broken state to another broken state. Your patch is certainly the right starting point and goes a long way to identifying the remaining issue (the creation counter), which I'm thinking about how to use appropriately (creation counter is actually a very useful concept in general, it just gets in the way slightly in this case).
I think the two tickets you pointed out aren't really related to this, since they're based on people hoping for different behaviour from the default manager than what actually happens. Default manager == default behaviour. So I'm not going to worry about those tickets yet.
comment:16 by , 16 years ago
Letting the order of class attributes carry semantics is not intuitive (that's not normal for python declarations). Given that if you have no legacy data you want too hide from django you most probably want a vanilla default manager, couldn't the default manager be assigned explicitly and default to a plain Manager
instance?
class Foo(Model): foo_objects = FooManager() class Meta: default_manager = 'foo_objects' class Bar(Model): bar_objects = BarManager()
Now, Foo._default_manager == Foo.foo_objects
but Bar._default_manager != Bar.bar_objects
.
I agree #7566 and #2698 should be wontfix. The problems seems to be that the docs advertise managers as a method to create views of a model - so it somehow makes sense to consider the default manager a default view rather than a restriction of what django can do with the db.
comment:19 by , 16 years ago
7154.copy-managers.r7818.diff seems to break non-abstract inheritance:
class BaseModel(models.Model): pass class Subclass(BaseModel): pass Subclass._default_manager.model == BaseModel
This also breaks some core functionality like:
basemodel_instance.subclass
comment:20 by , 16 years ago
I just svn up
d to r7884, applied 7154.copy-managers.r7818.diff (not clean but successful) and ran:
>>> from django.db.models import * >>> class Base(Model): ... class Meta: ... app_label='' ... >>> class Sub(Base): ... class Meta: ... app_label='' ... >>> Sub._default_manager.model <class 'Sub'>
For me modeltests/model_inheritance passes, which has a test for base.sub
access.
Details?
comment:21 by , 16 years ago
Sorry, I should have checked better. Try this:
from django.db import models class BaseItem(models.Model): class Meta: app_label='' class GameManager(models.Manager): pass class Game(BaseItem): objects = GameManager() class Meta: app_label=''
>>> Game._default_manager.model <class '__main__.BaseItem'>
comment:22 by , 16 years ago
These examples aren't valid. Setting app_label
to the empty string doesn't make sense, so if the problem goes away when app_label
is set correctly, then it's not a problem at all.
comment:23 by , 16 years ago
Forgot to mention - without the patch, I get:
In [5]: Game._default_manager Out[5]: <__main__.GameManager object at 0x0268CC30> In [6]: Game._default_manager.model Out[6]: <class '__main__.Game'>
comment:24 by , 16 years ago
@malcolm: They don't - I actually wasn't smart enough to it in the shell, so I setup a demo project first and had everything in models.py
by , 16 years ago
Attachment: | 7154.copy-managers.r7884.diff added |
---|
comment:25 by , 16 years ago
It was exactly what Malcolm forsaw in his first comment. The removed remove-and-readd magic is back.
comment:26 by , 16 years ago
Please don't worry about updating this patch any further. I've started merging it with trunk and I'm doing it in a bit of a different fashion that's closer to the existing trunk code. My version is already fairly shuffled around from these patches, so new versions won't get incorporated. Just hang tight for a bit and something will land in the next couple of days.
comment:28 by , 16 years ago
Cc: | added |
---|
comment:29 by , 16 years ago
Keywords: | 1.0-blocker added |
---|
comment:30 by , 16 years ago
I also noticed #8170, interestingly enough, it seems to work in the admin UI just fine. What does it do differently?
comment:31 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
See http://groups.google.com/group/django-developers/browse_thread/thread/7f0459b0774f9770