Hi all,
I don't know if using the queryset's hypothetical use_manager() method
would be the right approach to the reverse manager problem. In fact,
I'm not entirely happy with the current manager() proposal either: it
feels very much like an after-the-fact change to the default manager.
That's how it is implemented; first, you retrieve the default manager,
then you decide that's not what you wanted after all.
I don't think that is how selecting the reverse manager should feel.
Also, as long as we don't unify managers and querysets entirely, I
think that whatever returns to us the reverse manager should, in fact,
return a manager, i.e. we should be able to call all manager methods
defined on that manager, not just the @queryset ones (given that those
would be made available in the querysets derived from that manager).
Unfortunately, I'm not sure what would be a better way, or syntax, for
setting the reverse manager. In #3871, russellm suggested the manager()
method approach to select which manager is used. To me that feels very
much like an after-the-fact approach. I also allows you to chain
manager() calls which doesn't seem very useful at all:
{{{
reporter.article_set.manager('published_articles').manager('articles')
}}}
I think we need something different. Just brainstorming, these are some
thoughts how choosing reverse managers _could_ be implemented; this is
not yet related to any actual implementation, only some ideas:
1. use a dict-like approach on the current `related_name` reverse
manager, e.g.
{{{
reporter.article_set['published_articles']
}}}
This is not verbose, and probably not intuitive at all; it uses
array indexing for something that's not at all an array. However,
`article_set` could be thought of as a dict mapping manager names
to managers, so maybe it makes at least some sense after all.
2. add `related_name` to models.Manager as well, e.g.
{{{
class Article(models.Model):
reporter = models.ForeignKey(Reporter, related_name='article_set')
...
articles = models.Manager()
published_articles = PublishedManager(related_name='published_articles')
}}}
`reporter.article_set` returns the default manager as before, but
now `reporter.published_articles` returns related objects via
PublishedManager.
It would be an error to specify both `related_name` on the
ForeignKey and on the default manager. In fact, `related_name` on
the ForeignKey could be translated into the `related_name`
attribute on the default manager at model import time, keeping
`ForeignKey(related_name=(…))` for backwards compatibility as well
as for simple models which do not need a manager other than the
default models.Manager().
In fact, the more intuitive and unified way to write the above
example (with two managers) would be to move the `related_name`
from ForeignKey to the default manager:
{{{
class Article(models.Model):
reporter = models.ForeignKey(Reporter)
...
articles = models.Manager(related_name='article_set')
published_articles = PublishedManager(related_name='published_articles')
}}}
Non-default managers without the `related_name` attribute would not
be accessible in reverse relations.
3. make `related_name` on ForeignKey a dict mapping attributes to
manager names, e.g.
{{{
class Article(models.Model):
reporter = models.ForeignKey(Reporter, related_name={'article_set':
'_default_manager', 'published_articles': 'published_articles'})
...
articles = models.Manager()
published_articles = PublishedManager()
}}}
This doesn't seem sensible at all. Lots of boilerplate and overly
verbose.
4. add `reverse_managers` to ForeignKey, specifying which managers
(by name) should be made available on the reverse relation, e.g.
{{{
class Article(models.Model):
reporter = models.ForeignKey(Reporter, related_name='article_set',
reverse_managers=['published_articles'])
...
articles = models.Manager()
published_articles = PublishedManager()
}}}
This seems like a very arbitrary choice. Also, it demonstrates the
overlap between `related_name` and `reverse_managers`. Both
attributes are responsible for more or less the same thing:
defining the name of the reverse-mapping attribute on the related
class.
To summarize, I think that selecting custom managers in reverse
relations is a worthwhile effort. However, it would feel more natural
if you wouldn't need yet another method for selecting which manager is
chosen.
In fact, we don't have a special method for selecting non-default
forward managers, so the same property should hold for non-default
reverse managers: it all comes down to `Article.articles.(…)` and
`Article.published_articles.(…)` in the forward relation, so it should
be equally easy to write `reporter.articles.(…)` and
`reporter.published_articles.(…)` in the reverse case.
The question remains how this should be defined in the model. Simply
sticking _all_ reverse descriptors in the related class doesn't seem
like a sensible thing to do; this was the original approach in #3871
and was dismissed by russellm.
That said, I lean towards explicitly naming the managers which should
be made available, a.k.a. idea #2 from my list above.
This way, you also only had to define the backwards name once and could
use it for all reverse relations referring to that model; which makes
sense somewhat as it's still the same related model in either case,
with the same managers (contrast this to adding an attribute to the
ForeignKey that only influences the relation between two particular
models). On the other hand, this could easily introduce name clashes
between forward and reverse attributes.
Any thoughts?
Sebastian.
On Sat, 14 Jan 2012 22:15:33 +0200
Kääriäinen Anssi <[email protected]> wrote:
> Just a quick thought: you should check out the work done for allowing use of
> manager methods in querysets (thread "RFC: query methods"). It seems that
> work does have some overlap with this feature. The patch for #3871 implements
> .manager('manager_name') for reverse relation managers, and there was some
> discussion for allowing .use_manager('manager_name') for querymethods.
> .use_manager() is not going to be in the query methods patch. I haven't
> looked #3871 in detail, but maybe the work done for query methods would make
> the #3871 patch easier to implement?
>
> The idea would be to issue: .use_manager(wanted_manager).all() in the
> .manager() method. The first method call would change the base manager to
> use, the second (.all) call would make it return a queryset, so that you
> would not have the .clear and .remove methods available. This might be a
> stupid idea, but maybe worth a try? The .use_manager() call would not need to
> exist on queryset level.
>
> 1.4 is feature frozen if I am not mistaken, so this would be 1.5 stuff.
>
> - Anssi
> ________________________________________
> From: [email protected] [[email protected]]
> On Behalf Of Sebastian Goll [[email protected]]
> Sent: Saturday, January 14, 2012 21:35
> To: [email protected]
> Subject: Re: Custom managers in reverse relations
>
> Hi all,
>
> My latest post to the list seems to have been lost in the pre-Christmas
> storm. Sorry for that!
>
>
> The issue of picking which custom manager is used in resolving reverse
> relations still stands. Let my give you an example why this is useful:
>
> {{{
> class Reporter(models.Model):
> ...
>
> class Article(models.Model):
> reporter = models.ForeignKey(Reporter)
> ...
> articles = models.Manager()
> published_articles = PublishedManager()
> }}}
>
> We put some thought into designing PublishedManager. Maybe it needs to
> do some things in addition to simply checking a flag, who knows. The
> thing is: right now, we simply cannot make use of this manager when
> looking up a reporter's articles: with `reporter.article_set` we
> always get _all_ articles. [1]
>
>
> Now we have two options: doing the filtering manually, on the returned
> queryset, or specify that we want to use PublishedManager, accessible
> through the `published_articles` attribute of the Article class.
>
> The latter is implemented by the patches in ticket #3871:
>
> https://code.djangoproject.com/ticket/3871
>
>
> Does this seem like a good idea? Should it even be possible to specify
> which custom manager is used for reverse relations? Or, am I missing
> something and this is already possible in some other way?
>
> Since I'm looking forward to seeing this implementation in Django 1.4,
> I ask for your input on the matter.
>
> Thanks!
> Sebastian.
>
> [1] In fact, that's not entirely true: we get whatever is returned by
> the _default_ manager of the Article class. This seems like an
> arbitrary choice: it's not a "plain" manager that always returns all
> related instances, it's whatever we picked as the default manager.
>
>
> On Fri, 23 Dec 2011 21:56:24 +0100
> Sebastian Goll <[email protected]> wrote:
>
> > Hi all,
> >
> > I'd like to draw your attention to long-open ticket #3871 [1].
> >
> > The idea is to let ORM users choose which custom manager to use for reverse
> > "many" relations, i.e. reverse foreign key (…_set) as well as forward and
> > reverse many-to-many relations.
> >
> > There are several proposed patches to this ticket, the latest was added by
> > me a week ago. The current implementation adds a "manager()" method to the
> > reverse manager which allows you to pick a manager different from the
> > default one on the related model. All changes are entirely
> > backwards-compatible – if you don't call the "manager()" method, everything
> > is as before, i.e. the default manager is used to look up related model
> > instances.
> >
> >
> > During my review of the previous patch I found that it doesn't apply
> > cleanly to trunk, as well as some concerns with regard to the general
> > approach of the implementation.
> >
> > Therefore, I wrote an alternative patch which is currently awaiting review.
> > Since I wrote that patch, I cannot review it myself. If you can spare some
> > time, maybe you can take a look at it and if you feel the current approach
> > is okay, bump the ticket to "ready for check-in".
> >
> > Of course feel free to raise any concerns you might have.
> >
> > Regards,
> > Sebastian.
> >
> > PS: Merry X-Mas and whatnot! :D
> >
> > [1] https://code.djangoproject.com/ticket/3871
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.
> To post to this group, send email to [email protected].
> To unsubscribe from this group, send email to
> [email protected].
> For more options, visit this group at
> http://groups.google.com/group/django-developers?hl=en.
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
