[Django] #20932: Issues with model Manager and inheritance.
Django
----------------------------------------------+--------------------
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
I've found a couple of issues with the current handling of managers with
inheritance.
1. Weird bug that I can't explain yet:
https://github.com/loic/django/compare/manager_inheritance_bug
`model_inheritance_regress.tests.ModelInheritanceTest.test_abstract_base_class_m2m_relation_inheritance`
fails with `AttributeError: 'BachelorParty' object has no attribute
'bachelorparty_ptr'`.
2. Managers in MTI *are* inherited, contrary to what is said
[https://docs.djangoproject.com/en/dev/topics/db/managers/#custom-
managers-and-model-inheritance in the docs].
Quoting the docs: "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.
Note: I know we usually should have one ticket for each bug. This time
though, we only have a couple of lines of codes that have multiple
consequences, so I'd like to keep the discussion centralized. We can
always open a ticket for each bug when the time comes to actually
implement what we decide.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 1
* needs_better_patch: => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:1>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:2>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by LaurensBER):
I've run into issue two and if possible I suggest that we add a line at
the documentation explaining this unexpected behaviour.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:3>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by LaurensBER):
* cc: LaurensBER (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:4>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by ar45):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:5>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by ar45):
* cc: aronp@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:6>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Old description:
New description:
I've found a couple of issues with the current handling of managers with
inheritance.
1. Given:
{{{#!python
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
[https://docs.djangoproject.com/en/dev/topics/db/managers/#custom-
managers-and-model-inheritance 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.
--
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:7>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by ar45):
Replying to [comment:7 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()`
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:8>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by loic):
Replying to [comment:8 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
[https://github.com/django/django/blob/10a162809fa4de3a56bb7f2d3cb12b6b82a6f826/django/db/models/manager.py#L21
this condition].
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:9>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by loic):
OK this boils down to `AbstractEvent.events` being inherited by
`MessyBachelorParty` despite having `BachelorParty` in between, hence why
`AbstractEvent.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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:10>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by poleha):
I don't try to enforce anything in
[https://code.djangoproject.com/ticket/25897/ #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 [https://code.djangoproject.com/ticket/25897,
#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
[https://docs.djangoproject.com/en/dev/topics/db/managers/#custom-
managers-and-model-inheritance/ documentation], but I am not sure that it
worth breaking regular python mro.
Replying to [comment:10 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:11>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by loic):
Replying to [comment:11 poleha]:
> I don't try to enforce anything in
[https://code.djangoproject.com/ticket/25897/ #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 [comment:11 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
[https://docs.djangoproject.com/en/dev/topics/db/managers/#custom-
managers-and-model-inheritance/ 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:12>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by loic):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:13>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by mjtamlyn):
* cc: mjtamlyn (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:14>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by loic):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:15>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:16>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Changes (by loic):
* needs_docs: 0 => 1
Comment:
I had a go at Carl's idea and it seems to work pretty well:
https://github.com/django/django/pull/6157.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:17>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:18>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by ar45):
+1 Nice!
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:19>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by timgraham):
* keywords: => manager-inheritance
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:20>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
The [https://github.com/django/django/pull/6175 latest PR] LGTM.
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:21>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: manager-inheritance | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Loïc Bistuer <loic.bistuer@…>):
In [changeset:"ed0ff913c648b16c4471fc9a9441d1ee48cb5420" ed0ff91]:
{{{
#!CommitTicketReference repository=""
revision="ed0ff913c648b16c4471fc9a9441d1ee48cb5420"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:23>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: manager-inheritance | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Loïc Bistuer <loic.bistuer@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"3a47d42fa33012b2156bf04058d933df6b3082d2" 3a47d42]:
{{{
#!CommitTicketReference repository=""
revision="3a47d42fa33012b2156bf04058d933df6b3082d2"
Fixed #20932, #25897 -- Streamlined manager inheritance.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20932#comment:22>