umongo icon indicating copy to clipboard operation
umongo copied to clipboard

Troubles using super() inside Document

Open lafrech opened this issue 7 years ago • 4 comments

It could be me doing things wrong.

When using inheritance, you sometimes want to use super().

AFAIU, you can't use the zero argument form super() because the compiler will pass it the template class, not the implementation class. So you have to write super(MyDocument, self).

        @instance.register
        class Doc(Document):

            name = fields.StrField()

            def __init__(self, **kwargs):
                super(Doc, self).__init__(**kwargs)

This does not look like such a big deal. I mean I can easily live with this.

Unfortunately, I'm afraid it does not work that great if registration is not done using a decorator. More precisely:

This works:

        class Doc(Document):

            name = fields.StrField()

            def __init__(self, **kwargs):
                super(Doc, self).__init__(**kwargs)

        Doc = self.instance.register(Doc)
        doc = Doc()

However, this does not:

        class Doc(Document):

            name = fields.StrField()

            def __init__(self, **kwargs):
                super(Doc, self).__init__(**kwargs)

        SuperDoc = self.instance.register(Doc)
        doc = SuperDoc()

Here's what I get:

>       super(Doc, self).__init__(**kwargs)
E       TypeError: super(type, obj): obj must be an instance or subtype of type

Indeed, in this latter case, when in __init__, type(Doc) returns

'<class 'umongo.template.MetaTemplate'>'

In other words, it worked in the first case because at init time, Doc, in the namespace, now refers to the implementation. (I'm not familiar with Python internals, but that's how it looks.)

But in the general case, it seems broken because Doc still refers to the template.

BTW, this works (for illustration purpose but pretty useless in practice):

    def test_super_in_inheritance(self):

        class Doc(Document):

            name = fields.StrField()

            def __init__(self, **kwargs):
                super(SuperDoc, self).__init__(**kwargs)

        SuperDoc = self.instance.register(Doc)
        doc = SuperDoc()

The consequence of this is that some use cases are broken:

  • super + Multi-driver as described in the docs.
  • super + Registration happening in another file

I'm in that second use case and I don't see any workaround.

super(type(self), self).__init__(**kwargs) makes the test pass but I imagine it would break in a GrandParent - Parent - Child case.

Should we add some method to fetch the right implementation?

In the method, we know self and we know the template class (or we could know it if we kept a link in to it at registration), so we could move up the inheritance tree from the implementation class until we meet the one that corresponds to the template.

Am I missing something?

lafrech avatar Jan 19 '17 17:01 lafrech

More on this.

The use case presented in the tests above is a bit silly because you can't call __init__ in Document template anyway.

Also, I should be more explicit about the fact that the point is to get to the parent implementation.

I suppose in some cases, getting the template is enough. The method from the template is unchanged in the implementation. But as soon as the method does stuff in DB, we need the implementation's method. And the implementation from the same instance. Good thing, we do have a link to the instance and the template class so we can do that:

    def test_super_in_inheritance(self):

        class Parent(Document):

            def get_impl_from_tmpl(self, tmpl):
                return self.opts.instance.retrieve_document(tmpl)

            class Meta:
                allow_inheritance = True

            name = fields.StrField()

            def get_name(self):
                return self.name

        class Child(Parent):

            def get_name(self):
                # Option 1 - Works using template's get_name method
                name = Parent.get_name(self)
                # Option 2 - Works using implementation's get_name method
                name = self.get_impl_from_tmpl(Parent).get_name(self)
                return name[0]


        ParentImpl = self.instance.register(Parent)
        ChildImpl = self.instance.register(Child)

        child = ChildImpl(name='Toto')
        child.get_name()

Rather than using super(), we can specify the parent explicitly.

Option 1 - The parent class we pass in there will be the template, or the implementation if the document is registered into an implementation with the same name in the namespace (not sure I'm clear, but this is the same mechanism as in above comment).

Option 2 - Or Document could have an extra method to use the instance (self) and the parent template to get to the parent implementation in the same instance. The method is get_impl_from_tmpl. In my test, it is defined in Parent but in uMongo it would be in Document. Downside of this, you need to call this yourself so you can't use inheritance seamlessly. Good thing is you always get an implementation class so you can do DB stuff in the inherited method.

Pitfall, this breaks if you act silly and replace Parent in the namespace:

        ParentImpl = self.instance.register(Parent)
        ChildImpl = self.instance.register(Child)

        Parent = 'Let me try something stupid'
        child = ChildImpl(name='Toto')
        child.get_name()

lafrech avatar Jan 20 '17 10:01 lafrech

Yeah, __init__ and super() seems really hard to cleanly build from template to implementation. From my point of view it's not such a big deal given a Document rotate around it data from the database and so should not initialize anything but them.

However, we could create a special callback method (i.g. _on_init()) that would be called when the document is initialized. This would solve the trouble and we could even create an error message in the builder when copying fields from template to implementation if we found an __init__ field.

touilleMan avatar Jan 21 '17 22:01 touilleMan

From my point of view it's not such a big deal given a Document rotate around it data from the database and so should not initialize anything but them.

I don't understand this sentence.

However, we could create a special callback method (i.g. _on_init()) that would be called when the document is initialized.

Sure. Although you sometimes want more flexibility so you might need pre_init and post_init.

But __init__ is not the only method with an issue, is it?

I mean, sometimes, you want to override your own methods, like my get_name example and I think this is also broken.

Here's an updated proposal, this time using super() and passing the class as a string to avoid relying on the namespace not being messed up with:

    def test_super_in_inheritance(self):

        class Parent(Document):

            def get_impl_from_tmpl(self, tmpl):
                return self.opts.instance.retrieve_document(tmpl)

            class Meta:
                allow_inheritance = True

            name = fields.StrField()

            def get_name(self):
                return self.name

        class Child(Parent):

            def get_name(self):
                name = super(self.get_impl_from_tmpl('Child'), self).get_name()
                return name[0]

        ParentImpl = self.instance.register(Parent)
        ChildImpl = self.instance.register(Child)

        child = ChildImpl(name='Toto')
        child.get_name()

Using super() this way lets it deal with multi-inheritance and complicated stuff I wouldn't like to screw up.

lafrech avatar Jan 21 '17 22:01 lafrech

Another proposal with a _super method:

    def test_super_in_inheritance(self):

        class Parent(Document):

            def _super(self, tmpl):
                return super(_get_impl_from_tmpl(self, tmpl))

            def _get_impl_from_tmpl(self, tmpl):
                return self.opts.instance.retrieve_document(tmpl)

            class Meta:
                allow_inheritance = True

            name = fields.StrField()

            def get_name(self):
                return self.name

        class Child(Parent):

            def get_name(self):
                name = self._super('Child').get_name()
                return name[0]

        ParentImpl = self.instance.register(Parent)
        ChildImpl = self.instance.register(Child)

        child = ChildImpl(name='Toto')
        child.get_name()

Or with custom _super() as a static method, to be as close as builtin super as possible. But it's a bit of a pity to pass self manually just so that it looks more like the builtin.

    def test_super_in_inheritance(self):

        class Parent(Document):

            @staticmethod
            def _super(tmpl, self):
                return super(_get_impl_from_tmpl(self, tmpl))

            def _get_impl_from_tmpl(self, tmpl):
                return self.opts.instance.retrieve_document(tmpl)

            class Meta:
                allow_inheritance = True

            name = fields.StrField()

            def get_name(self):
                return self.name

        class Child(Parent):

            def get_name(self):
                name = self._super('Child', self).get_name()
                return name[0]

        ParentImpl = self.instance.register(Parent)
        ChildImpl = self.instance.register(Child)

        child = ChildImpl(name='Toto')
        child.get_name()

lafrech avatar Jan 22 '17 13:01 lafrech