Re: #14891, a.k.a. "related managers don't work how we claim they do"
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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 04/19/2011 04:58 AM, Johannes Dollinger wrote: >> 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 > 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. 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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 04/18/2011 02:59 PM, Carl Meyer wrote: 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? -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
Hi Johannes, 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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 04/18/2011 04:35 PM, Ivan Sagalaev wrote: > 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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 04/18/2011 11:45 AM, Johannes Dollinger wrote: 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. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 04/18/2011 11:16 AM, Carl Meyer wrote:
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…
--
You received this message because you are subscribed to the Google Groups "Django
developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
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. And it would be a step towards discouraging use of multiple managers. __ Johannes -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
Hi Ivan,
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
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 04/16/2011 04:30 PM, Carl Meyer wrote: 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. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #14891, a.k.a. "related managers don't work how we claim they do"
On 17/04/11 00:30, Carl Meyer 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. 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/ -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
#14891, a.k.a. "related managers don't work how we claim they do"
Hi all, 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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
