[MERGE] core: introduce mechanism for `selection_add` cleanup #46325

Elkasitu · 2020-02-26T11:04:09Z

Context and rationale

Let A and B be two different modules.
A defines a required Selection field F of model M and B extends
it through the selection_add argument.
Create records of model M and have some of them have any of the
options introduced by module B selected for field F.
Uninstall module B.

The result will be records of model M with an option for field F that no
longer exists, this makes the registry inconsistent and prone to
crashing (it is sufficient to access the form view of such a record to
trigger a crash).

This commit introduces a mechanism similar to the ondelete argument
found in Many2one fields, the argument name is the same but it's
different both in behaviour and in implementation.

The ondelete mechanism for Selection fields is enforced for any
Selection field with required set to True, this means that the
developer is required to set a cleanup behaviour for when their module
is uninstalled. For possible cleanup options, see the fields.Selection
docstring.

As far as implementation goes, everything is implemented in Python
unlike with Many2one fields where the behaviour is delegated to
PostgreSQL.

The ondelete setting will be processed during
ir.model.fields.selection.unlink() to ensure that the registry is left
in an appropriate state after module uninstall.

To do

Write rationale and such
~~Factorize _update_selections()~~
~~Handle custom field creation~~
Update documentation
Merge all business code modifications into single commit (post-review)
Clean up the multiple uninstall_hook that handle this locally
Treat None as 'null'

odoo/addons/base/models/ir_model.py

odoo/addons/test_new_api/tests/test_schema.py

odoo/fields.py

xmo-odoo

maybe a restrict option (requiring the user to fix their records first) would make sense?
warning about the updates might be a good idea as well as some of the changes might be pretty brutal

odoo/addons/base/models/ir_model.py

odoo/addons/test_new_api/models/test_new_api.py

xmo-odoo · 2020-03-30T10:03:24Z

odoo/fields.py

+                ondelete = field.args.get('ondelete') or {}
+                new_values = [kv[0] for kv in selection_add if kv[0] not in values]
+                for key in new_values:
+                    ondelete.setdefault(key, 'set null')


Doesn't that mean there's always a value and the code in ir_model has no need to check for a missing / empty ondelete?

Computed selection fields will have their ondelete set to None (see above), if they don't define a selection_add this will remain as None and they will be treated the same as non-compute selection fields during _process_ondelete

xmo-odoo · 2020-03-30T10:11:23Z

addons/stock/models/barcode.py

+        'weight': 'set default',
+        'location': 'set default',
+        'lot': 'set default',
+        'package': 'set default',


Did stock say to do that, rather than e.g. delete the rule? Because it seems a bit weird that we'd just convert stock barcode rules to "unit product"

I invited them to review the code and they didn't seem to object to this particular change, but maybe you're right.

cc @sle-odoo

Elkasitu · 2020-03-30T11:57:00Z

@xmo-odoo Don't think it makes sense to have an ondelete='restrict', this won't stop the uninstall anyway (like with m2o currently)

Thanks for the review

xmo-odoo · 2020-03-30T12:05:46Z

@xmo-odoo Don't think it makes sense to have an ondelete='restrict', this won't stop the uninstall anyway (like with m2o currently)

Won't it? Couldn't we raise on a restrict if we find matching records? Though we can always add that option later if we have a use case for it anyway.

This is necessary in order to create models on-the-fly within tests and to test improper Model / Field creation: registry.reset_changes will only perform the reset of the registry if it is invalidated, however if the setup of models crashes before it is invalidated (as is the case of a test), the reset_changes will not be triggered and thus the registry will be left dirty for subsequent tests.

- Let A and B be two different modules. - A defines a required Selection field F of model M and B extends it through the `selection_add` argument. - Create records of model M and have some of them have any of the options introduced by module B selected for field F. - Uninstall module B. The result will be records of Model M with an option for field F that no longer exists, this makes the registry inconsistent and prone to crashing (it is sufficient to access the form view of such a record to trigger a crash). This commit introduces a mechanism similar to the `ondelete` argument found in Many2one fields, the argument name is the same but it's different both in behaviour and in implementation. The `ondelete` mechanism for Selection fields is enforced for **any** Selection field with required set to `True`, this means that the developer is required to set a cleanup behaviour for when their module is uninstalled. For possible cleanup options, see fields.Selection's docstring. As far as implementation goes, everything is implemented in Python unlike with Many2one fields where the behaviour is delegated to PostgreSQL. The `ondelete` setting will be processed during `ir.model.fields.selection.unlink()` to ensure that the registry is left in an appropriate state after module uninstall.

With this commit, Selection fields with `required=True` which are extended via `selection_add` are given proper ondelete policies to ensure the cleanup of records containing these extended options during uninstall of the extending module. This commit also cleans up leftover uninstall hooks that were being used to handle the same set of problems prior to the ondelete mechanism being implemented for Selection fields.

rco-odoo · 2020-03-30T14:07:20Z

@robodoo rebase-ff r+

robodoo · 2020-03-30T14:07:31Z

Merge method set to rebase and fast-forward

With this commit, Selection fields with `required=True` which are extended via `selection_add` are given proper ondelete policies to ensure the cleanup of records containing these extended options during uninstall of the extending module. This commit also cleans up leftover uninstall hooks that were being used to handle the same set of problems prior to the ondelete mechanism being implemented for Selection fields. closes #46325 Related: odoo/enterprise#9117 Signed-off-by: Raphael Collet (rco) <rco@openerp.com>

rco-odoo · 2020-03-30T15:17:32Z

@Elkasitu good job!

With this commit, Selection fields with `required=True` which are extended via `selection_add` are given proper ondelete policies to ensure the cleanup of records containing these extended options during uninstall of the extending module. This commit also cleans up leftover uninstall hooks that were being used to handle the same set of problems prior to the ondelete mechanism being implemented for Selection fields. closes odoo#46325 Related: odoo/enterprise#9117 Signed-off-by: Raphael Collet (rco) <rco@openerp.com>

robodoo added seen 🙂 CI 🤖 Robodoo has seen passing statuses labels Feb 26, 2020

C3POdoo added the RD research & development, internal work label Feb 26, 2020

Elkasitu requested review from rco-odoo and mart-e February 27, 2020 15:18

robodoo removed the CI 🤖 Robodoo has seen passing statuses label Feb 28, 2020

Elkasitu force-pushed the master-selection-hook-adt branch from 6312f3e to edb1d16 Compare February 28, 2020 12:46

robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 28, 2020

Elkasitu marked this pull request as ready for review February 28, 2020 13:53

robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 2, 2020

Elkasitu force-pushed the master-selection-hook-adt branch from 0418488 to 9994548 Compare March 4, 2020 10:00

robodoo added the CI 🤖 Robodoo has seen passing statuses label Mar 4, 2020