#20932 closed Bug (fixed)
Issues with model Manager and inheritance.
Reported by: | loic84 | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | manager-inheritance |
Cc: | Anssi Kääriäinen, LaurensBER, aronp@…, Marc Tamlyn | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I've found a couple of issues with the current handling of managers with inheritance.
- 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.
- 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.
Change History (23)
comment:1 by , 11 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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.
comment:3 by , 9 years ago
I've run into issue two and if possible I suggest that we add a line at the documentation explaining this unexpected behaviour.
comment:4 by , 9 years ago
Cc: | added |
---|
comment:5 by , 9 years ago
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.
comment:6 by , 9 years ago
Cc: | added |
---|
follow-up: 8 comment:7 by , 9 years ago
Description: | modified (diff) |
---|
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.
follow-up: 9 comment:8 by , 9 years ago
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 fromBachelorParty
.
With the fix from the PR (after the deprecation period),
MessyBachelorParty.objects
would raise anAttributeError
, but the expected behaviour would be thatMessyBachelorParty
gets an implicitobjects = 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()
comment:9 by , 9 years ago
Replying to ar45:
After the deprecation period, the class setup in
ModelBase
will not find any existing managers -- sonew_class._prepare()
which callsensure_default_manager()
will create an implicit manager. SoMessyBachelorParty
will get an implicitobjects = 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.
follow-up: 11 comment:10 by , 9 years ago
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?
follow-up: 12 comment:11 by , 9 years ago
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 byMessyBachelorParty
despite havingBachelorParty
in between, hence whyMessyBachelorParty.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?
comment:12 by , 9 years ago
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 if the child model doesn't have any other declared managers on itself or one of its abstract parents).
comment:13 by , 9 years ago
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
.
comment:14 by , 9 years ago
Cc: | added |
---|
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.
comment:15 by , 9 years ago
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.
comment:16 by , 9 years ago
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.
comment:17 by , 9 years ago
Needs documentation: | set |
---|
I had a go at Carl's idea and it seems to work pretty well: https://github.com/django/django/pull/6157.
comment:18 by , 9 years ago
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.
comment:20 by , 8 years ago
Keywords: | manager-inheritance added |
---|
comment:21 by , 8 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
The latest PR LGTM.
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.