Code

Opened 4 years ago

Last modified 16 months ago

#13313 new Bug

Custom Default Manager with extra __init__ arguments fails if model is used in a ManyToManyField

Reported by: CBWhiz@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: ManyToManyField, Manager
Cc: tomasz.zielinski@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Suppose I have the following classes:

class SpecialManager(models.Manager):
    def __init__(self, special_arg):
        self.special_arg=special_arg
    def random_method():
        pass

class SpecialModel(models.Model):
    objects = SpecialManager('hello')
    pass

class InnocentModel(models.Model):
    my_friends = models.ManyToManyField(SpecialModel)

This is all well and fine, until I actually try to use the model:

>>> obj = InnocentModel.objects.all()[0]
>>> obj.my_friends
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "d:\code\env\src\django\django\db\models\fields\related.py", line 716, in __get__
    reverse=False
  File "d:\code\env\src\django\django\db\models\fields\related.py", line 469, in __init__
    super(ManyRelatedManager, self).__init__()
TypeError: __init__() takes at least 2 arguments (1 given)

This is because related.py creates a subclass of the manager's default model for relation purposes:

http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py?rev=12908#L706

        # Dynamically create a class that subclasses the related
        # model's default manager.
        rel_model=self.field.rel.to
        superclass = rel_model._default_manager.__class__
        RelatedManager = create_many_related_manager(superclass, self.field.rel)

        manager = RelatedManager(
            model=rel_model,
            core_filters={'%s__pk' % self.field.related_query_name(): instance._get_pk_val()},
            instance=instance,
            symmetrical=self.field.rel.symmetrical,
            source_field_name=self.field.m2m_field_name(),
            target_field_name=self.field.m2m_reverse_field_name(),
            reverse=False
        )

This manager is then instantiated, and calls super():

http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py?rev=12908#L469

465 	    class ManyRelatedManager(superclass):
466 	        def __init__(self, model=None, core_filters=None, instance=None, symmetrical=None,
467 	                join_table=None, source_field_name=None, target_field_name=None,
468 	                reverse=False):
469 	            super(ManyRelatedManager, self).__init__()

And because this super() is now calling the custom manager's init without passing in any parameters, we get the traceback above.

So, a few questions:

  1. Is this a documentation error? Should managers not take init paramaters?
  1. You'll notice I don't have use_for_related_fields specified on my manager. According to the documentation ( http://docs.djangoproject.com/en/dev/topics/db/managers/#using-managers-for-related-object-access ) django should use the default manager, right?

Attachments (0)

Change History (12)

comment:1 Changed 4 years ago by CBWhiz@…

  • Keywords ManyToManyField, Manager added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.1 to SVN

comment:2 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Design decision needed

This is certainly something that needs to be addressed for 1.2; however, I'm not immediately certain if it's a documentation issue (don't do that!) or an implementation problem.

comment:3 Changed 4 years ago by CBWhiz@…

Assuming you want to support this feature, one main way I can think of is by using copy.deepcopy to copy the the already instantiated default manager from the model, and then monkeypatch in the currently subclassed methods.

Another possibility is basically making a ManyRelatedManagerProxy, which either proxies the other manager methods explicitly or overrides getattribute to run the originally wanted method from the already instantiated default model or the new subclass m2m functions. In effect, make the proxy be a virtual subclass, so that init is not needed and so any manager state is preserved (if, for example, get_query_set() uses an arg originally passed to init)

Both options have costs, but already the ReverseManyRelatedObjectsDescriptor generates a new subclass for each call without caching. Why not cache the (or with this change, the monkeypatched) manager returned by get directly on the ReverseManyRelatedObjectsDescriptor?

comment:4 Changed 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

On closer examination, this isn't actually a regression for 1.2; it's a problem that has always existed, we've just never noticed it. The solution isn't obvious, however.

  • The m2m manager must be a subclass of the _default_manager on the model. Unless we do some serious twisting, a proxy won't do the job here.
  • We can't use the _base_manager; this would avoid the issue, but would be backwards incompatible.
  • Copying and monkeypatching might work, but it's messy. It also means that managers need to be copyable, which can be harder to achieve than you might think (witness the recurring problems with pickling and caching of objects).

One possible suggestion (via Alex Gaynor) - add a method to managers that provides the any extra arguments that are necessary to construct an identical instance. For example:

class MyManager(models.Manager):
    def __init__(self, arg1, arg2):
        self.arg1 = arg1
        self.arg2 = arg2
        ...

    def constructor_args(self):
        return (self.arg1, self.arg2)

Then, the many-to-many code can construct a subclass by calling super(MyManager, self).__init__(*_default_manager.constructor_args()) . I'm open to other suggestions, though.

Regarding the caching ideas - you're right, there is absolutely no reason that we need to recreate the model class every time. The m2m/FK related descriptor code is in severe need of a cleanup; this is one aspect that could be cleaned up.

Since this isn't a regression, and the solution isn't obvious, I'm bumping to 1.3. The interim workaround is to ensure that default managers take no arguments.

comment:5 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:6 Changed 3 years ago by TomaszZielinski

  • Cc tomasz.zielinski@… added
  • Easy pickings unset

comment:7 Changed 3 years ago by TomaszZielinski

  • Easy pickings set

comment:8 Changed 3 years ago by TomaszZielinski

I'm confused about the easy picking which I've just restored - have I really unset them (by accident) in comment 6 ?? I don't see any trace of anyone setting them before.

comment:9 Changed 3 years ago by lukeplant

  • Easy pickings unset

You 'unset' it because:
1) it is a new field
2) previously it was effectively 'null'
3) it has a default value of 'false'

comment:10 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 16 months ago by akaariai

  • Triage Stage changed from Design decision needed to Accepted

I have a feeling that a wrapper object could work better than dynamic subclass for the related managers. Though this needs more investigation.

If there is a solution that turns out to be relatively easy it seems worth it to allow init args, but anything too complex seems like a bad idea. If no clean patch arises then documenting the current situation should be done.

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.