Opened 8 years ago

Closed 8 years ago

Last modified 8 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

Description

Change History (13)

comment:1 by Sergey Fedoseev, 8 years ago

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 by Sergey Fedoseev, 8 years ago

Has patch: set

comment:4 by Tim Graham, 8 years ago

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?

in reply to:  4 comment:5 by Alex Poleha, 8 years ago

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 by Tim Graham, 8 years ago

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 by Tim Graham, 8 years ago

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

Cc: aronp@… added

Related to /Fixes #20932

comment:9 by Loic Bistuer, 8 years ago

Cc: Loic Bistuer added

comment:10 by Tim Graham, 8 years ago

Keywords: manager-inheritance added; manager inheritance removed

comment:11 by Loic Bistuer, 8 years ago

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

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

Resolution: fixed
Status: newclosed

In 3a47d42:

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

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

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