Use custom managers in reverse relations

[EspenG]

This is my case:
I got a Article model with two managers, the default "objects" and a custom one "published_objects". My Article model also got a foreign key to a Category model.
From the Category model I'm then able to write a_category.article_set.do_something(). The problem is then that this only allows me to use the default manager "objects", and not "published_objects".

More general:
It's not possible to override witch manager to use using reverse relations like foo.xxx_set

Possible syntax's for doing this:
On IRC someone came up with foo.xxx_set_manager1.all(), foo.xxx_set_manager2.all(), that is foo.xxx.set_<manager_name>.all().
One other way I came up with is using foo.xxx_set for the default manager, and then if you want to access a custom manager you could do something like this: foo.xxx_set.managers.my_custom_manager.all().
Others?

Hopefully someone got a great ide on how this should be done.

[Jeff Forcier <reg@…>]

I believe someone also came up with a suggestion of selecting the Manager for a given ForeignKey relationship via the FK definition itself - e.g. foo = ForeignKey(Foo,manager=Foo.manager2). I'm thinking something "clever" like ForeignKey(Foo.manager2) would be nicely succinct and could preserve backwards compatibility by assuming the default Manager if you just pass in the Foo class.

However, that sort of syntax/implementation then locks you into a single manager; while that's really no worse than how things currently stand, the foo.xxx_set_<manager_name> version would be more flexible, if more verbose. Ideally there'd be a way to get the best of both worlds.

[EspenG]

I have made a patch witch fixes this ticket and it got 100% backwards compatibility.

Let's say you got two managers in the related model (remote), objects and test_objects, you can access them remotely like this: foo.remote_setobjects and foo.remote_settest_objects, while foo.remote_set stil uses the default manager.

Please test the code, and if this is the way you guys like it I will also write documentation and tests.

Espen

[EspenG]

Oh, I forgot to say that the patch is only for M2M fields so far.

Espen

[EspenG <espen@…>]

Now also support ForeignKeys

[EspenG <espen@…>]

Ok, now I have been working on this ticket for a while and I have found a big problem. The patch itself works fine _some_ times, and I think I have figured out why it only works some times and not always.

At first I could not understand why everything worked when I tried to to what the patch does manually in the python shell but only sometimes when I used the patch on my system. Then I noticed that it worked if I rearranged the order the different models where in it worked. This then led me to the theory that in order for me to parse the different models (to find managers) i first have to wait until all the other initializing of the models are done, though I'm not a 100% sure about this. It would be nice if someone could confirm this.

Espen

[Simon G. <dev@…>]

Marked as Design Dec. Needed.

Espen - please raise this issue on Django-developers, you're more likely to get a response there :)

--Simon

[Russell Keith-Magee]

Accepted on principle, but needs a cleaner API. EspenG's proposal is close; however, rather than seeing managers acquire an attribute for each of the model's manager, it would be better for a manager to have a method that allows it to return a different manager on the model - e.g., Author.article_set.manager('published') would give you the 'published' manager for Articles.

[v1v3kn]

Patch that fixes this by letting the user choose the manager by a .managers() function.

[v1v3kn]

Added a patch that provides this functionality, based on Russell's suggestion,
It allows you to do

#author is an Author object instance
author.article_set.managers('published_objects') # returns the published_objects manager instead of the default manager

[v1v3kn]

updated patch with docs

[v1v3kn]

patch with docs

[Sebastian Goll]

I just ran into this issue described here myself (very closely related to the given introductory example, though in my case it's public events versus all events, possibly including private ones) which I want to access in the reverse …_set relation.

So I'd love to see these additions go to trunk. How are the chances of this happening in time for Django 1.4?

[Sebastian Goll]

What is the current status on this ticket? The deadline for non-trivial new features for 1.4 is the upcoming weekend (Dec 17–18). To me, the latest patch in this ticket seems essentially complete – it has documentation and tests, plus the actual changes to the code are not terribly large nor convoluted.

What is the next step in getting a developer involved into reviewing this and check-in to trunk?

[Aymeric Augustin]

As explained in the ​contributing guide, at this point, this ticket needs to be reviewed (by anyone) and marked as ready for checkin.

[Sebastian Goll]

Thanks for the hint on the contributing guide. I wasn't aware that everybody could (and should) mark appropriate patches as ready for check-in. During my review I noticed that the latest patch in this ticket (3871-patch-with-docs.diff) doesn't apply cleanly to trunk. Also, I think the implementation, by monkey-patching the managers() method to the related managers, is not ideal.

Therefore, I wrote an alternative patch which I am attaching. The key differences to the last patch are:

• Method name manager() instead of managers(). Seems more appropriate for selecting a single manager (also, suggested by russellm).
• Checks in place for disallowing the remove() and clear() methods where they might show unexpected—and potentially dangerous—behavior, i.e. data loss. Namely, we do not check (and cannot do so without performance penalty database-wise) if the selected objects for removal are actually within the domain of the related manager. The only exception to this is clear() on a simple reverse foreign-key manager, which works as expected.
• Structure of the implementation. manager() is defined in the [Many]RelatedManager class alongside all other methods such as add(), remove() etc. No monkey-patching necessary.
• More elaborate tests. All three kinds of relevant related managers are tested: reverse foreign-key, reverse many-to-many, and forward many-to-many. Also additional tests for checking for unknown managers, and availability and behavior of the remove() and clear() methods.

Since I wrote the new patch, I cannot myself review it. Thus, I ask for somebody else to take a look at it. The test suite passes, so there should be no regressions. All changes are backwards-compatible: the only addition is the manager() method.

[Sebastian Goll]

Revised patch.

[myusuf3]

This patch no longer applies

[Sebastian Goll]

Replying to myusuf3:

This patch no longer applies

What exactly do you mean by that? The latest patch r17204-custom-reverse-managers.diff still applies perfectly cleanly to trunk, as of r17575:

$ svn up
At revision 17575.
$ patch -p0 <r17204-custom-reverse-managers.diff
patching file docs/ref/models/relations.txt
patching file django/db/models/fields/related.py
patching file tests/modeltests/custom_managers/tests.py
patching file tests/modeltests/custom_managers/models.py

[myusuf3]

Replying to sebastian:

Replying to myusuf3:

This patch no longer applies

What exactly do you mean by that? The latest patch r17204-custom-reverse-managers.diff still applies perfectly cleanly to trunk, as of r17575:

$ svn up
At revision 17575.
$ patch -p0 <r17204-custom-reverse-managers.diff
patching file docs/ref/models/relations.txt
patching file django/db/models/fields/related.py
patching file tests/modeltests/custom_managers/tests.py
patching file tests/modeltests/custom_managers/models.py

sorry I was using the wrong -p level.

[anonymous]

What's the scoop here? It obviously didn't make it into 1.4 but it seems like a clear target for 1.5. This is a pretty basic thing to want to do with Django relations; I was surprised when I discovered it didn't exist.

[anonymous]

Ditto

[Simon Charette]

I would go ahead and update the patch to mention the feature was added in 1.5 but I think it's feature frozen.

[ntucker]

I would very much appreciate this being added. :)

[loic84]

Proof of concept following discussion with @akaariai on IRC:

​https://github.com/loic/django/compare/manager.__call__

[Anssi Kääriäinen]

The patch in r17204-custom-reverse-managers.diff​ uses MyModel.m2mfield.manager('somemanager') while the call based syntax is MyModel.m2mfield('somemanager'). I am not sure which one is better. Opinions?

I think either the __call__ or .manager() based syntax should be adopted. Some solution would be welcome to this long standing issue.

[loic84]

PR ​https://github.com/django/django/pull/1630.

[Anssi Kääriäinen]

So, latest & ready for checkin idea is to go with MyModel.somerelation(manager='somemanager').filter... syntax. Last call for API vetoes. I'll try to get this into master tonight.

[Anssi Kääriäinen]

Seems like trac didn't pick up the commit. I merged this in 04a2a6b0f9cb6bb98edfe84bf4361216d60a4e38.

[jbg@…]

Since this was committed, I get "KeyError: 'manager'" at django/db/models/fields/related.py:379 when accessing a reverse relation from a template.

I have bisected the issue to 04a2a6b0f9cb6bb98edfe84bf4361216d60a4e38.

Full traceback:

Traceback (most recent call last):
  File "/home/.../virtualenv/lib/python3.3/site-packages/waitress/channel.py", line 332, in service
    task.service()
  File "/home/.../virtualenv/lib/python3.3/site-packages/waitress/task.py", line 173, in service
    self.execute()
  File "/home/.../virtualenv/lib/python3.3/site-packages/waitress/task.py", line 388, in execute
    app_iter = self.channel.server.application(env, start_response)
  File "/home/.../django/django/core/handlers/wsgi.py", line 206, in __call__
    response = self.get_response(request)
  File "/home/.../django/django/core/handlers/base.py", line 196, in get_response
    response = self.handle_uncaught_exception(request, resolver, sys.exc_info())
  File "/home/.../django/django/core/handlers/base.py", line 114, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/.../django/django/db/transaction.py", line 360, in inner
    return func(*args, **kwargs)
  File "/home/.../...py", line 61, in ...
    return render(request, '....html', {'...': ...})
  File "/home/.../django/django/shortcuts/__init__.py", line 45, in render
    return HttpResponse(loader.render_to_string(*args, **kwargs),
  File "/home/.../django/django/template/loader.py", line 169, in render_to_string
    return t.render(context_instance)
  File "/home/.../django/django/template/base.py", line 141, in render
    return self._render(context)
  File "/home/.../django/django/template/base.py", line 135, in _render
    return self.nodelist.render(context)
  File "/home/.../django/django/template/base.py", line 830, in render
    bit = self.render_node(node, context)
  File "/home/.../django/django/template/base.py", line 844, in render_node
    return node.render(context)
  File "/home/.../django/django/template/loader_tags.py", line 122, in render
    return compiled_parent._render(context)
  File "/home/.../django/django/template/base.py", line 135, in _render
    return self.nodelist.render(context)
  File "/home/.../django/django/template/base.py", line 830, in render
    bit = self.render_node(node, context)
  File "/home/.../django/django/template/base.py", line 844, in render_node
    return node.render(context)
  File "/home/.../django/django/template/loader_tags.py", line 62, in render
    result = block.nodelist.render(context)
  File "/home/.../django/django/template/base.py", line 830, in render
    bit = self.render_node(node, context)
  File "/home/.../django/django/template/base.py", line 844, in render_node
    return node.render(context)
  File "/home/.../django/django/template/defaulttags.py", line 507, in render
    six.iteritems(self.extra_context))
  File "/home/.../django/django/template/defaulttags.py", line 506, in <genexpr>
    values = dict((key, val.resolve(context)) for key, val in
  File "/home/.../django/django/template/base.py", line 586, in resolve
    obj = self.var.resolve(context)
  File "/home/.../django/django/template/base.py", line 722, in resolve
    value = self._resolve_lookup(context)
  File "/home/.../django/django/template/base.py", line 776, in _resolve_lookup
    current = current()
  File "/home/.../django/django/db/models/fields/related.py", line 379, in __call__
    manager = getattr(self.model, kwargs.pop('manager'))

[loic84]

PR ​https://github.com/django/django/pull/1692.

[Tim Graham <timograham@…>]

In d847ddfe1d4475e768c547b17642d3ed0894d54e:

Fixed #3871 -- Fixed regression introduced by 04a2a6b.

Added do_not_call_in_templates=True attribute to RelatedManagers
to prevent them from being called.

Thanks jbg@ for the report.

[Anssi Kääriäinen <akaariai@…>]

In 17c3997f6828e88e4646071a8187c1318b65597d:

Fixed #21169 -- Reworked RelatedManager methods use default filtering

The remove() and clear() methods of the related managers created by
ForeignKey, GenericForeignKey, and ManyToManyField suffered from a
number of issues. Some operations ran multiple data modifying queries without
wrapping them in a transaction, and some operations didn't respect default
filtering when it was present (i.e. when the default manager on the related
model implemented a custom get_queryset()).

Fixing the issues introduced some backward incompatible changes:

• The implementation of remove() for ForeignKey related managers changed from a series of Model.save() calls to a single QuerySet.update() call. The change means that pre_save and post_save signals aren't called anymore.

• The remove() and clear() methods for GenericForeignKey related managers now perform bulk delete so Model.delete() isn't called anymore.

• The remove() and clear() methods for ManyToManyField related managers perform nested queries when filtering is involved, which may or may not be an issue depending on the database and the data itself.

Refs. #3871, #21174.

Thanks Anssi Kääriäinen and Tim Graham for the reviews.