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

[sdksho@…]

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.

[Russell Keith-Magee]

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.

[James Addison]

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

[Carl Meyer]

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.

[Carl Meyer]

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.

[Carl Meyer]

More discussion at ​http://groups.google.com/group/django-developers/browse_thread/thread/be12b1f422db3634

[Carl Meyer]

#17038 was a duplicate.

[anonymous]

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

[Ramiro Morales]

#17746 was another duplicate.

[anonymous]

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!

[anonymous]

Please someone fix this

[alan.meadows@…]

Please for the love of god fix this

[anonymous]

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!

[anonymous]

SOMEONE FIX THIS!!! PLEAAAAASEEEEE

[anonymous]

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.

[anonymous]

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.

[Anssi Kääriäinen]

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.

[Russell Keith-Magee]

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.

[Alex Gaynor]

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.

[HM]

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

[palkeo]

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.

[palkeo]

New version of the patch adding custom managers for ForeignKey fields

[jordonwii]

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.

[Josh Smeaton]

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.

[Loic Bistuer]

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.

[Loic Bistuer]

​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.

[Loïc Bistuer <loic.bistuer@…>]

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.