Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#7154 closed (fixed)

Not all managers gets copied from abstract models.

Reported by: sebastian_noack Owned by: mtredinnick
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)

Change History (39)

comment:1 Changed 7 years ago by sebastian_noack

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to mtredinnick
  • Patch needs improvement unset

comment:3 Changed 7 years ago by sebastian_noack

  • Needs tests unset

comment:4 Changed 7 years ago by miracle2k

  • Cc elsdoerfer@… added
  • Owner changed from mtredinnick to miracle2k
  • Status changed from new to assigned

Patch works for me.

comment:5 Changed 7 years ago by miracle2k

  • Owner changed from miracle2k to mtredinnick
  • Status changed from assigned to new

comment:6 follow-up: Changed 7 years ago by emulbreh

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 7 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 7 years ago by emulbreh

Replying to sebastian_noack:

Order does not matter here, ...

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

comment:9 Changed 7 years ago by anonymous

  • Keywords qsrf-cleanup added
  • milestone set to 1.0

comment:10 follow-up: Changed 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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".

Changed 7 years ago by emulbreh

comment:11 in reply to: ↑ 10 Changed 7 years ago by emulbreh

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 7 years ago by emulbreh

comment:12 follow-up: Changed 7 years ago by emulbreh

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to 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 in reply to: ↑ 13 Changed 7 years ago by emulbreh

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 7 years ago by mtredinnick

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 7 years ago by emulbreh

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 7 years ago by emulbreh

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

comment:18 Changed 7 years ago by tie

Another referce: #7505

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

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 7 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 7 years ago by mtredinnick

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 7 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 7 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 7 years ago by emulbreh

comment:25 Changed 7 years ago by emulbreh

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

comment:26 Changed 7 years ago by mtredinnick

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 7 years ago by dnordberg

Has the merge status changed?

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

  • Cc post@… added

comment:29 Changed 7 years ago by jacob

  • Keywords 1.0-blocker added

comment:30 Changed 7 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 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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