Opened 13 years ago

Closed 8 years ago

#14891 closed Bug (fixed)

use_for_related_fields=False is not honored by reverse FK or M2M related managers

Reported by: sdksho@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: Manager ManyToManyField use_for_related_fields
Cc: Maniac@…, bmihelac@…, charette.s@…, palkeo, loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I noticed that the ManyToManyField ignored my "use_for_related_fields = False" setting, and investigated by looking into the source code. It seems that the ManyToManyField always uses _default_manager, and the ForeignKey-fields use _base_manager. If this is by design, I have failed to find the documentation explaining why the two kinds of lookups should have different behaviour (and what to do if you want custom behaviour for the objects-manager, but not for ManyToMany lookups), and this should then be considered a plea for a documentation update.

Attachments (1)

patch.diff (6.7 KB ) - added by palkeo 11 years ago.
New version of the patch adding custom managers for ForeignKey fields

Download all attachments as: .zip

Change History (31)

comment:1 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedDesign decision needed

I can't think of an obvious reason why use_for_related_fields isn't honored on m2m fields. However, introducing this change has the potential to introduce some fairly major backwards incompatibilities.

Some discussion of the impact of this change will be required.

comment:2 by James Addison, 13 years ago

Severity: Normal
Type: New feature

At best, this is a documentation update; at worst, possible backwards incompatibilities as already mentioned (not my words!). Regardless, marking this as 'new feature'.

comment:3 by Carl Meyer, 13 years ago

Summary: ManyToManyRelatedField uses different Manager than ForeignKey-fielduse_for_related_fields=False is not honored by reverse FK or M2M related managers
Type: New featureBug

This isn't just M2M fields, it's reverse FKs as well. ForeignRelatedObjectsDescriptor, ManyRelatedObjectsDescriptor, and ReverseManyRelatedObjectsDescriptor all unconditionally use _default_manager rather than _base_manager as the superclass for the dynamic manager they create (the only exception is for internal deletion purposes), and ForeignRelatedObjectsDescriptor.create_manager and create_many_related_manager both create a dynamic manager class whose get_query_set method gets its initial queryset from super(). In other words, regardless of the value of use_for_related_fields you always get a related-manager that is a subclass of and behaves the same as your default manager.

The only place "use_for_related_fields = False" (the default, in theory!) is actually honored currently is for OneToOneField (SingleRelatedObjectDescriptor and ReverseSingleRelatedObjectDescriptor both use _base_manager), and internal deletion traversal.

This definitely does not match the documented behavior... and fixing it is also likely to break people's code. The fix is simple, if we actually want to make it - just always use _base_manager instead of _default_manager in all of those descriptors. (ensure_default_manager already sets _base_manager to be the same as _default_manager if use_for_related_fields = True).

I'm changing the categorization here to "bug." When we've clearly documented how a feature is supposed to work, and it doesn't work, fixing it is not a "new feature," regardless of how backwards-incompatible the fix is. Our usual policy is that we fix bugs regardless -- but I'm sure exceptions to that have been made in the past when there was a likelihood of breaking lots of code, and this might be a case for such an exception.

comment:4 by Carl Meyer, 13 years ago

Has patch: set

Patch with test for this pushed to https://github.com/carljm/django/compare/master...t14891 for consideration.

The first commit there has failing tests demonstrating that we currently don't do what our documentation says.

The second commit has the fix (essentially s/_default_manager/_base_manager/g in related.py.)

And the third commit fixes the two places in Django's own test suite where we were assuming the semantics of use_for_related_fields=True without specifying it. This is a pretty good indicator that fixing this will break other peoples' code as well.

The result of the branch is that all tests pass and the behavior of related managers matches what our documentation says it is.

comment:6 by Ivan Sagalaev, 13 years ago

Cc: Maniac@… added

comment:7 by Carl Meyer, 12 years ago

UI/UX: unset

#17038 was a duplicate.

comment:8 by anonymous, 12 years ago

Is there any plan to fix this? This is still happening in 1.3.

comment:9 by Ramiro Morales, 12 years ago

#17746 was another duplicate.

comment:10 by anonymous, 12 years ago

Is there any plan to fix this? How do we get this bug fixed? It would be great if someone could come up with a solution or a workaround. Thanks!

comment:11 by anonymous, 12 years ago

Please someone fix this

comment:12 by bmihelac, 12 years ago

Cc: bmihelac@… added

comment:13 by alan.meadows@…, 11 years ago

Please for the love of god fix this

comment:14 by anonymous, 11 years ago

Really nasty bug... reading the documentation about use_for_related_fields 'implied' that it would actually use for ManyToMany as well...

Any progress on this?

Thanks!

comment:15 by anonymous, 11 years ago

SOMEONE FIX THIS!!! PLEAAAAASEEEEE

comment:16 by Simon Charette, 11 years ago

Cc: charette.s@… added

comment:17 by anonymous, 11 years ago

I refuse to eat until this is fixed!
DAY 1 of my hunger strike begins today.. hopefully there will be a resolution soon. I am eating my last piece of bread now.

comment:18 by anonymous, 11 years ago

DAY 2 of hunger strike.

I'm beginning to feel it slowly, I was really craving for some breakfast, but I still refuse to eat until this is resolved.

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

One suggestion to move this forward: always use _default_manager, deprecate use_for_related_fields, add ability to use per-relation managers. Maybe with:

class SomeModel(models.Model):
    somemanager = MyManager()
    m2m = models.ManyToManyField(RelatedObject, manager=RelatedObject.somemanager, reversemanager=somemanager)

I must admit I haven't done enough research yet, so the above suggestion might be missing something crucial...

It seems just fixing this issue to match our docs would cause problems at least for some users. So, even if we want to do that, then we would likely want to invent a new flag behaving as the currently documented use_for_related_fields and deprecate the old one so that users have time to adapt to the change in behaviour.

comment:20 by Russell Keith-Magee, 11 years ago

This is an open source project. If you truly feel that this is the single greatest bug facing Django, and can't imagine how we've let it linger for so long, YOU CAN FIX IT YOURSELF. You can fork the repo, make the necessary change, and use that fork in your own work. Using an anonymous account to howl at the moon about how hard done by you are because nobody else will volunteer to fix your problem for free is neither helpful, nor endearing.

comment:21 by Alex Gaynor, 11 years ago

Triage Stage: Design decision neededAccepted

Marking as accepted, I'm not sure what the fix will be: the proposed change is fairly backwards incompatible (but maybe that's ok), at a minimum the docs are wrong.

comment:22 by HM, 11 years ago

So, is there a workaround? I very much need the backwards manager to have the same extra methods as the forwards manager.

comment:23 by palkeo, 11 years ago

Needs documentation: set
Patch needs improvement: set

Hi !

I'm trying to add the support for per-relation custom managers.

I just implemented it to the ForeignKey relations, and it seems to works fine.
It is also backwards compatible, and I think it may be possible to deprecate progressively the « use_for_related_fields » option.

Here is the branch I am working on : https://github.com/makinacorpus/django/tree/custom-managers

I'm waiting for some feedback, and if you approve my current patch, I will add that support for the one-to-one and many-to-many fields.

comment:24 by palkeo, 11 years ago

Cc: palkeo added

by palkeo, 11 years ago

Attachment: patch.diff added

New version of the patch adding custom managers for ForeignKey fields

comment:25 by loic84, 11 years ago

Cc: loic@… added

comment:26 by jordonwii, 10 years ago

At the very least, there should be an update to the docs (particularly this page) indicating that use_for_related_fields does not work.

A decision should be made as to whether this is going to get fixed, if not. Django in practice is currently in direct contradiction to its documentation.

comment:27 by Josh Smeaton, 9 years ago

Here's an idea to close out this ticket. Free to anyone that's hurt by this issue and would like to attempt to fix it.

1) deprecate use_for_related_fields flag on manager
2) remove the documentation for use_for_related_fields
3) implement 'manager="manager_name"' and 'reverse_manager="manager_name"' on related fields (ware migrations)
4) document the new keyword arguments to ForeignKey/M2MField/OneToOneField

This will give you full control over which managers are used at all times, while still allowing explicit override: model.foo_set(manager='other')

This will be more verbose, but it'll be correct.

class MyModel(models.Model):
    objects = MyDefaultManager()
    related = MyRelatedManager()
    foo = models.ForeignKey(Foo, manager='objects', related_manager='related')

I don't think putting a big warning in the docs "this does not work" is sufficient, especially when there are definitely avenues for fixing the underlying problem. It's the lazy way out. Doing nothing is also bad. But until someone steps up to actually do the work, then it's still going to be an issue.

Now, does my proposal above have any significant drawbacks? It's a combination of previous talk and my discussions with Loic on IRC.

comment:28 by Loic Bistuer, 8 years ago

I like the (manager='objects', related_manager='related') API since it allows fine grained control over the relation manager as expected by the relational field. It's in the same spirit as https://docs.djangoproject.com/en/1.9/topics/db/queries/#using-a-custom-reverse-manager which allows controlling the reverse manager per query per relational field.

But I also see merit in allowing the model to say which manager should be used by default by relations. My work on #20932 can easily be extended to support a related_manager_name meta.

It would work as follows:

class Model(models.Model):
    objects = models.Manager()
    custom_manager = CustomManager()

    class Meta:
        related_manager_name = 'custom_manager'

PR https://github.com/django/django/pull/6175 shows a working implementation + deprecation.

comment:29 by Loic Bistuer, 8 years ago

PR6175 deprecates use_for_related_fields. "to-one" relations go through _base_manager which can be specified with the base_manager_name model option and "to-many" relations go through _default_manager which can be specified with the default_manager_name model option.

We tried to add a blanket "use for every relations" manager in the form of related_manager_name but eventually decided against. For finer control over relation managers, I think comment:27 is the way to go but this is a new feature which deserves a ticket of its own.

comment:30 by Loïc Bistuer <loic.bistuer@…>, 8 years ago

Resolution: fixed
Status: newclosed

In ed0ff91:

Fixed #10506, #13793, #14891, #25201 -- Introduced new APIs to specify models' default and base managers.

This deprecates use_for_related_fields.

Old API:

class CustomManager(models.Model):

use_for_related_fields = True

class Model(models.Model):

custom_manager = CustomManager()

New API:

class Model(models.Model):

custom_manager = CustomManager()

class Meta:

base_manager_name = 'custom_manager'

Refs #20932, #25897.

Thanks Carl Meyer for the guidance throughout this work.
Thanks Tim Graham for writing the docs.

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