Opened 17 years ago

Closed 10 years ago

Last modified 10 years ago

#3871 closed New feature (fixed)

Use custom managers in reverse relations

Reported by: EspenG Owned by: v1v3kn
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: paolo@…, espen@…, v1v3kn, Sebastian Goll, r.dav.lc@…, daevaorn@…, Stephane "Twidi" Angel, 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 17 years ago.
3871.2.diff (4.7 KB ) - added by EspenG <espen@…> 17 years ago.
Now also support ForeignKeys
3871-patch.diff (5.6 KB ) - added by v1v3kn 12 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 12 years ago.
updated patch with docs
3871-patch-with-docs.diff (7.0 KB ) - added by v1v3kn 12 years ago.
patch with docs
r17204-custom-reverse-managers.diff (16.7 KB ) - added by Sebastian Goll 12 years ago.
Revised patch.

Download all attachments as: .zip

Change History (46)

comment:1 by Jeff Forcier <reg@…>, 17 years ago

Cc: reg@… added

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 by Paolo Dina <paolo@…>, 17 years ago

Cc: paolo@… added

comment:3 by anonymous, 17 years ago

Cc: espen@… added

comment:4 by EspenG, 17 years ago

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

by EspenG, 17 years ago

Attachment: 3871.diff added

comment:5 by EspenG, 17 years ago

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

Espen

by EspenG <espen@…>, 17 years ago

Attachment: 3871.2.diff added

Now also support ForeignKeys

comment:6 by EspenG <espen@…>, 17 years ago

Has patch: unset
Patch needs improvement: set

comment:7 by EspenG <espen@…>, 17 years ago

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 by Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedDesign 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 by Chris Beaven, 16 years ago

Component: Core frameworkDatabase wrapper

comment:10 by Russell Keith-Magee, 14 years ago

Triage Stage: Design decision neededAccepted

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 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: New feature

by v1v3kn, 12 years ago

Attachment: 3871-patch.diff added

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

comment:12 by v1v3kn, 12 years ago

Easy pickings: unset
Has patch: set
Needs tests: unset
Owner: changed from nobody to v1v3kn
Status: newassigned
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

by v1v3kn, 12 years ago

Attachment: 3871-patch-with-docs added

updated patch with docs

comment:13 by v1v3kn, 12 years ago

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

by v1v3kn, 12 years ago

Attachment: 3871-patch-with-docs.diff added

patch with docs

comment:14 by Sebastian Goll, 12 years ago

Cc: Sebastian Goll 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 by Jeff Forcier, 12 years ago

Cc: reg@… removed

comment:16 by Davor Lucic, 12 years ago

Cc: r.dav.lc@… added

comment:17 by Sebastian Goll, 12 years ago

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 by Aymeric Augustin, 12 years ago

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 by Sebastian Goll, 12 years ago

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.

by Sebastian Goll, 12 years ago

Revised patch.

comment:20 by Alexander Koshelev, 12 years ago

Cc: daevaorn@… added

comment:21 by myusuf3, 12 years ago

Patch needs improvement: set

This patch no longer applies

in reply to:  21 ; comment:22 by Sebastian Goll, 12 years ago

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

in reply to:  22 comment:23 by myusuf3, 12 years ago

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 by Stephane "Twidi" Angel, 12 years ago

Cc: Stephane "Twidi" Angel added

comment:25 by Jeremy Dunck, 12 years ago

Cc: jdunck@… added

comment:26 by anonymous, 12 years ago

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 by anonymous, 11 years ago

Ditto

comment:28 by Simon Charette, 11 years ago

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 by ntucker, 11 years ago

I would very much appreciate this being added. :)

comment:30 by loic84, 11 years ago

Cc: loic@… added

Proof of concept following discussion with @akaariai on IRC:

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

comment:31 by Anssi Kääriäinen, 11 years ago

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 by Tim Graham, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:34 by Anssi Kääriäinen, 10 years ago

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 by Anssi Kääriäinen, 10 years ago

Resolution: fixed
Status: assignedclosed

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

comment:36 by jbg@…, 10 years ago

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 by loic84, 10 years ago

Resolution: fixed
Status: closednew

comment:39 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

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 by Anssi Kääriäinen <akaariai@…>, 10 years ago

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