Not all managers gets copied from abstract models.

[Sebastian Noack]

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.

[Sebastian Noack]

See ​http://groups.google.com/group/django-developers/browse_thread/thread/7f0459b0774f9770

[miracle2k]

Patch works for me.

[Johannes Dollinger]

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))

[Sebastian Noack]

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 an Options.add_manager() method (analogous to Options.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.

[Johannes Dollinger]

Replying to sebastian_noack:

Order does not matter here, ...

I should have had a closer look, of course you're right.

[Malcolm Tredinnick]

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".

[Johannes Dollinger]

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.

[Johannes Dollinger]

It's been a week, the patch still applies, the tests still pass.

[Malcolm Tredinnick]

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).

[Johannes Dollinger]

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.

[Malcolm Tredinnick]

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.

[Johannes Dollinger]

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.

[Johannes Dollinger]

For reference: #7666, in the spirit of #7566 and #2698.

[tie]

Another referce: #7505

[anonymous]

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

[Johannes Dollinger]

I just svn upd 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?

[miracle2k]

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'>

[Malcolm Tredinnick]

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.

[miracle2k]

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'>

[miracle2k]

@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

[Johannes Dollinger]

It was exactly what Malcolm forsaw in his first comment. The removed remove-and-readd magic is back.

[Malcolm Tredinnick]

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.

[dnordberg]

Has the merge status changed?

[eibaan@…]

I also noticed #8170, interestingly enough, it seems to work in the admin UI just fine. What does it do differently?

[Malcolm Tredinnick]

(In [8851]) Fixed #7154 -- Inherit all model managers from abstract base classes.
Also added documentation describing how manager inheritance works (and when
manager aren't inherited). Based on some patches from sebastian_noack and
emulbreh.

[Jacob]

Milestone 1.0 deleted