Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#25897 closed Bug (fixed)

Managers defined on non-abstract base classes are in fact inherited by child classes.

Reported by: Alex Poleha Owned by: Alex Poleha
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords: manager-inheritance
Cc: aronp@…, Loic Bistuer Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Change History (13)

comment:1 Changed 3 years ago by Sergey Fedoseev

Triage Stage: UnreviewedAccepted
Version: master1.8

steps to reproduce:

class TestManager(models.Manager):
    pass

class Test(models.Model):
    objects = TestManager()
    other_objects = TestManager()

class TestInherited(Test):
    pass
In [2]: TestInherited.objects
Out[2]: <django.db.models.manager.Manager at 0x7fd8e8ae2550>

In [3]: TestInherited.other_objects
Out[3]: <geotest.models.TestManager at 0x7fd8e8a74390>

comment:3 Changed 3 years ago by Sergey Fedoseev

Has patch: set

comment:4 Changed 3 years ago by Tim Graham

Is there any indication this is a regression such that the fix should be backported or should we instead correct older versions of the documentation?

comment:5 in reply to:  4 Changed 3 years ago by Alex Poleha

Replying to timgraham:

Is there any indication this is a regression such that the fix should be backported or should we instead correct older versions of the documentation?

I found mentioning this:

Managers defined on non-abstract base classes are not inherited by child classes

in documentation since version 1.4:

https://docs.djangoproject.com/en/1.4/topics/db/managers/#custom-managers-and-model-inheritance

And this bug exists in all versions since 1.4.

I think that fixing this bug in current version without fixing the documentation would be enough. But I am ready to provide path for all versions since 1.4. And I don't think that fixing old versions of documentation would be correct in this case.

comment:6 Changed 2 years ago by Tim Graham

Looking at the original commit where the documentation was added, f31425e8e23621fd4329c7377c9e220f526a1c49, it looks like named managers were always inherited. What does seem to work is that the default manager is never inherited.

The test case in the commit looks like this:

class Parent(models.Model):
    manager = OnlyFred()

# Will not inherit default manager from parent.
class Child7(Parent):
    pass

>>> Parent._default_manager.all()
[<Parent: fred>]

>>> Child7._default_manager.order_by('name')
[<Child7: barney>, <Child7: fred>]

>>> Child7.manager.all()
[<Parent: fred>]

>>> Child7._default_manager.order_by('name')
[<Child7: barney>, <Child7: fred>]

I wonder if the behavior should actually be changed (which will cause backwards incompatibility for anyone relying on it) or if the documentation should be fixed? Maybe we should ask on the DevelopersMailingList.

comment:7 Changed 2 years ago by Tim Graham

Patch needs improvement: set

There hasn't been any immediate feedback on the mailing list except for me suggesting to put this through the deprecation cycle. The patch is updated for that but needs documentation as noted on the pull request.

comment:8 Changed 2 years ago by Aron Podrigal

Cc: aronp@… added

Related to /Fixes #20932

comment:9 Changed 2 years ago by Loic Bistuer

Cc: Loic Bistuer added

comment:10 Changed 2 years ago by Tim Graham

Keywords: manager-inheritance added; manager inheritance removed

comment:11 Changed 2 years ago by Loic Bistuer

PR6175 revamps manager inheritance and inheriting managers from non-abstract base classes is the new expected behavior.

comment:12 Changed 2 years ago by Loïc Bistuer <loic.bistuer@…>

Resolution: fixed
Status: newclosed

In 3a47d42:

Fixed #20932, #25897 -- Streamlined manager inheritance.

comment:13 Changed 2 years ago by Loïc Bistuer <loic.bistuer@…>

In ed0ff91:

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.

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