Changes between Initial Version and Version 7 of Ticket #20932


Ignore:
Timestamp:
Feb 10, 2016, 11:30:49 AM (8 years ago)
Author:
Loic Bistuer
Comment:

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.

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #20932

    • Property Has patch set
    • Property Patch needs improvement set
    • Property Needs tests set
    • Property Triage Stage Unreviewed → Accepted
    • Property Cc LaurensBER aronp@… added

  • Ticket #20932 – Description

    initial v7  
    11I've found a couple of issues with the current handling of managers with inheritance.
    22
    3 1. Weird bug that I can't explain yet: https://github.com/loic/django/compare/manager_inheritance_bug
     31. Given:
    44
    5  `model_inheritance_regress.tests.ModelInheritanceTest.test_abstract_base_class_m2m_relation_inheritance` fails with `AttributeError: 'BachelorParty' object has no attribute 'bachelorparty_ptr'`.
     5{{{#!python
     6class AbstractEvent(models.Model):
     7    events = models.Manager()
     8 
     9    class Meta:
     10        abstract = True
    611
    7 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].
     12class BachelorParty(AbstractEvent):
     13    objects = models.Manager()
    814
    9  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.
     15class MessyBachelorParty(BachelorParty):
     16    pass
     17}}}
    1018
    11 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.
     19If `AbstractEvent` has a manager called anything but `objects` (e.g. `events`):
     20`MessyBachelorParty.objects.model == <class 'model_inheritance_regress.models.BachelorParty'>`
     21
     22Which causes the following failure:
     23{{{
     24model_inheritance_regress.tests.ModelInheritanceTest.test_abstract_base_class_m2m_relation_inheritance` fails with `AttributeError: 'BachelorParty' object has no attribute 'bachelorparty_ptr'.
     25}}}
     26
     27If `AbstractEvent` doesn't have an explicit manager, or has one called `objects`:
     28`MessyBachelorParty.objects.model == <class 'model_inheritance_regress.models.MessyBachelorParty'>` which is the expected result.
     29
     302. 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):
     31
     32  "Managers defined on non-abstract base classes are not inherited by child classes. [...] Therefore, they aren’t passed onto child classes.".
     33
     34The 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.
Back to Top