Opened 22 months ago

Last modified 21 months ago

#20932 new Bug

Issues with model Manager and inheritance.

Reported by: loic84 Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: akaariai Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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'.

  1. Managers in MTI *are* inherited, contrary to what is said 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.

Change History (2)

comment:1 Changed 22 months ago by loic84

  • Has patch set
  • Needs documentation unset
  • 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 Changed 21 months ago by timo

  • Triage Stage changed from Unreviewed to 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.

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