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

[CBWhiz@…]

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?

2. 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?

[Russell Keith-Magee]

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.

[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?

[Russell Keith-Magee]

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.

[Tomasz Zieliński]

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.

[Luke Plant]

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

[Jacob]

Milestone 1.3 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Anssi Kääriäinen]

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.