Opened 10 years ago

Last modified 7 years 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


Suppose I have the following classes:

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

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

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\", line 716, in __get__
  File "d:\code\env\src\django\django\db\models\fields\", line 469, in __init__
    super(ManyRelatedManager, self).__init__()
TypeError: __init__() takes at least 2 arguments (1 given)

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

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

        manager = RelatedManager(
            core_filters={'%s__pk' % self.field.related_query_name(): instance._get_pk_val()},

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

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 ( ) django should use the default manager, right?

Change History (12)

comment:1 Changed 10 years ago by CBWhiz@…

Keywords: ManyToManyField Manager added
Version: 1.1SVN

comment:2 Changed 10 years ago by Russell Keith-Magee

Triage Stage: UnreviewedDesign 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 10 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 10 years ago by Russell Keith-Magee

milestone: 1.21.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 9 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:6 Changed 9 years ago by Tomasz Zieliński

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

comment:7 Changed 9 years ago by Tomasz Zieliński

Easy pickings: set

comment:8 Changed 9 years ago by 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.

comment:9 Changed 9 years ago by Luke Plant

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 9 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 8 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 7 years ago by Anssi Kääriäinen

Triage Stage: Design decision neededAccepted

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.

Note: See TracTickets for help on using tickets.
Back to Top