Opened 11 years ago

Last modified 8 years ago

#20932 closed Bug

Issues with model Manager and inheritance. — at Version 7

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 Loic Bistuer)

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.

  1. 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 (7)

comment:1 by loic84, 11 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

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.

comment:2 by Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

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 LaurensBER, 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 LaurensBER, 9 years ago

Cc: LaurensBER added

comment:5 by Aron Podrigal, 8 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 Aron Podrigal, 8 years ago

Cc: aronp@… added

comment:7 by Loic Bistuer, 8 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.

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