Code

Opened 4 years ago

Last modified 5 weeks ago

#14891 new Bug

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 12 months ago.
New version of the patch adding custom managers for ForeignKey fields

Download all attachments as: .zip

Change History (27)

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 Changed 3 years ago by jaddison

  • Severity set to Normal
  • Type set to 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 Changed 3 years ago by carljm

  • Summary changed from ManyToManyRelatedField uses different Manager than ForeignKey-field to use_for_related_fields=False is not honored by reverse FK or M2M related managers
  • Type changed from New feature to Bug

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 Changed 3 years ago by carljm

  • 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 Changed 3 years ago by isagalaev

  • Cc Maniac@… added

comment:7 Changed 3 years ago by carljm

  • UI/UX unset

#17038 was a duplicate.

comment:8 Changed 2 years ago by anonymous

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

comment:9 Changed 2 years ago by ramiro

#17746 was another duplicate.

comment:10 Changed 2 years ago by 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!

comment:11 Changed 2 years ago by anonymous

Please someone fix this

comment:12 Changed 2 years ago by bmihelac

  • Cc bmihelac@… added

comment:13 Changed 20 months ago by alan.meadows@…

Please for the love of god fix this

comment:14 Changed 19 months ago by 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!

comment:15 Changed 19 months ago by anonymous

SOMEONE FIX THIS!!! PLEAAAAASEEEEE

comment:16 Changed 19 months ago by charettes

  • Cc charette.s@… added

comment:17 Changed 18 months ago by 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.

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

comment:19 Changed 18 months ago by akaariai

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 Changed 18 months ago by russellm

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 Changed 15 months ago by Alex

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 14 months ago by HM

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

comment:23 Changed 12 months ago by palkeo

  • 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 Changed 12 months ago by palkeo

  • Cc palkeo added

Changed 12 months ago by palkeo

New version of the patch adding custom managers for ForeignKey fields

comment:25 Changed 10 months ago by loic84

  • Cc loic@… added

comment:26 Changed 5 weeks ago by 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.