[15.0][OU] Add domain field list

[ernestotejeda]

Add a list of domain fields that will be used by openupgradelib to update domain fields when renaming fields. OCA/openupgradelib#319

This list must be maintained manually since there seems to be no way to know when a Char field is used to store a domain and when it is not. This list should also be adapted and included in other versions.

[OCA-git-bot]

Hi @legalsylvain, @StefanRijnhart,
some modules you are maintaining are being modified, check this out!

[ernestotejeda]

@pedrobaeza It is ready for review

[legalsylvain]

Hi. @ernestotejeda. Thanks for the improvment.

However, I think that there is something wrong in the general design.
openupgrade_scripts.pre-migration.migrate() -> openupgradelib.rename_fields() -> openupgradelib.update_domain_fields() -> openupgrade_framework.domain_fields

That makes openupgrade -> openupgradelib -> openupgrade that is not good.

That settings doesn't have to be in the openupgrade_framework module. The objective of this module is to contains odoo patches.

Not sure to understand all the algorithm, but I think that we should :

• add extra optional values in the field_spec args of openpugradelib.rename_fields()
• and set all the tuples in each according pre-migration scripts. (base_automation, coupon, etc...)

What do you think ?

CC : @StefanRijnhart

[pedrobaeza]

It's not an easy design decision, and it has been well though, but there's no good one.

The problem

We have some fields that represents a domain, both on Odoo core (like mass_mailing) or OCA modules (like base_tier_validation). You can't know which ones are of this kind by introspection, as there's no such a specific field type for them.

When a field is renamed between versions, if one of these domains contain such field, it will give error the next time the domain is used.

We have an openupgradelib method rename_fields for handling when a field changes its name, but this renaming is done on in the pre-migration of the module that declares the field, not on the module that declares the widget field, so both don't know each other.

What we want is to be able to have dynamically adjusted these domains with the new field name for avoiding "post-migration runtime" errors and needing to manually change each of them.

Current solution

Maintain a list of static mappings of model, domain field and field referencing the model per version, and use it each time rename_fields is invoked.

Pros

• Integrated in the OpenUpgrade flows.
• Everyone can expand the list with their OCA modules just doing a PR on OpenUpgrade.
• Each version keeps their own mapping, as they can be very different.
• Resilient enough, checking the existence of both the dictionary and the target model/fields.

Cons

• Coupling between openupgrade_framework and openupgradelib. This coupling was already between openupgradelib and Odoo, so it's not too much. If the module doesn't exist, the method will act as previously.
• If doing a migration inside the same version, for this to work requires to have the module openupgrade_framework in your path.

[legalsylvain]

Hi @pedrobaeza. Thanks a lot for the explanation. The need is totally legit, and your argument makes senses. Indeed, having a fields.
However, I'd like to find a way that avoid that circular reference And (/or) the second point of your cons, that is quite annoying (*) because I guess all people making minor update of modules in their production instance will not add the module openupgrade_framework in the addons path. As a result, that improvment will not be used.
I don't have a clear idea how to realize that. Maybe @OCA/openupgrade-maintainers could have an idea ? I'll come back to it in a few days with some alternative ideas, if I've found any.

(*) : If doing a migration inside the same version, for this to work requires to have the module openupgrade_framework in your path.

CC : @StefanRijnhart

[pedrobaeza]

Note: you won't have any error, but you won't benefit either from this feature not having openupgrade_framework in the addons path.

[hbrunn]

how about recording the fields having been renamed in some global variable (or a table?) and postprocessing the domain fields in end_migration?

And shouldn't it be possible to detect those fields by searching for widget="domain" in all views?

[pedrobaeza]

We have already talked internally about both options:

• Storing things in a table is not a good option, due to 2 things: the possibility of having a table with incorrect data is high (old data, restarted migrations...), and anyway, the data won't be fed at the correct moment unless you do 2 migration passes.
• We also checked to have a computed m2m stored field at ir.ui.view level that writes the fields found with widget="domain", but that meant a drain in performance on any module update, as Odoo always rewrite the arch, or having to do the search for each call of the rename_fields method.

[legalsylvain]

Hi. Thinking again I think that the current file (openupgrade_framework/domain_fields.py) in V15 should be moved in openupgradelib, in openupgradelib/domain_fields_150.py.

Pro :

• A) It avoids the circular reference (openupgrade depends on openupgradelib that depends on openupgrade).
• B) It allow more easily to test the features in the openupgradelib CI.
• C) But overall, the features will work on "minor" upgrade. (16.0.1 -> 16.0.2). Sometimes, in OCA (or custom modules), there is a refactor inside the same revision and this feature is interesting. if we set the configuration in OCA/openupgrade, it will not be pulled. (openupgrade is never used out of major upgrade), as a result this new feature will be disabled, that is a pity.

Cons :

• I don't see any but feel free to add things.

it's the least disturbing design I can think of.

[MiquelRForgeFlow]

1. I will express my point of view:

That settings doesn't have to be in the openupgrade_framework module. The objective of this module is to contains odoo patches.

I fully agree with this.

Maintain a list of static mappings of model, domain field and field referencing the model per version, and use it each time rename_fields is invoked.

I agree.

I think that the current file (openupgrade_framework/domain_fields.py) in V15 should be moved in openupgradelib, in openupgradelib/domain_fields_150.py

I don't agree.

2. I propose the following solution:

In openupgrade_scripts, the same way we have apriori.py, we should have another domain_fields.py. Then, the same way in pre-migration of base we call the apriori.py, we should call the domain_fields.py in the code whenever we are calling rename_fields. Here is an example (taken from project in v15):

from odoo.addons.openupgrade_scripts.domain_fields import domain_fields
 
openupgrade.rename_fields(
        env,
        [
            ("project.task", "project_task", "dependency_task_ids", "depend_on_ids"),
            ("project.task", "project_task", "depending_task_ids", "dependent_ids"),
        ],
        domain_fields=domain_fields
    )

where domain_fields is an optional parameter in rename_fields.

3. Comparison

Pro: All previous pros.
Cons: No cons.

What do you all think of this?

[legalsylvain]

Pro: All previous pros.

Sorry @MiquelRForgeFlow, but I don't see why putting in openupgrade_scripts instead of openupgrade_framework solves the problem I mentioned. Specially the C point. Could you explain how the design you proposee will work in a case of an update between same release. (minor update) ?

• https://github.com/OCA/project/blob/12.0/project_role/migrations/12.0.2.1.0/pre-migration.py
• https://github.com/OCA/product-attribute/blob/12.0/product_dimension/migrations/12.0.2.0.0/pre-migration.py
• https://github.com/OCA/reporting-engine/blob/13.0/base_comment_template/migrations/13.0.2.0.0/pre-migration.py
• ...

Thanks !

[MiquelRForgeFlow]

but I don't see why putting in openupgrade_scripts instead of openupgrade_framework solves the problem I mentioned.

The same way you say putting in openupgrade_framework doesn't make sense because the objective of this module is to contains odoo patches exclusively, we can easily say and come to agree that openupgradelib is a library that it's not expected to be patched assiduously and by many people. Instead, OpenUpgrade project, and specifically openupgrade_scripts, is expected and desired to have a lot of contributions, with a continuous flux of updates. And domain_fields.py, in my point of view, falls under the later (like apriori.py) than the former.

Specially the C point. Could you explain how the design you proposee will work in a case of an update between same release. (minor update) ?

Easy-peasy. In these cases, you only have to modify the script by adding the parameter 🤷‍♂️:

openupgrade.rename_fields(env, field_renames, domain_fields=domain_fields)

Of course, you may argue that this implies to do a lot of PRs in order to adapt to this logic and than doing that it's too much work. And I would say, yes, it's work, but we can begin by doing it only in the important cases and not exhaustively. We do PRs near everyday and having to do a limited bit more shouldn't discourage us. Clearly it's not a retroactive feature, so any field_rename call without domain_fields parameter will not handle domains, and you may put this in the cons. But in my point of view, which is my argument, is that until now, we have worked without handling domains "very happily" and could still live without it, at least in old versions. I think that wanting to implement this kind of feature in a way that is full retroactive by all field_rename calls it's not very feasible if we want to be full coherent with all the previous statements done about the use of openupgrade_framework and openupgradelib. Sometimes, the perfect and ideal solution doesn't exist, and still we should compromise with one. In an ideal world, for example, the rename_models method would pass the env instead of a cursor, but it's not the case.

[legalsylvain]

Hi @MiquelRForgeFlow. sorry if I misspoke. I don't think the solution I'm proposing (openupgradelib) has only advantages. You're right, there are points where it's less good.
Here are the details of my thoughts, with the pros and cons of each solution. What do you think?

1) Is it the good place ?

• openupgradelib : 👎 No : openupgradelib is a library that provides "tools functions". It's not intuitive that it contains settings files.
• openupgrade_framework : 👎 No : Contains only odoo "patches". Should be kept as minimal as possible.
• openupgrade_scripts : 👎 No : Contains only "diff" information (=IE : changes between version N-1 and version N). domain_fields describes the version N.

2) Easy to Contribute

• openupgradelib : 👎 Not really, openupgradelib is quite confidential. (only 2 / 3 contributors).
• openupgrade_framework : 👎 Not really. this module is totally obscure for most users.
• openupgrade_scripts : 🆗 Yes. This is the best-known module of the "OpenUpgrade" project, and the apriori.py file is also well known (it needs to be patched, if you refactor custom code).

3) Circular dependency

• openupgradelib : 🆗 No.
• openupgrade_frameworks : 👎 Yes.
• openupgrade_scripts : 👎 Yes.

4) Works retroactively

• openupgradelib : 🆗 Yes. nothing to do. Once implemented in the library, available in all usage.
• openupgradelib_framework : 👎 No, we have to change two lines in all the existing code. Add an import and add an argument in the call of rename_fields. (domain_fields=domain_fields)
• openupgrade_scripts : 👎 No. (same arguments)

5) Easy to develop

• openupgradelib : 🆗 Yes. openupgradelib works as a black box. it will do the magic without adding extra args.
• openupgradelib_framework : 👎 No, the developper has to make the import and think to add the parameter.
  Add : from odoo.addons.openupgrade_scripts.domain_fields import domain_fields
  and add domain : openupgrade.rename_fields(env, field_renames, domain_fields=domain_fields)
• openupgrade_scripts : 👎 No. (same arguments)

6) Deployment changes on minor update (inside the same Odoo release)

• openupgradelib : 🆗 No. nothing to add in the deployment.
• openupgradelib_framework : 👎 yes. Requires to install the module (or pull the code)
• openupgrade_scripts : 👎 Requires to install the module (or pull the code)

7) Deployment changes on major update (between 2 major odoo releases)

• openupgradelib : 🆗 No. (nothing to add in the deployment)
• openupgradelib_framework : 🆗 No. (nothing to add in the deployment)
• openupgrade_scripts : 🆗 No. (nothing to add in the deployment)

[pedrobaeza]

@legalsylvain the chosen solution is the best minimizing the cons and favoring the contribution, as everything is contained in one file. The circular dependency you mention is relative, as this will work if the import can't be done, so we need to unblock this.

You are not proposing a viable solution, nor contributing code to it. We have spent a lot of hours designing this for all to benefit from it. Let's merge this as a first solution for a non solved problem, and if there can be future improvements (on our part trying this or on any other contributor part), let's accept it.

[legalsylvain]

Hi @pedrobaeza.

the chosen solution is the best minimizing the cons and favoring the contribution, as everything is contained in one file.

could you elaborate ? I propose to have a file in openupgradelib, and you propose to change a file in openupgrade project. That's the same complexity.

You are not proposing a viable solution

could you elaborate ?

• The solution is to move the file in openupgradelib.
• I think it's viable, see my previous comment that was quite detailed. [15.0][OU] Add domain field list #3892 (comment) .

You are [...] nor contributing code to it

As soon as we are agree with the design, i can make a PR against OCA/openupgradelib#319

I'm open to discuss, but I didn't received any counter argument for a month and a half, and now you ask to merge this PR. I don't get it.

If my proposal is not clear, we can do an online meeting.

Kind regards.

[pedrobaeza]

The solution is to move the file in openupgradelib.

I think that such solution is worst than the current one:

• It forces people to make 2 PRs (one on OpenUpgrade, and another in openupgradelib), when, analyzing a module, you see that a
• It mixes in the same place the applicable fields for all the Odoo versions. It also means a risk of side effects modifying something that can affect other Odoo versions, while you are thinking only in one specific Odoo version.
• It mixes "data" and code in openupgradelib, while till now, data like field renamings and so on have always been in OpenUpgrade.
• It bloats openupgradelib.

About your points:

• Circular dependency: As stated, that's not really a problem. This will work if the import can't be done.
• Easy to Contribute: It's the same to touch a file similar to apriori.py inside openupgrade_framework than inside openupgrade_scripts.
• Works retroactively: Your sentence about being better in openupgradelib is not true, as you can't assume all the mappings will be valid for all the versions. In the best case, it will crash. In the worst one, it will damage data. This should be added version per version.
• Deployment changes on minor update: This is the only real cons, but being a regular Odoo addon, openupgrade_framework can be added to the addons path easily, and it's harmless if you don't have it. And a field renaming operation is very unfrequent inside same version (and even between major versions, as OCA modules DB layout is pretty stable).

So seeing pros and cons, I still go with the current solution.

[legalsylvain]

Hi @pedrobaeza. Thanks for your answer.
When I see your answer, I think you did not understand my proposal. (I may not have explained it well, this is not a criticism).
I will try to do a poc in the next few days, and I will ping you.
To see if the code explains my thinking better than my comments !

[ndd-odoo]

this one is same with rename_models right, should we handle that as well?
Ex: many fields of many modules that store the name of a model (field res_model in mail.activity )
cc @pedrobaeza @legalsylvain

[pedrobaeza]

I don't get your question, Nguyễn

[ndd-odoo]

I don't get your question, Nguyễn

I mean that model renaming takes place a lot across all version migration right, and openupgradelib has rename_models so i suggest we do something similar to this one, define something like _fields_of_models_that_contain_model_name and then use that whenever rename_models is called to replace the new_module name in the field that contain the old one

[pedrobaeza]

Yes, it can be another improvement, but it's out of scope of this one.

[ndd-odoo]

Yes, it can be another improvement, but it's out of scope of this one.

OK thanks