Tryton - Issues

 

Issue4735

Title Allow to Apply inheritance on models loaded in the pool
Priority feature Status testing
Superseder Nosy List ced, jcavallo, nicoe, pokoli, reviewbot
Type feature request Components trytond
Assigned To pokoli Keywords review
Reviews 15231002
View: 15231002

Created on 2015-05-01.13:53:51 by pokoli, last changed by reviewbot.

Messages
review15231002 updated at https://codereview.tryton.org/15231002/#ps200001
review15231002 updated at https://codereview.tryton.org/15231002/#ps180001
review15231002 updated at https://codereview.tryton.org/15231002/#ps160001
msg32509 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-03-14.19:35:48
This design is not possible because we can not replace Model class by another implementation (this will be a crazy work) but we just want to apply a specific behaviour to all class that inherit a specific one. And to ensure this feature will be correctly used, we must force developer to not rely on any specific order in mro.
msg32507 (view) Author: [hidden] (jcavallo) Date: 2017-03-14.17:01:46
I would just want to point out that the behavior I am talking about achieves
a very closely what is being described here [1] and there [2].

Allowing to configure what class is ModelSQL (for instance) will make the new
class the parent of the models rather than their child, because the resolution
is done at import time rather than at the end of the pool initialization.

[1] https://discuss.tryton.org/t/allow-to-replace-some-standard-classes/105
[2] https://bugs.tryton.org/issue5686
msg32377 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-03-09.10:44:52
Also you proposal is something that is never done in Python standards.
There is no mechanism in Python for that so I do not think it could be the expected behaviour for any developer.
msg32376 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-03-09.10:38:17
This is classical bad design which introduces side effect and there is no *good* solution.
I think we must not complexity the feature for such case but keep it simple and predictive.
msg32375 (view) Author: [hidden] (jcavallo) Date: 2017-03-09.10:07:01
> On 2017-03-08 15:01, Jean CAVALLO wrote:
> > > I do not see the point. This makes Tryton "Mixin" inheritance
> > > working differently than the usual inheritance.
> > 
> > I am not sure to understand what you mean, could you elaborate ?
> 
> It is bad to have different behaviour.

I meant that I do not see what you mean by "This makes Tryton Mixin inheritance
working differently than the usual inheritance.

> > For instance, if one wants to override the TaxLineMixin class,
> > the mro will be something like :
> > 
> > TaxLineMixinOverride
> > account.invoice.line
> > TaxLineMixin
> 
> I see no problem.

Consider the following :

class Mixin:
    def value(self):
        return 10


class MixinOverride(Mixin):
    def value(self):
        return super(MixinOverride, self).value() + 1


class MyModel(Mixin):
    __name__ = 'my_model'
    def value(self):
        return super(MyModel, self).value() * 2


The difference between the mros is that if MixinOverride is at the end, the
final result will be 21. If it is right after Mixin, it will be 22.

I think the second version is what would be expected.
msg32362 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-03-08.15:45:05
On 2017-03-08 15:01, Jean CAVALLO wrote:
> > I do not see the point. This makes Tryton "Mixin" inheritance
> > working differently than the usual inheritance.
> 
> I am not sure to understand what you mean, could you elaborate ?

It is bad to have different behaviour.

> > Finally, I think it is good practice that Mixin should not depend
> > on the place it has in the mro like that it prevents some
> > side-effects.
> 
> It simplifies the implementation of the current feature, but I
> think it will make "fine-tuning" of Mixin classes a little
> complicated.

I do not understand "fine-tuning".

> For instance, I think you said you wanted to be able
> to override existing Mixin classes (do not remember exactly when,
> so correct me if I'm wrong). But doing so will be difficult with
> the current implementation.
> 
> For instance, if one wants to override the TaxLineMixin class,
> the mro will be something like :
> 
> TaxLineMixinOverride
> account.invoice.line
> TaxLineMixin

I see no problem.

> So the implementation of the TaxLineMixinOverride will have to
> deal with all method overrides that may have taken place in
> the models that extended the base TaxLineMixin class.

I do not understand. You do not have to override all methods. It is
object oriented programming.

> IMO, the
> "logical" Mixin extension should look like :
> 
> account.invoice.line
> TaxLineMixinOverride
> TaxLineMixin

I think we do not have the same logic.
And I do not see why one mro will be better than another except the one
that is consistent with the rest of the framework.
msg32361 (view) Author: [hidden] (jcavallo) Date: 2017-03-08.15:01:52
> I do not see the point. This makes Tryton "Mixin" inheritance
> working differently than the usual inheritance.

I am not sure to understand what you mean, could you elaborate ?

> Also modifying the __bases__ attribute is quite questioning
> (not sure it is allowed in every implementation).

Agreed, it may need some investigation. But we could achieve the
same result by registering in two passes (add a register_meta
method on modules to "build" the base classes, then calling the
regular register).

> Finally, I think it is good practice that Mixin should not depend
> on the place it has in the mro like that it prevents some
> side-effects.

It simplifies the implementation of the current feature, but I
think it will make "fine-tuning" of Mixin classes a little
complicated. For instance, I think you said you wanted to be able
to override existing Mixin classes (do not remember exactly when,
so correct me if I'm wrong). But doing so will be difficult with
the current implementation.

For instance, if one wants to override the TaxLineMixin class,
the mro will be something like :

TaxLineMixinOverride
account.invoice.line
TaxLineMixin

So the implementation of the TaxLineMixinOverride will have to
deal with all method overrides that may have taken place in
the models that extended the base TaxLineMixin class. IMO, the
"logical" Mixin extension should look like :

account.invoice.line
TaxLineMixinOverride
TaxLineMixin
msg32354 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-03-08.11:49:30
I do not see the point. This makes Tryton "Mixin" inheritance working differently than the usual inheritance.
Also modifying the __bases__ attribute is quite questioning (not sure it is allowed in every implementation).
Finally, I think it is good practice that Mixin should not depend on the place it has in the mro like that it prevents some side-effects.
msg32352 (view) Author: [hidden] (jcavallo) Date: 2017-03-08.11:26:27
Following cedric's suggestion, here is my design proposal.

Rather than patching the "last" class in the pool, I would like to inject the
new class right after the class it wants to replace.

Here is an example. Assuming we want to patch the ModelSQL class with a custom
OverridenModelSQL class on ir.ui.menu, the current mro (with an override in the
"my_module" module) looks like :

 <class 'trytond.pool.ir.ui.menu'>,
 <class 'trytond.modules.my_module.ir.ir.ui.menu'>,
 <class 'trytond.ir.ui.menu.ir.ui.menu'>,                    # <-- base class
 <class 'trytond.model.order.SequenceOrderedMixin'>,
 <class 'trytond.model.modelsql.ModelSQL'>,                  # <-- source
 <class 'trytond.model.modelstorage.ModelStorage'>,
 <class 'trytond.model.modelview.ModelView'>,
 <class 'trytond.model.model.Model'>,
 <class 'trytond.error.WarningErrorMixin'>,
 <class 'trytond.url.URLMixin'>,
 <class 'trytond.pool.PoolBase'>,
 <type 'object'>

With the current implementation, we would have (if I understand properly) :

 <class 'trytond.pool.ir.ui.menu'>,                          # <-- new type
 <class 'trytond.modules.my_mixin_module.my_file.OverridenModelSQL>,  # <-- target
 <class 'trytond.pool.ir.ui.menu'>,
 <class 'trytond.modules.my_module.ir.ir.ui.menu'>,
 <class 'trytond.ir.ui.menu.ir.ui.menu'>,                    # <-- base class
 <class 'trytond.model.order.SequenceOrderedMixin'>,
 <class 'trytond.model.modelsql.ModelSQL'>,                  # <-- source
 <class 'trytond.model.modelstorage.ModelStorage'>,
 <class 'trytond.model.modelview.ModelView'>,
 <class 'trytond.model.model.Model'>,
 <class 'trytond.error.WarningErrorMixin'>,
 <class 'trytond.url.URLMixin'>,
 <class 'trytond.pool.PoolBase'>,
 <type 'object'>

What I think should be better :

 <class 'trytond.pool.ir.ui.menu'>,
 <class 'trytond.modules.my_module.ir.ir.ui.menu'>,
 <class 'trytond.ir.ui.menu.ir.ui.menu'>,                    # <-- base class
 <class 'trytond.modules.my_mixin_module.my_file.OverridenModelSQL>,  # <-- target
 <class 'trytond.model.order.SequenceOrderedMixin'>,
 <class 'trytond.model.modelsql.ModelSQL'>,                  # <-- source
 <class 'trytond.model.modelstorage.ModelStorage'>,
 <class 'trytond.model.modelview.ModelView'>,
 <class 'trytond.model.model.Model'>,
 <class 'trytond.error.WarningErrorMixin'>,
 <class 'trytond.url.URLMixin'>,
 <class 'trytond.pool.PoolBase'>,
 <type 'object'>

I quickly made up something that would work, but I don't think it is
clean enough to be integrated as is :

    source = ModelSQL
    target = OverridenModelSQL

    def patch_model(klass):
        if issubclass(klass, target) or not issubclass(klass, source):
            return
        for mro_class in reversed(klass.__mro__):
            if mro_class == source:
                continue
            if source not in mro_class.__bases__:
                continue
            mro_class.__bases__ = tuple(
                x if x != source else target
                for x in mro_class.__bases__)

I tried calling the patch_model method on all classes in the pool at the
end of the pool.init method, and it behaved as expected. But I do not
know if manually updating the __bases__ attribute is acceptable.

Here we directly modify the base class inheritances, so it may be a
problem for multiple databases management. We could probably go around this
by creating another 'trytond.pool.ir.ui.menu' class and make it inherit
of both the original base class and the OverridenModelSQL class.
review15231002 updated at https://codereview.tryton.org/15231002/#ps140001
review15231002 updated at https://codereview.tryton.org/15231002/#ps120001
review15231002 updated at https://codereview.tryton.org/15231002/#ps100001
review15231002 updated at https://codereview.tryton.org/15231002/#ps80001
msg31520 (view) Author: [hidden] (pokoli) (Tryton committer) Date: 2017-01-27.13:43:45
I updated the review taking in account msg31265
msg31265 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-01-13.16:00:22
I think the design should be simpler.
Here it is:

- register in the pool Mixin with a list of classes
- at the end of the load_module_graph:
  - the pool is called to loop over all the classes
  - test if it is a subclass of all the classes registered with the Mixin.
  - add the Mixin to the pool class

I think it is a much simpler design and there is no need to have the Mixin applied before having loaded all the modules. And if there are cases where it is needed, then the module can just explicitly extend the needed classes with the Mixin.
review15231002 updated at https://codereview.tryton.org/15231002/#ps40001
msg21253 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2015-05-18.12:36:54
That's a very bad idea: explicit is better than implicit.
You can have name collision between different class type and even worst different signature for the same named method.
msg21252 (view) Author: [hidden] (pokoli) (Tryton committer) Date: 2015-05-18.12:20:45
I dont think it's necessary to specify what class you want to override, as in the code you can test issubclass or isinstance, so you know if the class is a part of a mixin.
msg21180 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2015-05-06.23:20:44
It will be good to also be able to extend any Mixin.
msg21147 (view) Author: [hidden] (pokoli) (Tryton committer) Date: 2015-05-01.13:53:50
See https://groups.google.com/forum/#!topic/tryton-dev/CeoBg6iAH7I
History
Date User Action Args
2017-06-07 17:29:42reviewbotsetmessages: + msg33951
2017-06-07 17:09:08reviewbotsetmessages: + msg33949
2017-03-22 14:01:39reviewbotsetmessages: + msg32679
2017-03-14 19:35:48cedsetmessages: + msg32509
2017-03-14 17:01:46jcavallosetmessages: + msg32507
2017-03-09 10:44:53cedsetmessages: + msg32377
2017-03-09 10:38:17cedsetmessages: + msg32376
2017-03-09 10:07:01jcavallosetmessages: + msg32375
2017-03-08 15:45:06cedsetmessages: + msg32362
2017-03-08 15:01:53jcavallosetmessages: + msg32361

Showing 10 items. Show all history (warning: this could be VERY long)