Opened 8 years ago

Closed 8 years ago

Last modified 5 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: master
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: UI/UX:

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 8 years ago.
0002-Now-all-managers-not-only-the-default-manager-gets.patch (4.6 KB) - added by Sebastian Noack 8 years ago.
0003-Now-all-managers-not-only-the-default-manager-gets.patch (6.6 KB) - added by Sebastian Noack 8 years ago.
0004-Now-all-managers-not-only-the-default-manager-gets.patch (9.9 KB) - added by Sebastian Noack 8 years ago.
7154.copy-managers.r7787.diff (9.6 KB) - added by Johannes Dollinger 8 years ago.
7154.copy-managers.r7818.diff (9.6 KB) - added by Johannes Dollinger 8 years ago.
7154.copy-managers.r7884.diff (9.3 KB) - added by Johannes Dollinger 8 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 8 years ago by Sebastian Noack

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Malcolm Tredinnick
Patch needs improvement: unset

comment:3 Changed 8 years ago by Sebastian Noack

Needs tests: unset

comment:4 Changed 8 years ago by miracle2k

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

Patch works for me.

comment:5 Changed 8 years ago by miracle2k

Owner: changed from miracle2k to Malcolm Tredinnick
Status: assignednew

comment:6 Changed 8 years ago by 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))

comment:7 in reply to:  6 Changed 8 years ago by 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.

comment:8 Changed 8 years ago by Johannes Dollinger

Replying to sebastian_noack:

Order does not matter here, ...

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

comment:9 Changed 8 years ago by anonymous

Keywords: qsrf-cleanup added
milestone: 1.0

comment:10 Changed 8 years ago by Malcolm Tredinnick

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

Changed 8 years ago by Johannes Dollinger

comment:11 in reply to:  10 Changed 8 years ago by 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.

Changed 8 years ago by Johannes Dollinger

comment:12 Changed 8 years ago by Johannes Dollinger

Triage Stage: AcceptedReady for checkin

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

comment:13 in reply to:  12 ; Changed 8 years ago by Malcolm Tredinnick

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

comment:14 in reply to:  13 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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.

comment:16 Changed 8 years ago by 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.

comment:17 Changed 8 years ago by Johannes Dollinger

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

comment:18 Changed 8 years ago by tie

Another referce: #7505

comment:19 Changed 8 years ago by 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

comment:20 Changed 8 years ago by 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?

comment:21 Changed 8 years ago by 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'>

comment:22 Changed 8 years ago by 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.

comment:23 Changed 8 years ago by 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'>

comment:24 Changed 8 years ago by 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

Changed 8 years ago by Johannes Dollinger

comment:25 Changed 8 years ago by Johannes Dollinger

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

comment:26 Changed 8 years ago by 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.

comment:27 Changed 8 years ago by dnordberg

Has the merge status changed?

comment:28 Changed 8 years ago by Bela Hausmann <post@…>

Cc: post@… added

comment:29 Changed 8 years ago by Jacob

Keywords: 1.0-blocker added

comment:30 Changed 8 years ago by eibaan@…

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

comment:31 Changed 8 years ago by Malcolm Tredinnick

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 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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