#25897 - managers defined on non-abstract base classes inherited by child classes

[Alex Poleha]

Hi. 

According to documentation managers defined on non-abstract base classes are not inherited by child classes. In fact they're inherited via python MRO. I made pull request to fix it. I find this inheritance embarrassing due to reasons, explained in documentation:

1. Managers defined on non-abstract base classes are not inherited by child classes. If you want to reuse a manager from a non-abstract base, redeclare it explicitly on the child class. These sorts of managers are likely to be fairly specific to the class they are defined on, so inheriting them can often lead to unexpected results (particularly as far as the default manager goes). Therefore, they aren’t passed onto child classes.

I also think this example shows some reasons why this inheritance is embarrasing:

class SimpleComment(models.Model):
    test_objects = models.Manager()


class BaseComment(models.Model):
    pass


class Comment(BaseComment):
    test_objects = models.Manager()

print(hasattr(models.SimpleComment, 'objects')) #False
print(hasattr(models.Comment, 'objects'))  #True, we may expect False here, since 'SimpleComment' gives False in similar sitation.
print(models.Comment.objects.model) #<class 'BaseComment'>, this manager is not contributed to 'Comment' class

Tim Graham suggests asking if anyone relying on this inheritance and documentation should be fixed instead. Any suggestions?

[Tim Graham]

Could this go through a deprecation where any use of the inherited managers to be removed will raise a warning for a couple releases? If anyone is relying on the behavior, they just need to add the managers to any subclasses, correct?

[Alex Poleha]

Thank you for the suggestion. Pull request is adjusted to give deprecation warning instead of raising AttributeError.
Yes, to silence the warning manager need to be be added to any subclass explicitly. It is explained in documentation for ages, so I don't think there would be a problem.

среда, 3 февраля 2016 г., 19:18:24 UTC+3 пользователь Tim Graham написал:

[Shai Berger]

Reading your description again, it seems like you apply the condition to
default managers as well. Default managers are not "specific to the class they
are defined on", and I see no problem in their inheritance. In particular:

class BaseComment(models.Model):
... some fields, no manager


class Comment(BaseComment):
... some fields, still no manager

Now, are you suggesting that Comment.objects must be defined explicitly? I find
that odd.

Shai.

On Saturday 06 February 2016 18:07:22 Alex Poleha wrote:
> Thank you for the suggestion. Pull request is adjusted to give deprecation
> warning instead of raising AttributeError.
> Yes, to silence the warning manager need to be be added to any subclass
> explicitly. It is explained in documentation

> <https://docs.djangoproject.com/en/dev/topics/db/managers/#custom-managers-
> and-model-inheritance> for ages, so I don't think there would be a problem.

>
> среда, 3 февраля 2016 г., 19:18:24 UTC+3 пользователь Tim Graham написал:
> > Could this go through a deprecation where any use of the inherited
> > managers to be removed will raise a warning for a couple releases? If
> > anyone is relying on the behavior, they just need to add the managers to
> > any subclasses, correct?
> >
> > On Wednesday, February 3, 2016 at 9:16:41 AM UTC-5, Alex Poleha wrote:
> >> Hi.
> >>
> >> According to documentation

> >> <https://docs.djangoproject.com/en/1.9/topics/db/managers/#custom-manage
> >> rs-and-model-inheritance> managers defined on non-abstract base classes

> >> are not inherited by child classes. In fact they're inherited via
> >> python MRO. I made pull request

> >> <https://github.com/django/django/pull/5797> to fix it. I find this

> >>
> >> inheritance embarrassing due to reasons, explained in documentation:

> >> 1. Managers defined on non-abstract base classes are *not* inherited

[Alex Poleha]

I apply my condition to default managers indeed and I see no problem here. Default manager is just first manager defined on class(or on it's non concrete base). It has no additional magic.

About your example. I would never have suggested such a thing. And there will be no need to redeclare 'objects' manager in Comment class for you example after applying my patch.

Please look at my example.
For SimpleComment default manager is 'test_objects'. It doesn't have 'objects' attribute.
For Comment before applying my patch there would be 'objects' attribute, but not in his own __dict__, but in Comment.__dict__, available through python mro. That's what I want to fix.

In my example if you want to have 'objects' attribute available for SimpleComment now and for Comment after applying my patch you will have to redeclare it explicitly, because these classes have 'test_objects' as default manager.

[Shai Berger]

On Sunday 07 February 2016 19:47:48 Alex Poleha wrote:
> I apply my condition to default managers indeed and I see no problem here.
> Default manager is just first manager defined on class(or on it's non
> concrete base). It has no additional magic.
>

Well, I was using wrong terminology, sorry; when I said "default manager" I
meant the automatic manager supplied by Django when there is no other. Sorry
about being unclear.

> About your example. I would never have suggested such a thing. And there
> will be no need to redeclare 'objects' manager in Comment class for you
> example after applying my patch.
>

That's all I wanted to be sure of; your example made me think otherwise, and I
didn't look at the actual code.

Thanks,
Shai.