Custom managers in reverse relations
Sebastian Goll
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
Sebastian Goll
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.
Kääriäinen Anssi
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: django-d...@googlegroups.com [django-d...@googlegroups.com] On Behalf Of Sebastian Goll [sebasti...@gmx.de]
Sent: Saturday, January 14, 2012 21:35
To: django-d...@googlegroups.com
Subject: Re: Custom managers in reverse relations
Hi all,
{{{
class Reporter(models.Model):
...
https://code.djangoproject.com/ticket/3871
Thanks!
Sebastian.
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Sebastian Goll
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.
Tai Lee
relations or any manager/queryset (either by getting and then throwing
away the default manager, or some other method), or applying manual
filters to achieve the same result, there is another alternative.
Instead of:
{{{
reporter.article_set.manager('published_articles')
}}}
Do:
{{{
Article.published_articles.filter(reporter=reporter)
}}}
This is probably not ideal, but is still easy to read and should be an
easy work-around in most cases.
Regarding option 1-4 above, only option 2 appeals to me. I prefer my
example above to option 1, and option 3-4 are a bit confusing. But
even for option 2, I think it will fall down when you have multiple
foreign keys to the same model. This is why we have `related_name` on
`ForeignKey` instead of `Manager`, and why it should not be moved to
`related_name` on `Manager`.
Cheers.
Tai.
>
> read more »