Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

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

0001-Now-all-managers-not-only-the-default-manager-gets.patch (4.0 KB ) - added by Sebastian Noack 16 years ago.
0002-Now-all-managers-not-only-the-default-manager-gets.patch (4.6 KB ) - added by Sebastian Noack 16 years ago.
0003-Now-all-managers-not-only-the-default-manager-gets.patch (6.6 KB ) - added by Sebastian Noack 16 years ago.
0004-Now-all-managers-not-only-the-default-manager-gets.patch (9.9 KB ) - added by Sebastian Noack 16 years ago.
7154.copy-managers.r7787.diff (9.6 KB ) - added by Johannes Dollinger 16 years ago.
7154.copy-managers.r7818.diff (9.6 KB ) - added by Johannes Dollinger 16 years ago.
7154.copy-managers.r7884.diff (9.3 KB ) - added by Johannes Dollinger 16 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 by Sebastian Noack, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick

comment:3 by Sebastian Noack, 16 years ago

Needs tests: unset

comment:4 by miracle2k, 16 years ago

Cc: elsdoerfer@… added
Owner: changed from Malcolm Tredinnick to miracle2k
Status: newassigned

Patch works for me.

comment:5 by miracle2k, 16 years ago

Owner: changed from miracle2k to Malcolm Tredinnick
Status: assignednew

comment:6 by Johannes Dollinger, 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))

in reply to:  6 comment:7 by Sebastian Noack, 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 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.

comment:8 by Johannes Dollinger, 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 anonymous, 16 years ago

Keywords: qsrf-cleanup added
milestone: 1.0

comment:10 by Malcolm Tredinnick, 16 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 Johannes Dollinger, 16 years ago

in reply to:  10 comment:11 by Johannes Dollinger, 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 Johannes Dollinger, 16 years ago

comment:12 by Johannes Dollinger, 16 years ago

Triage Stage: AcceptedReady for checkin

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

in reply to:  12 ; comment:13 by Malcolm Tredinnick, 16 years ago

Triage Stage: Ready for checkinAccepted

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

in reply to:  13 comment:14 by Johannes Dollinger, 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 Malcolm Tredinnick, 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 Johannes Dollinger, 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:17 by Johannes Dollinger, 16 years ago

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

comment:18 by tie, 16 years ago

Another referce: #7505

comment:19 by anonymous, 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 Johannes Dollinger, 16 years ago

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?

comment:21 by miracle2k, 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 Malcolm Tredinnick, 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 miracle2k, 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 miracle2k, 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 Johannes Dollinger, 16 years ago

comment:25 by Johannes Dollinger, 16 years ago

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

comment:26 by Malcolm Tredinnick, 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:27 by dnordberg, 16 years ago

Has the merge status changed?

comment:28 by Bela Hausmann <post@…>, 16 years ago

Cc: post@… added

comment:29 by Jacob, 16 years ago

Keywords: 1.0-blocker added

comment:30 by eibaan@…, 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 Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

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

comment:32 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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