New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #10506, #13793, #14891, #20932, #25201, #25897 -- Refactored managers inheritance #6175

Merged
merged 2 commits into from May 17, 2016

Conversation

Projects
None yet
5 participants
@loic
Member

loic commented Feb 21, 2016

Refactor managers inheritance and introduce new APIs for specifying the default and base manager of a given model. Also deprecate use_for_related_fields.

  • Reimplement 515a8a3 using the _base_manager and _default_manager properties.
  • Write tests for the 2 deprecations (use_for_related_fields, and legacy manager inheritance)
  • Document _base_manager and _default_manager, they've always existed but should be public APIs (so 3rd party apps use them).
  • Write docs + release notes for the new meta APIs.
  • Document the deprecations.
  • Decide on #6175 (comment).
Show outdated Hide outdated docs/topics/db/managers.txt Outdated
@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Feb 22, 2016

Member

This looks really nice IMO! Much easier to understand than the previous system.

The main thing I notice is that we still have a discrepancy between different types of relations in terms of which manager is the fallback default. The relation types that support(ed) use_for_related_fields fall back to _base_manager (once the deprecation of use_for_related_fields runs its course), whereas the other ones fall back to _default_manager. That's a bit weird. It would be nice if we could make the fallback consistent (I think it should always fall back to _base_manager, personally), and then we could clean up some of the "if related_manager" code by having the Options.related_manager property just do the fallback internally, instead of returning None.

I'm not sure how we could make that change in a backwards-compatible way, though :/

Member

carljm commented Feb 22, 2016

This looks really nice IMO! Much easier to understand than the previous system.

The main thing I notice is that we still have a discrepancy between different types of relations in terms of which manager is the fallback default. The relation types that support(ed) use_for_related_fields fall back to _base_manager (once the deprecation of use_for_related_fields runs its course), whereas the other ones fall back to _default_manager. That's a bit weird. It would be nice if we could make the fallback consistent (I think it should always fall back to _base_manager, personally), and then we could clean up some of the "if related_manager" code by having the Options.related_manager property just do the fallback internally, instead of returning None.

I'm not sure how we could make that change in a backwards-compatible way, though :/

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Feb 22, 2016

Member

Yes that discrepancy is very annoying. I'd have to think about it some more but changing either one of them has significant consequences. Upgrading _base_manager to _default_manager for "to-one" relations will be nasty to anyone who does filtering in get_queryset(). Downgrading _default_manager to _base_manager on "to-many" relations means all the custom manager methods wouldn't be accessible anymore by default.

But if we wanna do it, it should be pretty easy to do using the deprecation path I added to the other PR. If the related model has _meta.manager_inheritance_from_future use the new fallback, else issue warning and call the old fallback.

Member

loic commented Feb 22, 2016

Yes that discrepancy is very annoying. I'd have to think about it some more but changing either one of them has significant consequences. Upgrading _base_manager to _default_manager for "to-one" relations will be nasty to anyone who does filtering in get_queryset(). Downgrading _default_manager to _base_manager on "to-many" relations means all the custom manager methods wouldn't be accessible anymore by default.

But if we wanna do it, it should be pretty easy to do using the deprecation path I added to the other PR. If the related model has _meta.manager_inheritance_from_future use the new fallback, else issue warning and call the old fallback.

Show outdated Hide outdated django/db/models/options.py Outdated
@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Feb 22, 2016

Member

Maybe it really is better for to-one relations to default to _base_manager (because you don't get direct access to a manager there anyway, and you need to be very careful about any filtering), and to-many relations to default to _default_manager (because you do get a manager, and you may want to use custom methods on it, or get a custom queryset type back). But if that's the case, and we decide to continue with differing defaults there, then I'm not sure it makes sense to introduce a single related_manager and use that universally if set. If the needs in the two cases are sufficiently different to justify a different default, then they are sufficiently different to justify separate overriding of that default.

So I think that we should either a) go through a deprecation as you outlined to make all of the relation types default to base_manager, or b) provide both to_one_related_manager and to_many_related_manager overrides. I suppose there is also an option c) have related_manager apply only to to-one relations, the same as use_for_related_fields does now, and have to-many relations continue to unconditionally use the default manager.

I'm still thinking through which of those options I prefer. I think I'm actually leaning towards b at the moment (because I think the argument for differing needs in to-many vs to-one relations is pretty strong), though I don't love the unwieldy naming (and general profusion of options) that it requires.

Member

carljm commented Feb 22, 2016

Maybe it really is better for to-one relations to default to _base_manager (because you don't get direct access to a manager there anyway, and you need to be very careful about any filtering), and to-many relations to default to _default_manager (because you do get a manager, and you may want to use custom methods on it, or get a custom queryset type back). But if that's the case, and we decide to continue with differing defaults there, then I'm not sure it makes sense to introduce a single related_manager and use that universally if set. If the needs in the two cases are sufficiently different to justify a different default, then they are sufficiently different to justify separate overriding of that default.

So I think that we should either a) go through a deprecation as you outlined to make all of the relation types default to base_manager, or b) provide both to_one_related_manager and to_many_related_manager overrides. I suppose there is also an option c) have related_manager apply only to to-one relations, the same as use_for_related_fields does now, and have to-many relations continue to unconditionally use the default manager.

I'm still thinking through which of those options I prefer. I think I'm actually leaning towards b at the moment (because I think the argument for differing needs in to-many vs to-one relations is pretty strong), though I don't love the unwieldy naming (and general profusion of options) that it requires.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Feb 22, 2016

Member

(In any case, regardless of which of the above options we pick, it deserves much clearer -- and more accurate -- documentation than use_for_related_fields ever had!)

Member

carljm commented Feb 22, 2016

(In any case, regardless of which of the above options we pick, it deserves much clearer -- and more accurate -- documentation than use_for_related_fields ever had!)

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Feb 22, 2016

Member

I think it's worth thinking about use_for_related_fields as currently implemented in Django. My understanding of that attribute is that it has nothing to do with relations, and more to do with
"this model has got weird columns and really needs a special manager to work" (i.e. GIS). Since you couldn't control _base_manager directly, you'd set that attribute on the default manager to reach those "to-one" fields by also pledging you wouldn't filter things out. "to-many" fields were already covered by virtue of using the default manager directly.

Now that we have base_manager as a public API, I don't really see a case for c). A "this model needs me to work at all" kind of manager can just be declared on the model and specified using _meta.base_manager_name, if you don't have other managers it'll automatically be the default manager as well, if you need your default manager to do even more, then you can easily create a new class that extend the base manager class and use that as default manager.

I'm not too sure about b) I don't think in most cases you really need to differentiate between "to-one" and "to-many" relations as far as custom managers go.

I think my preferred option would have been for everything to use _default_manager (except things like dumpdata that need _base_manager obviously) and _meta.related_manager_name being there and affecting all relations equally for when you need to rectify something the default manager does that doesn't make sense in the context of relations.

Member

loic commented Feb 22, 2016

I think it's worth thinking about use_for_related_fields as currently implemented in Django. My understanding of that attribute is that it has nothing to do with relations, and more to do with
"this model has got weird columns and really needs a special manager to work" (i.e. GIS). Since you couldn't control _base_manager directly, you'd set that attribute on the default manager to reach those "to-one" fields by also pledging you wouldn't filter things out. "to-many" fields were already covered by virtue of using the default manager directly.

Now that we have base_manager as a public API, I don't really see a case for c). A "this model needs me to work at all" kind of manager can just be declared on the model and specified using _meta.base_manager_name, if you don't have other managers it'll automatically be the default manager as well, if you need your default manager to do even more, then you can easily create a new class that extend the base manager class and use that as default manager.

I'm not too sure about b) I don't think in most cases you really need to differentiate between "to-one" and "to-many" relations as far as custom managers go.

I think my preferred option would have been for everything to use _default_manager (except things like dumpdata that need _base_manager obviously) and _meta.related_manager_name being there and affecting all relations equally for when you need to rectify something the default manager does that doesn't make sense in the context of relations.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Feb 22, 2016

Member

Or maybe just deprecate use_for_related_fields and not replace it with anything specific to relations (people can use the generic _base_manager system I mentioned above). It's worth noting that Django 2.0 gets rid of GeoManager which I presume is the most common usage of use_for_related_fields in the wild.

Member

loic commented Feb 22, 2016

Or maybe just deprecate use_for_related_fields and not replace it with anything specific to relations (people can use the generic _base_manager system I mentioned above). It's worth noting that Django 2.0 gets rid of GeoManager which I presume is the most common usage of use_for_related_fields in the wild.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Feb 23, 2016

Member

I'm ok with having related_manager as an option, defaulting to default_manager. It goes from an "opt-in to default manager for to-one relations" system with use_for_related_fields, to an "opt out of default manager for any relation field" system. I think the most common issue people will hit with that is if they have a default manager that filters by default (e.g. for soft-deletion schemes), they will need to override related_manager just to get back to an unfiltered manager for their relation fields.

Member

carljm commented Feb 23, 2016

I'm ok with having related_manager as an option, defaulting to default_manager. It goes from an "opt-in to default manager for to-one relations" system with use_for_related_fields, to an "opt out of default manager for any relation field" system. I think the most common issue people will hit with that is if they have a default manager that filters by default (e.g. for soft-deletion schemes), they will need to override related_manager just to get back to an unfiltered manager for their relation fields.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Feb 23, 2016

Member

I just did a little experiment #6179, which demonstrates that use_for_related_fields actually means "also use default manager as base manager". It wasn't the case at first, but it is since 7d9b29a (2009).

So effectively _meta.base_manager_name and _meta.default_manager_name are enough to deprecate use_for_related_fields without loss of functionality. Therefore we should really look at related_manager as a new feature and not as a fix/cleanup anymore.

Now, if we ever wanted to add such feature and want the fallback to be consistent or even just change the to-one managers without adding related_manager, no doubt that now is the perfect time, because it can piggyback on the manager_inheritance_from_future upgrade path, and this kind of deprecation (that requires opt-in all over the place, then cleanup after the deprecation) is rather painful.

I see the following usage of _base_manager:

Other than the last two bullets, all usages are exactly where we'd expect. Having it for to-one relations is I guess debatable but personally it doesn't strike me as the obvious choice. If you are OK with missing records on a "to-many", why not on "to-one"? Unless it could break Django's internal somehow, although I can't see how (famous last words)...

I see the following options:

  1. Commit to the status quo regarding _base_manager and to-one relations, and don't add (possibly never add) related_manager.
  2. Change _base_manager for to-one relations, then add related_manager later should people want it.
  3. Change _base_manager for to-one relations and introduce related_manager as a new feature (maybe to sweeten the bitter deprecation?).

I think my vote goes to 2 and 3, but I can live with 1. The _base_manager change for to-one relations probably should seek the technical board approval.

Member

loic commented Feb 23, 2016

I just did a little experiment #6179, which demonstrates that use_for_related_fields actually means "also use default manager as base manager". It wasn't the case at first, but it is since 7d9b29a (2009).

So effectively _meta.base_manager_name and _meta.default_manager_name are enough to deprecate use_for_related_fields without loss of functionality. Therefore we should really look at related_manager as a new feature and not as a fix/cleanup anymore.

Now, if we ever wanted to add such feature and want the fallback to be consistent or even just change the to-one managers without adding related_manager, no doubt that now is the perfect time, because it can piggyback on the manager_inheritance_from_future upgrade path, and this kind of deprecation (that requires opt-in all over the place, then cleanup after the deprecation) is rather painful.

I see the following usage of _base_manager:

Other than the last two bullets, all usages are exactly where we'd expect. Having it for to-one relations is I guess debatable but personally it doesn't strike me as the obvious choice. If you are OK with missing records on a "to-many", why not on "to-one"? Unless it could break Django's internal somehow, although I can't see how (famous last words)...

I see the following options:

  1. Commit to the status quo regarding _base_manager and to-one relations, and don't add (possibly never add) related_manager.
  2. Change _base_manager for to-one relations, then add related_manager later should people want it.
  3. Change _base_manager for to-one relations and introduce related_manager as a new feature (maybe to sweeten the bitter deprecation?).

I think my vote goes to 2 and 3, but I can live with 1. The _base_manager change for to-one relations probably should seek the technical board approval.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Feb 23, 2016

Member

For completeness, I made another experiment #6182 that implements all the proposed _base_manager > _default_manager changes to see how the test suite behaves. Only 2 test failures, out of which only one is specifically testing for the behavior (for GFK nonetheless!). Which means that the behavior of FK/O2O wasn't actually tested! Sadly it doesn't make a difference since our docs were pretty explicit about the expected behavior.

Member

loic commented Feb 23, 2016

For completeness, I made another experiment #6182 that implements all the proposed _base_manager > _default_manager changes to see how the test suite behaves. Only 2 test failures, out of which only one is specifically testing for the behavior (for GFK nonetheless!). Which means that the behavior of FK/O2O wasn't actually tested! Sadly it doesn't make a difference since our docs were pretty explicit about the expected behavior.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Feb 28, 2016

Member

@timgraham what's the timeline like for 1.10 alpha?

One option could be to go ahead with the refactor, deprecate use_for_related_fields using the new base_manager_name and default_manager_name APIs and stop short of making the _base_manager > _default_manager change or introducing related_manager_name.

Then approach the orthogonal _base_manager > _default_manager issue independently but within the same release so that if we want to go ahead, we can use the same manager_inheritance_from_future deprecation/upgrade path.

related_manager_name can be added any time, possibly in a future release provided we addressed the _base_manager > _default_manager inconsistency.

Member

loic commented Feb 28, 2016

@timgraham what's the timeline like for 1.10 alpha?

One option could be to go ahead with the refactor, deprecate use_for_related_fields using the new base_manager_name and default_manager_name APIs and stop short of making the _base_manager > _default_manager change or introducing related_manager_name.

Then approach the orthogonal _base_manager > _default_manager issue independently but within the same release so that if we want to go ahead, we can use the same manager_inheritance_from_future deprecation/upgrade path.

related_manager_name can be added any time, possibly in a future release provided we addressed the _base_manager > _default_manager inconsistency.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham
Member

timgraham commented Feb 28, 2016

1.10 alpha is scheduled for May 16: https://code.djangoproject.com/wiki/Version1.10Roadmap

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Mar 5, 2016

Member

@loic Sorry it took me a while to get back to this. As I tried to say above, I do think there is a somewhat-reasonable rationale behind the use of _base_manager for to-one relations but not to-many. Presume the existence of a default manager that filters rows (let's say in a soft-deletion scenario; I've personally implemented this). It is reasonable to expect (and I would guess that quite a lot of existing Django code does expect) that if you have a non-nullable FK on a model, and a database that maintains referential integrity, that accessing that FK should always return a related object, it should never raise DoesNotExist. But if a filtering manager is used for this relationship, that is suddenly no longer guaranteed. If that object exists at all (even if it has been soft-deleted), chances are good that it's more useful to you to be able to get access to it (and then you can check whether it is soft-deleted or not), rather than have Django just raise a DoesNotExist error. In a soft-deletion case, for instance, you may intentionally load up a soft-deleted object (e.g. for "undelete" functionality), with to-one related objects that are also soft-deleted, and you want those relationships to still work, not to all raise DoesNotExist.

The situation with a to-many relationship is quite different, because you are getting back a queryset of objects, and having that queryset filtered by your default manager doesn't fundamentally change the contract in any way. Plus, you actually have access to both the manager and queryset (and their methods) when you use the to-many relation, therefore it's much more likely that it is useful to you to have your custom manager available (whereas in a to-one relation, the use of a custom manager is completely irrelevant to you, unless it's a filtering manager).

I'm not sure at this moment what that implies we should do. It may be that we should still aim for stronger consistency and ignore these differences. But I think we need to be clear that there are good practical reasons for the current apparent inconsistency, before we propose to change it.

(I like your latest idea of splitting the two changes up; always good to split a big change into smaller independent changes when we can.)

Member

carljm commented Mar 5, 2016

@loic Sorry it took me a while to get back to this. As I tried to say above, I do think there is a somewhat-reasonable rationale behind the use of _base_manager for to-one relations but not to-many. Presume the existence of a default manager that filters rows (let's say in a soft-deletion scenario; I've personally implemented this). It is reasonable to expect (and I would guess that quite a lot of existing Django code does expect) that if you have a non-nullable FK on a model, and a database that maintains referential integrity, that accessing that FK should always return a related object, it should never raise DoesNotExist. But if a filtering manager is used for this relationship, that is suddenly no longer guaranteed. If that object exists at all (even if it has been soft-deleted), chances are good that it's more useful to you to be able to get access to it (and then you can check whether it is soft-deleted or not), rather than have Django just raise a DoesNotExist error. In a soft-deletion case, for instance, you may intentionally load up a soft-deleted object (e.g. for "undelete" functionality), with to-one related objects that are also soft-deleted, and you want those relationships to still work, not to all raise DoesNotExist.

The situation with a to-many relationship is quite different, because you are getting back a queryset of objects, and having that queryset filtered by your default manager doesn't fundamentally change the contract in any way. Plus, you actually have access to both the manager and queryset (and their methods) when you use the to-many relation, therefore it's much more likely that it is useful to you to have your custom manager available (whereas in a to-one relation, the use of a custom manager is completely irrelevant to you, unless it's a filtering manager).

I'm not sure at this moment what that implies we should do. It may be that we should still aim for stronger consistency and ignore these differences. But I think we need to be clear that there are good practical reasons for the current apparent inconsistency, before we propose to change it.

(I like your latest idea of splitting the two changes up; always good to split a big change into smaller independent changes when we can.)

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Apr 15, 2016

Member

Thanks Carl.

In a soft-deletion case, for instance, you may intentionally load up a soft-deleted object (e.g. for "undelete" functionality) [...] The situation with a to-many relationship is quite different, because you are getting back a queryset of objects, and having that queryset filtered by your default manager doesn't fundamentally change the contract in any way. Plus, you actually have access to both the manager and queryset (and their methods)

That's very true. to-many also have the option to "undelete" using any manager available with the callable syntax. I wish we could implement this for to-one relations but __call__ doesn't work on descriptors so unless we add a magic wrapper around models it's not possible. It's worth noting that you can always undelete by manually querying the other model instead of going through the relation, but I agree it's not ideal.

So the more I think of it, the less I believe we should go for a blanket _related_manager / related_manager_name. I'm not too keen on having both a _to_one_related_manager and to_many_related_manager yet, but anyway this can be added anytime without backwards compatibility issues, so we can punt the issue.

Member

loic commented Apr 15, 2016

Thanks Carl.

In a soft-deletion case, for instance, you may intentionally load up a soft-deleted object (e.g. for "undelete" functionality) [...] The situation with a to-many relationship is quite different, because you are getting back a queryset of objects, and having that queryset filtered by your default manager doesn't fundamentally change the contract in any way. Plus, you actually have access to both the manager and queryset (and their methods)

That's very true. to-many also have the option to "undelete" using any manager available with the callable syntax. I wish we could implement this for to-one relations but __call__ doesn't work on descriptors so unless we add a magic wrapper around models it's not possible. It's worth noting that you can always undelete by manually querying the other model instead of going through the relation, but I agree it's not ideal.

So the more I think of it, the less I believe we should go for a blanket _related_manager / related_manager_name. I'm not too keen on having both a _to_one_related_manager and to_many_related_manager yet, but anyway this can be added anytime without backwards compatibility issues, so we can punt the issue.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Apr 15, 2016

Member

👍 on punting the tricky API issue and merging your excellent refactoring work :-)

Member

carljm commented Apr 15, 2016

👍 on punting the tricky API issue and merging your excellent refactoring work :-)

@loic loic changed the title from Managers meta to Refactor managers inheritance Apr 16, 2016

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic Apr 17, 2016

Member

@MarkusH if you have some time, could you validate the migrations/state.py bits?

Member

loic commented Apr 17, 2016

@MarkusH if you have some time, could you validate the migrations/state.py bits?

[models.Manager, FoodManager, FoodManager])
self.assertIs(managers[0][1], Food._default_manager)

This comment has been minimized.

@MarkusH

MarkusH May 6, 2016

Member

Why did you drop this check.

@MarkusH

MarkusH May 6, 2016

Member

Why did you drop this check.

This comment has been minimized.

@loic

loic May 8, 2016

Member

Because _default_manager is now just an accessor to the default manager, not the manager itself, and it can point to any manager depending on Meta.default_manager_name, so checking it agains the first manager in _meta. managers becomes a bit arbitrary.

@loic

loic May 8, 2016

Member

Because _default_manager is now just an accessor to the default manager, not the manager itself, and it can point to any manager depending on Meta.default_manager_name, so checking it agains the first manager in _meta. managers becomes a bit arbitrary.

@MarkusH

This comment has been minimized.

Show comment
Hide comment
@MarkusH

MarkusH May 6, 2016

Member

@MarkusH if you have some time, could you validate the migrations/state.py bits?

Sorry, @loic that it took me so long. Looks clean to me.

Member

MarkusH commented May 6, 2016

@MarkusH if you have some time, could you validate the migrations/state.py bits?

Sorry, @loic that it took me so long. Looks clean to me.

`access related objects`_. There are other places in the implementation of
Django where temporary plain managers are needed. Those automatically created
managers will normally be instances of the :class:`django.db.models.Manager`
class.

This comment has been minimized.

@loic

loic May 8, 2016

Member

Notes:

Now that we introduce APIs such as Meta.default_manager_name, Meta.base_manager_name, Model._default_manager, and Model._base_manager, we should stop referring to them as just concepts like "plain", or "automatic" but instead point to their respective documentation.

Regarding Model._default_manager and Model._base_manager we should make it explicit that these are the managers you should use any time you deal with undetermined models (e.g. Django generic views, or admin views), it's especially needed for 3rd party apps.

When documenting _base_manager, we should be more explicit about actual usage in Django: cascading deletions, dumpdata, to-one relations. We probably want to mention that unless you know that you really want to use _base_manager you should use _default_manager.

@loic

loic May 8, 2016

Member

Notes:

Now that we introduce APIs such as Meta.default_manager_name, Meta.base_manager_name, Model._default_manager, and Model._base_manager, we should stop referring to them as just concepts like "plain", or "automatic" but instead point to their respective documentation.

Regarding Model._default_manager and Model._base_manager we should make it explicit that these are the managers you should use any time you deal with undetermined models (e.g. Django generic views, or admin views), it's especially needed for 3rd party apps.

When documenting _base_manager, we should be more explicit about actual usage in Django: cascading deletions, dumpdata, to-one relations. We probably want to mention that unless you know that you really want to use _base_manager you should use _default_manager.

manager, there are a couple of things to remember.
Do not filter away any results in this type of manager subclass
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This comment has been minimized.

@loic

loic May 8, 2016

Member

This section can be reused for the documentation of Meta.default_manager_name.

@loic

loic May 8, 2016

Member

This section can be reused for the documentation of Meta.default_manager_name.

Show outdated Hide outdated docs/topics/db/managers.txt Outdated
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 13, 2016

Member

A start at the docs is at loic#2, I'll be happy to make any additions or improvements.

Member

timgraham commented May 13, 2016

A start at the docs is at loic#2, I'll be happy to make any additions or improvements.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic May 14, 2016

Member

Thanks a lot Tim for writing the docs, I feel really bad that this landed on you, the last few weeks have been completely chaotic. To add insult to injury, I'm afraid I have to propose some last minute changes to the API (which luckily would only have a minimal impact on the docs if accepted).

While reading the new docs I realised that I'm still not totally happy with the inheritance story of _base_manager and _default_manager:

  1. Other than for MTI, _default_manager keeps the old inheritance behavior: either a model has its own manager (the first declared or the one it defines with default_manager_name) or we pick the first inherited one. But if the parent explicitly defined its default manager with default_manager_name I think it would make sense to respect that.
  2. The inheritance behavior of _base_manager was previously discussed here. I did some experiments and I think the new behavior is not ideal, especially for proxy models, since by default they should behave exactly the same as their parent.

In absence of an explicit base_manager_name or default_manager_name I think we should account for those of the first parent. I first thought of making the base_manager_name and default_manager_name options sticky (like ordering), but I think declaring a manager is a stronger signal than an inherited meta for the case of _default_manager, so that doesn't work. Instead I propose that if we don't have an explicit manager, we take the one from the first parent. Other than the "a manager declared on a model trumps an inherited _default_manager" rule, this is basically MRO inheritance applied to the _*_manager accessors (since every Model define their own, the first parent always win).

I added a tentative commit to this PR to implement this behavior. @timgraham, @carljm what do you think?

Member

loic commented May 14, 2016

Thanks a lot Tim for writing the docs, I feel really bad that this landed on you, the last few weeks have been completely chaotic. To add insult to injury, I'm afraid I have to propose some last minute changes to the API (which luckily would only have a minimal impact on the docs if accepted).

While reading the new docs I realised that I'm still not totally happy with the inheritance story of _base_manager and _default_manager:

  1. Other than for MTI, _default_manager keeps the old inheritance behavior: either a model has its own manager (the first declared or the one it defines with default_manager_name) or we pick the first inherited one. But if the parent explicitly defined its default manager with default_manager_name I think it would make sense to respect that.
  2. The inheritance behavior of _base_manager was previously discussed here. I did some experiments and I think the new behavior is not ideal, especially for proxy models, since by default they should behave exactly the same as their parent.

In absence of an explicit base_manager_name or default_manager_name I think we should account for those of the first parent. I first thought of making the base_manager_name and default_manager_name options sticky (like ordering), but I think declaring a manager is a stronger signal than an inherited meta for the case of _default_manager, so that doesn't work. Instead I propose that if we don't have an explicit manager, we take the one from the first parent. Other than the "a manager declared on a model trumps an inherited _default_manager" rule, this is basically MRO inheritance applied to the _*_manager accessors (since every Model define their own, the first parent always win).

I added a tentative commit to this PR to implement this behavior. @timgraham, @carljm what do you think?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 14, 2016

Member

The proposed changes make sense to me.

Member

timgraham commented May 14, 2016

The proposed changes make sense to me.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 14, 2016

Member

I added some keywords to Trac tickets to help identify issues that this PR might affect.

Member

timgraham commented May 14, 2016

I added some keywords to Trac tickets to help identify issues that this PR might affect.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic May 15, 2016

Member

Thanks Tim, I squashed the tentative commit and updated the docs with the new behavior. Barring any objections I think this is good to go.

Member

loic commented May 15, 2016

Thanks Tim, I squashed the tentative commit and updated the docs with the new behavior. Barring any objections I think this is good to go.

Show outdated Hide outdated docs/releases/1.10.txt Outdated
Show outdated Hide outdated docs/releases/1.10.txt Outdated

@timgraham timgraham changed the title from Refactor managers inheritance to Fixed #20932, #13793, #14891 -- Refactored managers inheritance May 15, 2016

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 15, 2016

Member

You may also squash my docs commit into the appropriate commit(s).

Member

timgraham commented May 15, 2016

You may also squash my docs commit into the appropriate commit(s).

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 16, 2016

Member

Needs a rebase, otherwise LGTM.

Member

timgraham commented May 16, 2016

Needs a rebase, otherwise LGTM.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 16, 2016

Member

Does this fix #14891 as the commit message indicates? It seems like more work needs to be done to add Meta.related_manager_name unless I haven't full understood how these ideas have evolved. If you could comment on each related ticket to give an update on the next steps for remaining work, that would be really helpful.

Member

timgraham commented May 16, 2016

Does this fix #14891 as the commit message indicates? It seems like more work needs to be done to add Meta.related_manager_name unless I haven't full understood how these ideas have evolved. If you could comment on each related ticket to give an update on the next steps for remaining work, that would be really helpful.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic May 16, 2016

Member

I think anything related to use_for_related_fields should now be considered fixed. You can customize the manager used by "to-one" fields with base_manager_name (which is also used for other types of operations) and the manager for "to-many" fields with "default_manager_name" (which is obviously also the default manager of the model). You can also customize the manager for "to-many" fields at query time through __call__. We tried to add a blanket "use for every relations" manager in the form of related_manager_name but it's probably not that helpful.

In the space of relation managers customization, I think the manager and related_manager kwargs proposal is the way to go but IMO that's a new feature that deserves its own ticket.

Member

loic commented May 16, 2016

I think anything related to use_for_related_fields should now be considered fixed. You can customize the manager used by "to-one" fields with base_manager_name (which is also used for other types of operations) and the manager for "to-many" fields with "default_manager_name" (which is obviously also the default manager of the model). You can also customize the manager for "to-many" fields at query time through __call__. We tried to add a blanket "use for every relations" manager in the form of related_manager_name but it's probably not that helpful.

In the space of relation managers customization, I think the manager and related_manager kwargs proposal is the way to go but IMO that's a new feature that deserves its own ticket.

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic May 16, 2016

Member

Since I mention the future manager and related_manager kwarg for relation fields, and the __call__ syntax that has a mandatory kwarg called manager, now would be a good time to assert if we are really happy with the _name suffix of the newly introduced meta options...

Member

loic commented May 16, 2016

Since I mention the future manager and related_manager kwarg for relation fields, and the __call__ syntax that has a mandatory kwarg called manager, now would be a good time to assert if we are really happy with the _name suffix of the newly introduced meta options...

@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic May 16, 2016

Member

I should probably also mention that we thought of calling the __call__ kwarg "manager_name" at the time, but concluded it was too long.

In favor of keeping the suffix though, I think (it's been a long while!) we also considered that we may want to futurely support something like model.m2m(manager=MyManager()) for the __call__ syntax, which is certainly something we don't want to support in Meta.

Member

loic commented May 16, 2016

I should probably also mention that we thought of calling the __call__ kwarg "manager_name" at the time, but concluded it was too long.

In favor of keeping the suffix though, I think (it's been a long while!) we also considered that we may want to futurely support something like model.m2m(manager=MyManager()) for the __call__ syntax, which is certainly something we don't want to support in Meta.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 17, 2016

Member

I think we could drop the _name suffix of the options without creating any confusion.

Member

timgraham commented May 17, 2016

I think we could drop the _name suffix of the options without creating any confusion.

@loic loic changed the title from Fixed #20932, #13793, #14891 -- Refactored managers inheritance to Fixed #10506, #13793, #14891, #20932, #25201, #25897 -- Refactored managers inheritance May 17, 2016

Fixed #10506, #13793, #14891, #25201 -- Introduced new APIs to specif…
…y 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.
@loic

This comment has been minimized.

Show comment
Hide comment
@loic

loic May 17, 2016

Member

I'm aborting the removal of the _name suffix, since Options exposes both the name (supplied by the user) and the manager instance (derived from that name and/or from the MRO resolution), it makes the removal a bit fiddly. Worse a custom Options class trying to quack like Django's may want to return a kind of manager directly under the Options.default|base_manager keys.

Since the other types of API could at least in theory support both a manager name and a manager class/instance (e.g. model.m2m(manager=MyManager()), or ForeignKey(manager=MyManager(), related_manager=MyReveseManager()) whereas Model.Meta shouldn't (TIMTOWTDI and all that) I think I can live with the naming inconsistency.

Member

loic commented May 17, 2016

I'm aborting the removal of the _name suffix, since Options exposes both the name (supplied by the user) and the manager instance (derived from that name and/or from the MRO resolution), it makes the removal a bit fiddly. Worse a custom Options class trying to quack like Django's may want to return a kind of manager directly under the Options.default|base_manager keys.

Since the other types of API could at least in theory support both a manager name and a manager class/instance (e.g. model.m2m(manager=MyManager()), or ForeignKey(manager=MyManager(), related_manager=MyReveseManager()) whereas Model.Meta shouldn't (TIMTOWTDI and all that) I think I can live with the naming inconsistency.

@loic loic merged commit ed0ff91 into django:master May 17, 2016

6 checks passed

default Build finished.
Details
docs Build finished.
Details
flake8 Build finished.
Details
isort Build finished.
Details
javascript Build finished.
Details
windows Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment