Issues with model Manager and inheritance.

[loic84]

I've found a couple of issues with the current handling of managers with inheritance.

1. Given:

class AbstractEvent(models.Model):
    events = models.Manager()
 
    class Meta:
        abstract = True

class BachelorParty(AbstractEvent):
    objects = models.Manager()

class MessyBachelorParty(BachelorParty):
    pass

If AbstractEvent has a manager called anything but objects (e.g. events):
MessyBachelorParty.objects.model == <class 'model_inheritance_regress.models.BachelorParty'>

Which causes the following failure:

model_inheritance_regress.tests.ModelInheritanceTest.test_abstract_base_class_m2m_relation_inheritance` fails with `AttributeError: 'BachelorParty' object has no attribute 'bachelorparty_ptr'.

If AbstractEvent doesn't have an explicit manager, or has one called objects:
MessyBachelorParty.objects.model == <class 'model_inheritance_regress.models.MessyBachelorParty'> which is the expected result.

2. Managers in MTI *are* inherited, contrary to what is said ​in the docs (now tracked specifically in #25897):

"Managers defined on non-abstract base classes are not inherited by child classes. [...] Therefore, they aren’t passed onto child classes.".

The problem is that, since we create the new class by calling object.__new__(), normal Python attribute resolution applies, and we gain access to the attributes of the base classes. This doesn't happen to fields because these are removed during the class creation process, but managers are left behind. It's always tempting to think we could just delete them, but you cannot delete something that is not in your own __dict__. The problem is not so much that we inherit those managers, but that they don't return the right model type as demonstrated in ​https://github.com/loic/django/compare/manager_inheritance_bug2.

[loic84]

I investigated a little and came up with that: ​https://github.com/loic/django/compare/manager_inheritance.

I basically copy all managers (abstract and concrete) from the concrete parents which fixes both issues.

Abstract managers from concrete parents were already copied for some reason (​https://github.com/django/django/commit/f31425e#L5R94).

I'm not too sure how this affects proxy models or how these should deal with manager inheritance in general, so I'll need to investigate some more, unless someone steps in with the answer.

Either way, the docs will need to be amended.

[Tim Graham]

Haven't reviewed this in detail, but marking as accepted to remove from unreviewed queue as it seems like there's at least a doc update required.

[LaurensBER]

I've run into issue two and if possible I suggest that we add a line at the documentation explaining this unexpected behaviour.

[Aron Podrigal]

This issues seems to be fixed by #25897 with PR ​https://github.com/django/django/pull/5797

Always Copying over managers may be a better solution as it would be simpler with backwards compatibility not having to go through the deprecation period as in #25897.

[Loic Bistuer]

Investigated the first issue a bit more and updated the ticket description with the findings. I don't think it's addressed by PR ​https://github.com/django/django/pull/5797.

Currently MessyBachelorParty.objects is the wrongly inherited manager from BachelorParty.

With the fix from the PR (after the deprecation period), MessyBachelorParty.objects would raise an AttributeError, but the expected behaviour would be that MessyBachelorParty gets an implicit objects = models.Manager() created automatically.

[Aron Podrigal]

Replying to loic:

Investigated the first issue a bit more and updated the ticket description with the findings. I don't think it's addressed by PR ​https://github.com/django/django/pull/5797.

Currently MessyBachelorParty.objects is the wrongly inherited manager from BachelorParty.

With the fix from the PR (after the deprecation period), MessyBachelorParty.objects would raise an AttributeError, but the expected behaviour would be that MessyBachelorParty gets an implicit objects = models.Manager() created automatically.

After the deprecation period, the class setup in ModelBase will not find any existing managers -- so new_class._prepare() which calls ensure_default_manager() will create an implicit manager. So MessyBachelorParty will get an implicit objects = models.Manager()

[Loic Bistuer]

Replying to ar45:

After the deprecation period, the class setup in ModelBase will not find any existing managers -- so new_class._prepare() which calls ensure_default_manager() will create an implicit manager. So MessyBachelorParty will get an implicit objects = models.Manager()

That doesn't match my observations, MessyBachelorParty.objects raises an attribute error when ManagerDescriptor.__get__() raises an AttributeError. MessyBachelorParty._default_manager is set (and yields the right model type), I haven't had time to investigate fully but I guess it trips ​this condition.

[Loic Bistuer]

OK this boils down to AbstractEvent.events being inherited by MessyBachelorParty despite having BachelorParty in between, hence why MessyBachelorParty.objects doesn't exist.

There is a design question here, should models inherit from managers of abstract models no matter what, or should we consider inheritance must stop when it hits a concrete parents.

Since Django inheritance mimics Python inheritance to a large degree I wonder more and more if we shouldn't always copy managers and revise the docs. #25897 tries to enforce the documented behaviour by masking managers at runtime, but that's fighting against standard Python inheritance (which doesn't support private members).

Can anyone see downsides to inheriting managers from concrete parents?

[Alex Poleha]

I don't try to enforce anything in #25897, I simply want to fix the existing approach, which doesn't work as expected. I think that the developer who coded the current approach was intended to achieve what would be achieved if #25897 would be applied.
Do you suggest removing all the code about contributing managers to models and simple use regular python inheritance?
I must confess I like it and I thought of it many times while examining the current approach. Python inheritance works well and it's predictable. There's some login behind current approach, which is described in ​documentation, but I am not sure that it worth breaking regular python mro.

Replying to loic:

OK this boils down to AbstractEvent.events being inherited by MessyBachelorParty despite having BachelorParty in between, hence why MessyBachelorParty.objects doesn't exist.

There is a design question here, should models inherit from managers of abstract models no matter what, or should we consider inheritance must stop when it hits a concrete parents.

Since Django inheritance mimics Python inheritance to a large degree I wonder more and more if we shouldn't always copy managers and revise the docs. #25897 tries to enforce the documented behaviour by masking managers at runtime, but that's fighting against standard Python inheritance (which doesn't support private members).

Can anyone see downsides to inheriting managers from concrete parents?

[Loic Bistuer]

Replying to poleha:

I don't try to enforce anything in #25897, I simply want to fix the existing approach, which doesn't work as expected.

Well there is no existing approach, just documentation that says one thing and code that does another, but by making the code do as the docs says we enforce the idea described in the docs.

Replying to poleha:

Do you suggest removing all the code about contributing managers to models and simple use regular python inheritance?

Well not exactly, we still need to "copy" the manager over so they return the correct model type. Currently we copy them for abstract and proxied parents but not concrete parents. This actually leads to the weird case where we also copy managers from the abstract parents of the concrete parents (which is the issue described in this ticket).

I must confess I like it and I thought of it many times while examining the current approach. Python inheritance works well and it's predictable. There's some login behind current approach, which is described in ​documentation, but I am not sure that it worth breaking regular python mro.

I like it too. But if we go that way, we need to decide what to do with the default manager creation logic, should we still create the objects manager and have it aliased to _default_manager if there are no other managers defined on the child model or any of its abstract parents?

If we don't create it, then we have a backwards incompatibility but with some (maybe lots of) efforts it should be possible to add a deprecation period. The upside is that it becomes easy to explain and reason about since the behavior would now be consistent across all 3 types of inheritances.

If we do create it, then we remain compatible at the expense of more convoluted logic when deciding whether or not to create the objects manager and more complicated rules in the documentation. We also make objects a special case, because if an inherited custom manager is called objects it will be overridden by a plain models.Manager() instance (and only of the child model doesn't have any other declared manager on itself or one of its abstract parents).

[Loic Bistuer]

Just to clarify the nature of the backwards incompatibility:

class BachelorParty(Model):
    events = CustomManager()

class MessyBachelorParty(BachelorParty):
    pass

MessyBachelorParty.objects wouldn't exist anymore, and MessyBachelorParty._default_manager would be an instance of CustomManager.

class BachelorParty(Model):
    objects = CustomManager()

class MessyBachelorParty(BachelorParty):
    pass

MessyBachelorParty.objects would become an instance of CustomManager instead of just models.Manager.

[Marc Tamlyn]

I think that backwards incompat is acceptable, and pretty natural. The end result here becomes easy to explain:

• Resolve managers explicitly defined on all models you inherit from (irrespective of their abstract or concrete nature) using normal MRO
• If there are no managers explicitly defined, create objects = models.Manager() as every model must have a manager.

The guiding point being the implicit creation is a last resort.

Because the existing behaviour is strange and incorrectly documented, there's no harm in rewriting it to make it logical. It should be easy to make loud warnings that MessyBachelorParty.objects is not defined as MessyBachelorParty.events is.

---

The other alternative approach, which I have to say I quite like but would be more far reaching, is to effectively define objects on Model and let normal MRO do the rest (subject to model binding). This would mean every model has a manager called objects, which is either the default manager or a defined custom manager. We scrap _default_manager and just use objects as the default. This would be a much bigger backwards incompatibility, but in my experience whenever I do define a custom manager, I either call it objects, or call it something else and redefine objects. I find the disappearing nature of objects somewhat strange and magical. Such a change would need to be discussed on the mailing list, what I'm curious about is whether anyone actually has a concrete need for objects to disappear.

[Loic Bistuer]

I really like the idea of having Model.objects = models.Manager() and let the MRO do its thing. By itself, it wouldn't be that far reaching (I don't think anyone would have reasons to check for its absence)

The problematic bit would be getting rid of _default_manager because people that added an explicit custom manager named anything but objects would now see their default manager downgraded to the inherited models.Manager.

Unless we apply the same magic logic that currently overrides _default_manager to override an inherited objects, but then we can't say that normal MRO resolution applies.

[Carl Meyer]

I like the proposal loic84 outlined (and mjtamlyn further explained as option 1); given the longstanding discrepancy between the docs and the actual behavior, I think that's a reasonable level of backwards-incompatibility (if well documented) to achieve more predictable behavior.

I think eliminating _default_manager is a much bigger backwards-incompatibility for the reason loic84 mentioned, and I don't see that it really gains us much. To me "normal inheritance plus default objects added if no other manager" is fine.

The other aspect here is that as long as we have to re-instantiate managers at every inheritance level in order to re-bind them to the correct model class, none of this can really fairly be described as "normal MRO" because it's a different instance at every level, which is certainly not how attribute resolution via the MRO normally behaves. I discussed briefly with loic84 in IRC the option of having managers actually assign a descriptor to the class that would bind a manager instance to the correct model class on access, to avoid this need for re-binding in advance at every inheritance level. But that's definitely a bigger change and would need exploration to see if it's feasible.

[Loic Bistuer]

I had a go at Carl's idea and it seems to work pretty well: ​https://github.com/django/django/pull/6157.

[Carl Meyer]

Wow, neat! That really cleans things up, and isn't nearly as invasive as I thought it might be. Forgot that managers already assign a descriptor, so it's just moving some behavior around, it's not a fundamental change to how managers are accessed.

[Aron Podrigal]

+1 Nice!

[Tim Graham]

The ​latest PR LGTM.

[Loïc Bistuer <loic.bistuer@…>]

In 3a47d42:

Fixed #20932, #25897 -- Streamlined manager inheritance.

[Loïc Bistuer <loic.bistuer@…>]

In ed0ff91:

Fixed #10506, #13793, #14891, #25201 -- Introduced new APIs to specify models' default and base managers.

This deprecates use_for_related_fields.

Old API:

class CustomManager(models.Model):

use_for_related_fields = True

class Model(models.Model):

custom_manager = CustomManager()

New API:

class Model(models.Model):

custom_manager = CustomManager()

class Meta:

base_manager_name = 'custom_manager'

Refs #20932, #25897.

Thanks Carl Meyer for the guidance throughout this work.
Thanks Tim Graham for writing the docs.