[Django] #26749: Different manager for _base_manager and related descriptors
Django
----------------------------------------------+---------------------
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.10
Severity: Normal | Keywords: manager
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+---------------------
I am in the process of testing Django 1.10 and upgrading the third-party
package I maintain, django-hvad.
This ticket is related to the two following commits:
* '''Streamlined manager inheritance.:'''
https://github.com/django/django/commit/3a47d42fa33012b2156bf04058d933df6b3082d2
* '''Introduced new APIs to specify models' default and base managers:'''
https://github.com/django/django/commit/ed0ff913c648b16c4471fc9a9441d1ee48cb5420
The issue I have with the way inheritance and customization now works is
it is very hard to have a different manager as the `base_manager` and the
one used for related field descriptors.
''Why the need?''
Django-hvad is one of the modules that change the default querysets. It
does it by replacing the default manager with one that brings extra
features. Namely, automatic joining onto a translations model.
Having those features available on related fields is essential to provide
a consistent interface, and after years of having it, it is now essential
to backward compatibility as well.
It does not break the basic rule of “a base manager must not hide
objects”, as it merely adds automatic `select_related()` rules and
`iterator()` encapsulation.
Yet, those extra features cannot be on the `base_manager`, due to the way
ORM internals use it (for instance, triggering additional joins on a
delete would not work).
''Why the ticket?''
Up to Django 1.9, this was possible using `use_for_related_fields = True`,
and fixing the `_base_manager` in the `class_prepared` hook. Not clean but
workable.
With the new API though, all the manager machinery uses property
descriptors on the `Options` class, making it difficult and fragile to
override. And at the same time, using `use_for_related_fields` got
deprecated, which means there is no option to use '''a different manager
as the base manager and as the related field descriptor's manager'''
anymore.
Unless there is another way to do this?
--
Ticket URL: <https://code.djangoproject.com/ticket/26749>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: loic (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:1>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
At some point in the design of the new API we had another model option
called `related_manager_name` to do basically what OP is asking
(discussions at https://github.com/django/django/pull/6175). There was
some design decisions to make on whether it should only affect to-one
rels, or every rels, and on whether we should introduce even more
granularity with options like `to_one_related_manager_name` and
`to_many_related_manager_name`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:2>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* stage: Unreviewed => Accepted
Comment:
After discussion with loic in IRC, I think it makes sense to go ahead and
add a single `related_manager_name` option. That would cover this use case
(and all the ones I can think of), and wouldn't preclude later layering on
`many_related_manager_name` or `single_related_manager_name` if someone
finds compelling needs for them later on.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:3>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
Could it be possible to clarify the intended use of `base_manager`? All
over Django codebase, it's only used at 4 points:
1. when finding out the list of related objects in the deletion collector
2. when saving an existing model, the internal `_update` method is called
on a queryset created by te base manager.
3. when saving a newly created model, the internal `_insert` method is
called on the base manager.
4. in the `contrib.contenttypes` application, for ̀GenericForeignKey`.
5. ''And well, since the changes of the linked commits, it's now also used
for related fields, but hopefully that should be gone or overridable
soon.''
----
My understanding is use cases 1-3 are internal uses, and are the intended
users of the base manager.
On the other hand, use cases 4-5 expose the manager to user code, and
should be overridable to offer custom behavior to user code without
altering ORM internals.
The change you suggest, with `related_manager_name`, seems in line with my
understanding? If so, should `GenericForeignKey` use it as well?
(by the way, in the
[https://docs.djangoproject.com/en/1.10/topics/db/managers/#django.db.models.Model._default_manager
default manager documentation], I thinkg it should say
`default_manager_name` instead of `base_manager_name`?)
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:4>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
> Could it be possible to clarify the intended use of exhaustive?
I was originally planning to document the exhaustive list of base manager
usages, but Tim actually wrote the docs for that patch and went with a
terser description. I'm not against adding a bit more details, patch
welcome.
> since the changes of the linked commits, it's now also used for related
fields, but hopefully that should be gone or overridable soon
The previous implementation of one-related descriptors weren't
representative of intent:
- The forward M2O descriptor had its own logic with regards to
`use_for_related_fields` because its implementation predated the
introduction of `_base_manager`
(7d9b29a56ddc1a793a02d55743c372a8256b97aa).
- The reverse O2O descriptor as originally implemented **only used**
`_base_manager` until I changed it in Django 1.8
(e043aae9bb2e3fa244f59b07fc16f57476cb80ff).
In practice `use_for_related_fields` meant "the default manager can also
be used as base manager" since the introduction of `_base_manager`, as
demonstrated by your need to monkeypatch `_base_manager` back to a plain
manager during the class preparation. Also `_base_manager` was introduced
to generalize the `use_for_related_fields` mechanism as documented in the
[https://github.com/django/django/commit/292f503845401868ae9bf02b9bb1c38f35774c2c
#diff-f291b9c8a76d4ccc6c2aa3708b27b251R270 historical note] that was
commited the same day `_base_manager` was introduced.
> My understanding is use cases 1-3 are internal uses, and are the
intended users of the base manager.
I don't see a distinction between internal and external use. There is
nothing internal about the concept of inserting data and/or wanting to
guarantee unfiltered results (e.g. dumpdata, collection for deletion,
etc.), and for that you'd want to use a plain manager. Granted those are
low-level operations that most projects don't need to worry about, but
3rd-party libraries are definitely among the intended users.
> The change you suggest, with related_manager_name, seems in line with my
understanding? If so, should GenericForeignKey use it as well?
Definitely, GenericForeignKey and GenericRelation actually don't pick the
most adequate managers due to their reliance on the
`get_object_for_this_type` and `get_all_objects_for_this_type` utilities
which use `_base_manager`. I'm planning to undocument these and remove
every usage in Django (deprecating them would break
[https://github.com/search?q=get_object_for_this_type&type=Code&utf8=%E2%9C%93
too much working code]) in favor of
[https://github.com/django/django/commit/37564b084b4a8f64fefd4af98eb0a4bd13258311
inlining their code].
> (by the way, in the default manager documentation, I thinkg it should
say default_manager_name instead of base_manager_name?)
Well spotted, want to make a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:5>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
Before changing anything, I must be certain to fully understand.
What I meant by "internal" is the way the manager is used does not expose
it to user code in cases 1-3. In those use cases, the manager provides a
service to Django ORM, which could completely be implemented with no
manager interaction, user code would not notice.
→ On the other hand, in cases 4-5, Django simply picks the manager and
passes it to user code, which makes it possible for the manager to expose
additional features to user code.
So we have two very different use patterns. Therefore it makes sense to
separate them — `related_manager` and `base_manager`. Actually, I'd even
say that `related_manager` is closer to `default_manager` than
`base_manager` in purpose and use pattern.
----
Let's get back to `base_manager`. Actually, cases 1-3 are not the whole
story, as you pointed out. It also serves the purpose of providing a
unfiltered-access-to-model guarantee.
I believe that's a big red flag. Because those two purposes are different:
cases 1-3 are consumed by the ORM while the unfiltered guarantee is
consumed by user code.
Now, this unfiltered-guarantee is clearly defined and definitely a good
addition to the public side of the API. No problem with that. Now, though,
the purpose of customizing the inner workings of the ORM seems to be
inconsistently served:
* Overriding insertion:
- Either override `MyModel._do_insert`
- or just let it forward the call to the base_manager and override
`MyBaseManager._insert`
* Overriding updates:
- Override `Queryset.update` (for queryset-based updates)
- and either:
- override `MyModel._do_update`
- or let it forward the call to base_manager's queryset and
override `QuerySet._update`
* Overriding deletion:
- If object can be fast deleted, override base_manager's queryset
`_raw_delete` method.
- otherwise, no luck, you cannot override deletion, it is a hardcoded
`sql.DeleteQuery(model)`.
There is no object clearly responsible for updating the database.
Sometimes the queryset does it, sometimes the base_manager, sometimes the
deletion collector bypasses both. Sometimes the calls go through the
model, with a default implementation that simply forwards them and
sometimes they don't.
Also, though `base_manager_name` is becoming a public API, doing anything
useful with it requires overriding private, undocumented API methods.
----
So, the final breakdown, as I understand it at the moment, is as such:
* `default_manager`: makes it possible to expose additional features to
user code, in the general case.
* `related_manager`: makes it possible to expose additional features to
user code, on related fields.
* `base_manager`: must provide the unfiltered, untampered guarantee for
use in things such as '''dumpdata'''.
* unspecified API clinging to `base_manager` and living in undocumented,
private API methods of the manager or its queryset: can be used to
override low-level operations. That is, cases 1-3.
I would advocate splitting `base_manager` and the unspecified API, for
several reasons:
1. it would make the `base_manager`'s role and responsibilities clearly
bound and defined.
2. there is no reason for those methods to live on the manager/queryset
in the first place.
3. the fact they live on the manager(/manager's queryset) means every
manager has them, yet they are only ever used/usable on the base
manager(/manager's queryset).
4. the unspecified API is inconsistent and incomplete anyway, certainly
not mature enough to go public with the `base_manager`.
Namely, I'd completely remove `Manager._insert` and `QuerySet._update` and
let `Model._do_insert` and `Model._do_update` do the job.
[[br]]
I hope this didn't sound too bold, I'm trying my best to make my meaning
unambiguous, somewhat at the expense of political correctness.[[br]]
If you think this has some value, I can try to make a proof of concept
patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:6>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
If I understand correctly, this is an accepted ticket asking for design
changes in a new feature. Shouldn't it be a blocker?
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:7>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* cc: shaib (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:8>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
I was taking a shot at adding the `related_manager_name`.
I was wondering what it should default to, if unspecified: the
`base_manager` or `default_manager`? To me, `default_manager` makes more
sense, but `base_manager` is closer to former behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:9>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
@spectras, I already have a branch that introduce `related_manager_name`,
but there are a bunch of design decisions to make that I'm not comfortable
making this late in the 1.10 cycle. I don't want to introduce new APIs in
a rush.
Instead I'd like to adapt the current deprecation shim for
`use_for_related_field` so django-hvad can use it to work on Django 1.10,
and then introduce definitive APIs in Django 1.11.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:10>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
Alright, well maybe it's better to just summarize what is needed:
If user code did not change any manager, then:
→ hvad must override the default manager and the manager used by related
fields, for an hvad-enabled one.
And in all cases, even if the user did override a manager, then:
→ hvad must ensure the base manager is not hvad-enabled:
`isinstance(model._base_manager, TranslationManager)` is `False`).
This is because hvad-enabled manager is not suitable as a base manager.
Though it ''does'' provide the unfiltered-guarantee, it breaks because it
alters the behavior of the deletion collector.
I'll investigate whether it's possible to prevent that in a future
version, but that's probably a full rewrite with backward
incompatibilities, something not happening in time for Django 1.10.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:11>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
@spectras, could you please try
https://github.com/django/django/pull/6825.
Then on django-hvad:
- `Model._plain_manager = models.Manager()`.
- Instead of patching `Model._base_manager` patch
`Model._meta.base_manager_name = '_plain_manager'`.
- `TranslationManager.silence_use_for_related_fields_deprecation = True`
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:12>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
Passes the test suite. I'll fiddle with it a bit more, but unless I find
odd corner cases, it's good.
I like the way the PR only changes the deprecation path, allowing a clean
base to refine the API on next release.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:13>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
I guess the downside of this approach is that it'll trigger a migration
for each model. Think it's acceptable?
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:14>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
The migration process has been very well streamlined since South
functionality has been merged into Django, so yes — I'll make sure to
mention it in the release notes. That will be the opportunity to include
another change that requires a migration as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:15>
Django
Reporter: spectras | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
As the release is getting closer, I'm coming back here to know whether the
POC fix will be merged into stable/1.10.x?
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:16>
Django
Reporter: spectras | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Cleanup/optimization
Comment:
I expect it should be, but the patch is missing a test. If you want to try
writing one, that would be helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:17>
Django
Reporter: spectras | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
Can do.
I checked current tests, this corner case is not covered. I'll try to add
that today.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:18>
Django
Reporter: spectras | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by spectras):
I opened another pull request as I cannot update loic's one. It is here:
https://github.com/django/django/pull/6889
I'm open to all suggestions to improve it.
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:19>
Django
Reporter: spectras | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manager | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: spectras | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:21>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Keywords: manager | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f4afb85d7e900123fa8f88110adc011ab184e153" f4afb85d]:
{{{
#!CommitTicketReference repository=""
revision="f4afb85d7e900123fa8f88110adc011ab184e153"
Fixed #26749 -- Preserved behavior of use_for_related_field during
deprecation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:22>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: manager | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"39c25b772b566f77e94119f880fc6f99d563fcb6" 39c25b7]:
{{{
#!CommitTicketReference repository=""
revision="39c25b772b566f77e94119f880fc6f99d563fcb6"
[1.10.x] Fixed #26749 -- Preserved behavior of use_for_related_field
during deprecation.
Backport of f4afb85d7e900123fa8f88110adc011ab184e153 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26749#comment:23>