#14891, a.k.a. "related managers don't work how we claim they do"
Carl Meyer
Our documentation on "automatic managers" [1] and related managers [2]
is quite clear that automatically-created managers for related-objects
access will be plain-vanilla Manager instances, unless you set
"use_for_related_fields = True" on the custom Manager subclass that you
use as a default manager.
Unfortunately, this documentation is not true; near as I can tell, it
never has been since it was introduced.
Vanilla managers are used as documented in a few places: OneToOneField
related-object access, and traversing FKs and M2Ms for internal
cascade-deletion purposes. But in general, related-object access for
reverse-FKs and M2Ms, contrary to the documentation, will _always_ use
your default manager (actually, a dynamic subclass of it) and will
respect any restrictions in the get_query_set method of your default
manager, even if "use_for_related_fields" is absent or explicitly set to
False on your Manager subclass.
I have more details, and the fix (if we want it) on ticket #14891 [3]
The dilemma here is that fixing this bug is quite likely to break
significant amounts of code that is relying on it. As evidence of that,
when fixing it I had to also fix two places in Django's own test suite
where we assumed that a custom default manager would be used for
related-object access, without setting use_for_related_fields=True on
the manager.
So - do we (A) fix the behavior to match our documented semantics, note
it in the release notes, and hope for the best? Or (B) bow to
backwards-compatibility and change the documentation to match the actual
behavior? Or (C) find some middle ground (a deprecation path for the
current behavior)?
Personally, I don't find option (B) very appealing. The current behavior
isn't crippling (you can generally work around it if you need to by just
not making your custom manager the default one), but it is inconsistent,
hard to explain, and means "use_for_related_fields" is quite
misleadingly named.
Input welcome,
Carl
[1]
http://docs.djangoproject.com/en/dev/topics/db/managers/#set-use-for-related-fields-when-you-define-the-class
[2]
http://docs.djangoproject.com/en/dev/topics/db/managers/#managers-for-related-objects
[3] http://code.djangoproject.com/ticket/14891
Luke Plant
> So - do we (A) fix the behavior to match our documented semantics, note
> it in the release notes, and hope for the best? Or (B) bow to
> backwards-compatibility and change the documentation to match the actual
> behavior? Or (C) find some middle ground (a deprecation path for the
> current behavior)?
I vote for A - fix the bug.
Luke
--
"In my opinion, we don't devote nearly enough scientific research
to finding a cure for jerks." (Calvin and Hobbes)
Luke Plant || http://lukeplant.me.uk/
Ivan Sagalaev
> in general, related-object access for
> reverse-FKs and M2Ms, contrary to the documentation, will _always_ use
> your default manager (actually, a dynamic subclass of it)
It kind of makes sense. You don't want your deleted items to appear in
results just because you conveniently get a queryset from a related
manager. In other words, `topic.article_set.all()` should always yield
the same data as `Article.objects.filter(topic=topic)`.
But I would agree that in this case:
> even if "use_for_related_fields" is absent or explicitly set to
> False on your Manager subclass.
� the default manager should not be used as a base class. Fixing just
this would be the best option because it retains current behavior by
default while allowing other uses if needed.
Carl Meyer
On 04/18/2011 01:07 PM, Ivan Sagalaev wrote:
>> even if "use_for_related_fields" is absent or explicitly set to
>> False on your Manager subclass.
>
> � the default manager should not be used as a base class. Fixing just
> this would be the best option because it retains current behavior by
> default while allowing other uses if needed.
By "just this" I presume you actually mean just the second half of what
you quoted ("explicitly set to False")? In other words, you're proposing
to make use_for_related_fields work as advertised, but make it default
to True instead of False?
If I were designing this from scratch, I think I would also prefer
use_for_related_fields to default to True. But that would be a
non-bugfix change that would impact backwards-compatibility for things
that currently do properly respect use_for_related_fields. For example,
people might suddenly start getting ObjectNotFound from a OneToOneField
descriptor where previously they got the "hidden-by-default-manager"
object back. So I think we could only do that with some kind of
deprecation path for the current default of False.
Carl
Carl Meyer
On 04/18/2011 12:47 PM, Luke Plant wrote:
>> So - do we (A) fix the behavior to match our documented semantics, note
>> it in the release notes, and hope for the best? Or (B) bow to
>> backwards-compatibility and change the documentation to match the actual
>> behavior? Or (C) find some middle ground (a deprecation path for the
>> current behavior)?
>
> I vote for A - fix the bug.
That's my leaning, too.
The biggest pain this would cause would be for people using third-party
custom managers that don't set use_for_related_fields, and relying on it
being used for related fields anyway. Since use_for_related_fields
currently must be set as a class attribute, not an instance attribute,
they would either need to subclass the third-party manager just in order
to add use_for_related_fields=True, or they would need to monkeypatch it on.
In my mind, this reveals a different problem: the author of a custom
Manager subclass is not necessarily the best person to decide whether
that manager should be used for related fields in a particular use of
it. Making Manager.__init__ accept a use_for_related_fields argument is
problematic for backwards-compat with existing subclasses, but we could
respect it if set directly as an instance attribute:
objects = MyCustomManager()
objects.use_for_related_fields = True
I may open that as a separate ticket.
Carl
Johannes Dollinger
Am 17.04.2011 um 01:30 schrieb Carl Meyer:
> So - do we (A) fix the behavior to match our documented semantics, note
> it in the release notes, and hope for the best? Or (B) bow to
> backwards-compatibility and change the documentation to match the actual
> behavior? Or (C) find some middle ground (a deprecation path for the
> current behavior)?
I'd vote for (C).
Deprecate `use_for_related_fields` and always use the default manager for related managers. Then add the possibility to specify custom mangers for individual relations:
ForeignKey(Foo, related_manager=RSpecialManager)
ManyToManyField(Foo, manager=SpecialManger, related_manager= RSpecialManager)
More fine grained control would especially be useful for subclasses of ForeignKey and ManyToManyField fields.
It comes at the expense of verbosity, but it appears to be a rarely used feature (given that the bug was discovered only now). And thus, explicitness might actually be a good idea.
<pet-peeve-rant>And it would be a step towards discouraging use of multiple managers.</pet-peeve-rant>
__
Johannes
Ivan Sagalaev
> By "just this" I presume you actually mean just the second half of what
> you quoted ("explicitly set to False")? In other words, you're proposing
> to make use_for_related_fields work as advertised, but make it default
> to True instead of False?
Not exactly� I mean that when use_for_related_fields is set explicitly
Django should respect it. Right now, as I understand from your first
mail, it doesn't work as False when set to False. So I agree that this
should definitely be fixed.
What I was saying is that when this attribute is not set current
behavior does make sense:
- use default manager as a base for *_set managers
- use pure manager as a base for OneToOne and parent lookups
However it can't be described with neither "False by default" nor "True
by default". I think it's fine and we could just thoroughly document
this behavior.
P.S. Sorry for being sloppy with my first reply�
Ivan Sagalaev
> I'd vote for (C).
> Deprecate `use_for_related_fields` and always use the default manager for related managers. Then add the possibility to specify custom mangers for individual relations:
>
> ForeignKey(Foo, related_manager=RSpecialManager)
> ManyToManyField(Foo, manager=SpecialManger, related_manager= RSpecialManager)
I like this one too! Except for the "always use the default manager"
part which, as Carl noted elsewhere in the thread, is not how it works
right now. Different relation lookups use different defaults. But your
idea expresses this even better.
Carl Meyer
> Not exactly� I mean that when use_for_related_fields is set explicitly
> Django should respect it. Right now, as I understand from your first
> mail, it doesn't work as False when set to False. So I agree that this
> should definitely be fixed.
>
> What I was saying is that when this attribute is not set current
> behavior does make sense:
>
> - use default manager as a base for *_set managers
> - use pure manager as a base for OneToOne and parent lookups
>
> However it can't be described with neither "False by default" nor "True
> by default". I think it's fine and we could just thoroughly document
> this behavior.
Hmm. Why does it make sense for OneToOneField lookups to behave
differently from *_set managers? If I've got a default manager that
hides "deleted" objects, for instance: why should deleted objects by
default "not exist" when I traverse a reverse FK, but "exist" when I
traverse a OneToOneField?
Simply from a complexity-of-documentation standpoint I don't like the
idea that the effective "default" for use_for_related_fields would be
neither True nor False, so to counterbalance that I'd want a pretty
strong case for why it's the best option.
Carl
Carl Meyer
On 04/18/2011 01:45 PM, Johannes Dollinger wrote:
> Deprecate `use_for_related_fields` and always use the default manager
> for related managers. Then add the possibility to specify custom
> mangers for individual relations:
>
> ForeignKey(Foo, related_manager=RSpecialManager) ManyToManyField(Foo,
> manager=SpecialManger, related_manager= RSpecialManager)
>
> More fine grained control would especially be useful for subclasses
> of ForeignKey and ManyToManyField fields. It comes at the expense of
> verbosity, but it appears to be a rarely used feature (given that the
> bug was discovered only now). And thus, explicitness might actually
> be a good idea.
Do you have real-world use-cases in mind that require per-relation
manager configuration? I can't think of any uses I've run across that
weren't workable with a single default manager used by all relations.
Carl
Ivan Sagalaev
> Hmm. Why does it make sense for OneToOneField lookups to behave
> differently from *_set managers? If I've got a default manager that
> hides "deleted" objects, for instance: why should deleted objects by
> default "not exist" when I traverse a reverse FK, but "exist" when I
> traverse a OneToOneField?
OK, may be not reverse OneToOne� What I mean is that although it seems
natural to treat all relations equally they are actually used for
different use cases where different defaults make sense. For example:
- Reverse many-to-one (topic.article_set) access is used to access a
(limited) list of children which is expected to behave as any other list
of such objects and hence should use the default manager.
- Direct one-to-one or many-to-one (article.topic or profile.user)
access is used to access a parent object and in most real cases it
doesn't make sense if it's absent for example. Usually you're just
dealing with a "deleted" child accessing its "deleted" parent which is
OK. In this case it makes sense to use a pure manager to build the relation.
As for reverse one-to-one I'm really not sure because I can't recall any
real example to lay upon. Speaking about documentation simplicity (which
directly influences sanity of its readers) it can be made as simple as
that:
- pure manager is used whenever there is a clear child-parent realation
(direct OneToOne and ForeignKey access)
- all other relations use default managers
- explicit use_for_related_fields overrides default behavior
What do you think?
Johannes Dollinger
The only time I used a custom manager for a relationship was for a TagField – making .add() accept bare strings.
It's been more useful for me to use a custom descriptor, e.g. this TagField's descriptor delegates __set__ to an .assign() method on the manager.
Another use-case for custom descriptors was an experimental ListField, where the related manager behaves like a list of related objects (e.g. supports slicing, iteration, assignment).
I don't think these examples necessarily need a public API, although it'd be nice to reduce the the dependencies on internal API in my code.
Let me turn the question around: do you have real-world use-case that benefits from use_for_related_fields? Especially one that cannot be solved more cleanly with custom queryset methods?
Carl Meyer
>> Do you have real-world use-cases in mind that require per-relation
>> manager configuration? I can't think of any uses I've run across
>> that weren't workable with a single default manager used by all
>> relations.
>
> The only time I used a custom manager for a relationship was for a
> for me to use a custom descriptor, e.g. this TagField's descriptor
> delegates __set__ to an .assign() method on the manager. Another
> use-case for custom descriptors was an experimental ListField, where
> the related manager behaves like a list of related objects (e.g.
> supports slicing, iteration, assignment).
>
> I don't think these examples necessarily need a public API, although
> it'd be nice to reduce the the dependencies on internal API in my
> code.
Yeah... it'd be interesting to see a full proposal for a public API for
use-cases like this, but my sense is that some advanced uses are always
going to depend on implementation details.
> Let me turn the question around: do you have real-world use-case that
> benefits from use_for_related_fields? Especially one that cannot be
> solved more cleanly with custom queryset methods?
Depends whether you mean use_for_related_fields=True or
use_for_related_fields=False. For the former, sure - I have utility
managers that add extra chainable methods both to the manager and its
returned queryset, and I'd like to have the added method(s) available
everywhere, including on related-object managers.
I haven't yet had a case where I wanted a manager to be the default
manager for normal operation, but wanted to specify
use_for_related_fields=False.
Carl
Carl Meyer
On 04/19/2011 01:47 AM, Ivan Sagalaev wrote:
> OK, may be not reverse OneToOne� What I mean is that although it seems
> natural to treat all relations equally they are actually used for
> different use cases where different defaults make sense. For example:
>
> - Reverse many-to-one (topic.article_set) access is used to access a
> (limited) list of children which is expected to behave as any other list
> of such objects and hence should use the default manager.
>
> - Direct one-to-one or many-to-one (article.topic or profile.user)
> access is used to access a parent object and in most real cases it
> doesn't make sense if it's absent for example. Usually you're just
> dealing with a "deleted" child accessing its "deleted" parent which is
> OK. In this case it makes sense to use a pure manager to build the
> relation.
>
> As for reverse one-to-one I'm really not sure because I can't recall any
> real example to lay upon. Speaking about documentation simplicity (which
> directly influences sanity of its readers) it can be made as simple as
> that:
>
> - pure manager is used whenever there is a clear child-parent realation
> (direct OneToOne and ForeignKey access)
> - all other relations use default managers
> - explicit use_for_related_fields overrides default behavior
>
> What do you think?
I do find this pretty convincing. I don't see a good reason why a
default manager should not be used by default for many-related access.
In other words, I think the current behavior is probably better default
behavior more of the time than the documented behavior would be.
So I guess I've changed my position. I'm leaning towards fixing the docs
to match what we currently do - and I'm also feeling more and more like
the "use_for_related_fields" Manager class attribute is an ugly hack,
and open to looking at patches to replace it with per-relation override
of the default manager-selection.
Carl