Opened 8 years ago

Closed 22 months ago

Last modified 20 months ago

#3871 closed New feature (fixed)

Use custom managers in reverse relations

Reported by: EspenG Owned by: v1v3kn
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: paolo@…, espen@…, v1v3kn, sebastian, r.dav.lc@…, daevaorn@…, Twidi, jdunck@…, loic@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (6)

3871.diff (3.1 KB) - added by EspenG 8 years ago.
3871.2.diff (4.7 KB) - added by EspenG <espen@…> 8 years ago.
Now also support ForeignKeys
3871-patch.diff (5.6 KB) - added by v1v3kn 4 years ago.
Patch that fixes this by letting the user choose the manager by a .managers() function.
3871-patch-with-docs (7.0 KB) - added by v1v3kn 4 years ago.
updated patch with docs
3871-patch-with-docs.diff (7.0 KB) - added by v1v3kn 4 years ago.
patch with docs
r17204-custom-reverse-managers.diff (16.7 KB) - added by sebastian 4 years ago.
Revised patch.

Download all attachments as: .zip

Change History (46)

comment:1 Changed 8 years ago by Jeff Forcier <reg@…>

  • Cc reg@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 Changed 8 years ago by Paolo Dina <paolo@…>

  • Cc paolo@… added

comment:3 Changed 8 years ago by anonymous

  • Cc espen@… added

comment:4 Changed 8 years ago by EspenG

  • Has patch set
  • Needs documentation set
  • Needs tests set

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

Changed 8 years ago by EspenG

comment:5 Changed 8 years ago by EspenG

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

Espen

Changed 8 years ago by EspenG <espen@…>

Now also support ForeignKeys

comment:6 Changed 8 years ago by EspenG <espen@…>

  • Has patch unset
  • Patch needs improvement set

comment:7 Changed 8 years ago by 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

comment:8 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed

Marked as Design Dec. Needed.

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

--Simon

comment:9 Changed 8 years ago by SmileyChris

  • Component changed from Core framework to Database wrapper

comment:10 Changed 5 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

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.

comment:11 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

Changed 4 years ago by v1v3kn

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

comment:12 Changed 4 years ago by v1v3kn

  • Easy pickings unset
  • Has patch set
  • Needs tests unset
  • Owner changed from nobody to v1v3kn
  • Status changed from new to assigned
  • UI/UX unset

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

Changed 4 years ago by v1v3kn

updated patch with docs

comment:13 Changed 4 years ago by v1v3kn

  • Cc v1v3kn added
  • Needs documentation unset
  • Patch needs improvement unset

Changed 4 years ago by v1v3kn

patch with docs

comment:14 Changed 4 years ago by sebastian

  • Cc sebastian added

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?

comment:15 Changed 4 years ago by bitprophet

  • Cc reg@… removed

comment:16 Changed 4 years ago by rebus

  • Cc r.dav.lc@… added

comment:17 Changed 4 years ago by sebastian

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?

comment:18 Changed 4 years ago by aaugustin

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

comment:19 Changed 4 years ago by sebastian

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.

Changed 4 years ago by sebastian

Revised patch.

comment:20 Changed 4 years ago by alexkoshelev

  • Cc daevaorn@… added

comment:21 follow-up: Changed 3 years ago by myusuf3

  • Patch needs improvement set

This patch no longer applies

comment:22 in reply to: ↑ 21 ; follow-up: Changed 3 years ago by sebastian

  • Patch needs improvement unset

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

comment:23 in reply to: ↑ 22 Changed 3 years ago by 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.

comment:24 Changed 3 years ago by Twidi

  • Cc Twidi added

comment:25 Changed 3 years ago by jdunck

  • Cc jdunck@… added

comment:26 Changed 3 years ago by 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.

comment:27 Changed 3 years ago by anonymous

Ditto

comment:28 Changed 3 years ago by charettes

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

comment:29 Changed 2 years ago by ntucker

I would very much appreciate this being added. :)

comment:30 Changed 2 years ago by loic84

  • Cc loic@… added

Proof of concept following discussion with @akaariai on IRC:

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

comment:31 Changed 2 years ago by akaariai

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.

comment:33 Changed 23 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:34 Changed 23 months ago by akaariai

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.

comment:35 Changed 23 months ago by akaariai

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:36 Changed 22 months ago by 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'))

comment:37 Changed 22 months ago by loic84

  • Resolution fixed deleted
  • Status changed from closed to new

comment:39 Changed 22 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:40 Changed 20 months ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top