[ADD] computed writable inheritance compatibility guideline

[pedrobaeza]

cc @Tecnativa

[pedrobaeza]

@sbidoul @dreispt can you please check? Do you need extra explanations about this?

[sbidoul]

The reason is not obvious. Could you add an explanation?
Without knowing the context I'd say the hasattr looks a little bit like a hack :)

[sebastienbeau]

Hi, If I am right your idea is if their is two module convert a normal field into a computed field then you want to try to make it compatible and do not break the inheritance by trying to call super method if exist.

Do you have some example of case in OCA? (I like the idea but at the same time if two module convert a field to a computed field there is a lot of chance that the inheritance will not work in a functional point of view, the result expected can be weird and random dependency of loading order of the module). Maybe having glue module can solve it.

[pedrobaeza]

Thanks for resuming this. I have been very busy for expanding the reasoning. Yes, basically is what you depict in your comment: make sure from the beginning we have a base for making all OCA modules compatible with the conversion from regular fields to computed writable (which it didn't happen in the past for example for onchanges not defined in base modules).

This arose in this PR debate:

OCA/sale-workflow#1123 (comment)

You can read the thread for understanding the rationale process to get to this. The PR is not yet merged as we found other blocking points that has to do with ORM flaws themselves (several of them has been already tackled in Odoo meanwhile). And I don't have yet a real case for some modules using this system.

For the point about this not being the definitive solution for avoiding any compatibility problem, you are right of course, but we have the same problem in other parts of Odoo (and no possible to avoid it at all in any inheritable framework), so we have to take care about it and sometimes when a new module comes to the party, some adjustment must be done (similar to add hooks on base modules). Anyway, here we have better tools for controlling the final result:

• _origin puts a copy of the record before entering in an onchange/compute process, so it serves us to control the initial value of something for forcing what we want.
• computed writable fields don't need to assign a value for all the records in self, so another guideline can be to only assign what we are sure that should have that value.
• On conflicting assigned values, we still have the glue module option for ensuring the final value.

I hope this makes clearer the situation.

[dreispt]

From the proposed text, I understand that the case this applies to is when we are adding a computation method to a field that originally was a regular field, not computed.

And it seems that there is an expectation of taking advantage of a "computed writable field" new v13 feature.
I'm not familiar with this feature, and I wonder if this is not a supported feature, but rather an accidental behavior that might not work consistently work in the future.

Unless we confirm this is a stable framework feature, I would automate values in non compute fields using the usual tools, write() or onchange logic.

[pedrobaeza]

@dreispt you have understood properly the proposed text.

It's not an accidental behavior. "Computed writable fields" is a new feature of the ORM framework. They are called to replace onchanges.

I did a workshop in Spanish OCA days about computed writable fields that can serve you for knowing a bit about this (in Spanish, but I think is not a problem at all for you 😉 ): https://www.youtube.com/watch?v=lE4e4G45BHo&t=764s. Also the talks from Fabien (https://www.youtube.com/watch?v=ZLaccjGUEeo), Raphael (https://www.youtube.com/watch?v=oixsaLgKL5I) and Olivier (https://www.youtube.com/watch?v=OMZbbWrSsfw) about this change (and others) in Odoo XP are more complete than mine.

The question for this guideline is the blackhole when you have fields that have not been thought initially (usually on core modules) as computed writable (but regular ones), and you want to add "onchange" behavior through this mechanism.

A similar blackhole happened in the past with onchanges in v8 API: if you define a field in one module, but don't create an onchange method (whatever name it has), when you want to add an onchange over that field in dependent modules, if you select the same method name on several non chained modules (and thus, not calling super), they will overlap and only one of them will be executed. The guideline for that case was clear: if the onchange is defined among the field: use that onchange calling super. If not, create an onchange method with this name that assures no colliding: _onchange_<field>_<module_name>.

In this case, this technique can't be used, as investigated in the thread referenced above.

Maybe @rco-odoo or @Feyensv can bring us any additional thought or see if they agree with this guideline.

[pedrobaeza]

@sbidoul can you please merge this, now that computed writable field are everywhere?

[pedrobaeza]

@sbidoul @dreispt @simahawk gentle reminder

[rousseldenis]

Not sure this should be inserted in guidelines as it seems more a hack as already said.

I'd prefer @sebastienbeau solution. IMHO, module code should not insert code of something is not aware of. A glue module should do the job

[lmignon]

Hi,

This UC is really interesting but, IMO, the proposed solution looks like a hack for me too. From what I understand, the origin of this problem comes from the change of a field definition. From a certain point of view, this reminds me the Liskov Substitution Principle. By changing the definition of a field, the LSP is broken. However, in this case and from a technical point of view, it could be argued that for the user and for 99% of the code, this change does not introduce any significant incompatibility issue since manual assignment of a value to the field remains possible. Then what? I'm also facing this kind of need on some projects. In such a case, I prefer to adopt an explicit approach over the implicit one proposed in this change to the guideline. The explicit approach can be summarized as:

1. Create a specific addon where the unique scope is to change the field definition from 'normal' to computed='', readonly=....
2. Always depends on this new addon for any module making change to the way the field is computed

In this way, any module relaying on the fact that a field has been changed to become a computed field always have an explicit dependency on the same specific addon introducing this change and we avoid inheritance issue.

Given the number of repositories in OCA, such an approach would only be possible by creating a specific repository where modules that change the type of a field are grouped.

It's a bit more work but it add the advantage of making things explicit and traceable.

Regards,

lmi

[simahawk]

Personally, I agree w/ @lmignon and w/ Liskov :)

[pedrobaeza]

Thanks for your comments. The solution that @lmignon proposes will create bloated number of modules whose function reduces to a line, and the situation where your module needs to depend on a lot of other insignificant modules, or to create them, wait for the approval and merge, and then be able to propose the new one. Please don't go that overhead way.

This proposal is a compromise between convenience and avoid at maximum the problems.

It's not to modify a field, but the way Odoo has to express that a field has "the new type of onchange". If you think about it, the same problem happens on old style onchanges (and thus, we can't go with that approach for avoiding the problem): if the module that defines the field doesn't define an onchange method, you may have the problem of 2 modules defining the onchange method with the same name and replacing one each other. Luckily, with onchanges, there's the possibility of defining different method names, but at the end, you don't know the execution order, and at the end, you can have unexpected results.

On this uncertainty landscape, I think these 2 lines put as a guideline (that can be added as a fix if someone doesn't add them), solves most of the overhead problems, reducing the maximum possible problems, although we know that not all can be avoided, and only requiring glue modules in that cases.

[lmignon]

Thanks for your comments. The solution that @lmignon proposes will create bloated number of modules whose function reduces to a line, and the situation where your module needs to depend on a lot of other insignificant modules, or to create them, wait for the approval and merge, and then be able to propose the new one. Please don't go that overhead way.

These new modules even if they are basic are meaningfull. They explicitly declare that a breaking change in the field declaration is introduced.

On this uncertainty landscape, I think these 2 lines put as a guideline (that can be added as a fix if someone doesn't add them), solves most of the overhead problems, reducing the maximum possible problems, although we know that not all can be avoided, and only requiring glue modules in that cases.

A new addon is a one time effort with a few lines of code to avoid to pollute our code everywhere with the same 2 lines.

            if hasattr(super(), "_compute_field"):
                super()._compute_field()

Our code remains clean, we doesn't introduce overhead at runtime, things are explicit and traceable, ...

The more I think about the proposed change, the more I'm convince it's not a good idea but an anti-pattern.

Regards,

lmi

[pedrobaeza]

I totally don't agree with that, and I see a lot of approvals of this proposal, so how can we measure which is the good one?

[rousseldenis]

As mentioned above

[pedrobaeza]

You are doing a veto on something with divergent opinions. What you are going to get is that no guideline is put, and everyone does what they consider.

What I don't see is to wait for merging 3 auxiliary modules for a dumb option of putting a possible compatibility. That's something outside what you can ask for any contributor, while 2 lines, being "crap" or not, solves it. The other alternative is to find the incompatibilities with a hard debugging time, and add the compatibility on such cases. I suppose we are not the only one finding incompatibilities in our integration tests.

This one is not perfect, of course, but the alternative or doing nothing is worst IMO.

[sbidoul]

Do we have many examples? How prevalent is this problem?

[rousseldenis]

You are doing a veto on something with divergent opinions. What you are going to get is that no guideline is put, and everyone does what they consider.

I wanted to add some objectives facts on the approvals stats. Nothing more. I'm still open to debate. But in current state, I cannot go in your way.

[lmignon]

I have shared my concerns and explained why in my view this change is not appropriate and does not go in the direction of implementing quality development practices. At the same time, I propose an alternative approach to solve the problem outlined. I read in the comments several opinions of contributors expressing their disagreement with the proposed principle. In each of these opinions, it seems to me that the bias is to ensure that the need is well understood, to remain open to discussion and to challenge the problem exposed in order to respond appropriately. By not taking a direct position of rejection of the proposal in the comments, I think we leave more room for everyone to express themselves without entering into a binary confrontation (For or Against).
At this stage of the discussion, and since it seems that a decision has to be made, I am strongly against the proposal. (But I'm sill open to debate)

[pedrobaeza]

OK, good to see that you are open to debate. I have already counter-argument your proposal, but I will redact a more elaborated one to get to you all why this one can be a good base guideline.