Code

Opened 16 months ago

Last modified 4 months ago

#20057 new Bug

Reverse related manager should be a manager INSTANCE, not CLASS

Reported by: jonash Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5
Severity: Normal Keywords:
Cc: odin.omdal@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Please see https://github.com/carljm/django-model-utils/issues/31 for a problem description -- "Solution 1" is what this bug is filed for.

What I currently think of is actually pretty simple:

For the descriptor runtime related manager class hackery (this stuff here https://github.com/django/django/blob/d744c550d51955fd623897884446b7f34318e94d/django/db/models/fields/related.py#L484), don't subclass the original manager and then call @super(...)@ in the methods. Instead, make it a totally new class that calls instance methods.

We would probably have to extend the wrappery to replace inheritance. Or do some other type-hackery. (It can probably not get any uglier than it already is ;-)

Attachments (0)

Change History (4)

comment:1 Changed 16 months ago by jonash

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Reverse related manager should be on manager INSTANCE, not CLASS to Reverse related manager should be a manager INSTANCE, not CLASS

comment:2 Changed 16 months ago by carljm

  • Triage Stage changed from Unreviewed to Accepted

I haven't looked into this enough yet to really evaluate the feasibility of turning related managers into instance wrappers rather than subclasses, but I know the current system is a major hassle for custom managers that you want used for related-object queries, because it effectively means you can't do any important configuration of the manager via instantiation (since the related manager will be a new instance of a subclass). This is wasteful: since you're instantiating the manager when you assign it as a model class attribute, you ought to be able to usefully pass it arguments there. And it leads to nasty dynamic-subclass-creation hacks to achieve things that ought to be doable via simple instantiation arguments. (See https://github.com/carljm/django-model-utils/blob/master/model_utils/managers.py#L172). So I'm accepting this on principle: if we can do it, it would be an improvement.

comment:3 Changed 16 months ago by akaariai

FWIW I am very much +1 for changing the way the related managers are handled. As said above the code can't get any uglier than it already is...

comment:4 Changed 4 months ago by velmont

  • Cc odin.omdal@… added

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.