[IMP] util/records: enforce cascade removal for actions

[pauloamed]

The implementation of the python inheritance mechanism between the base class ir.actions.actions and its child classes (eg. ir.actions.act_window) does not allow the creation of foreign keys when ir.actions.actions is a M2O field of another model; what leads to the not execution of some constraints, one of them being the ondelete='cascade' constraint, which is set in PSQL level.

That said, when a ir.actions.actions record is deleted, if it is being referenced as a M2O field by another model (eg. ir.filters), records from this second model won't be affected, what leads to undesired behaviour: a MissingError in the UI, indicating that the action was deleted.

Such behaviour of not creating foreign keys and thus constraints is specific to ir.actions.actions.
This commit remedies this specific case, removing records with a M2O field to ir.actions.actions with ondelete=cascade when the action referenced in such field is removed using upgrade-util.

[robodoo]

Pull request status dashboard.

[pauloamed]

I do not intend to leave these NOTES in the final diff, these are only for discussions in this PR

[aj-fuentes]

You could move the notes from the code to the PR. Add them as comments here.

[aj-fuentes]

We could have a hard-coded list of standard action models. We don't need to take into account all possible models extending the actions base model. We don't that for views for example.

[KangOl]

We can just use for_each_inherit

[pauloamed]

my point in leaving it as a SQL query is not only to encompass custom models but also reduce the maintainability overhead in case there is any change in standard models. what do you think?

[aj-fuentes]

The SQL query version plus the query itself adds complexity, it's cleaner with for_each_inherit, IMO.

[KangOl]

OTOH, the list of models that are problematic (the ones that are using INHERIT at PostgreSQL level) is known. We can hard-code it.

Note that this list changed over time, so we need to list all of them.

[pauloamed]

the ones that are using INHERIT at PostgreSQL level

AFAIU, the issue also encompasses models that would inherit ir.actions.actions without using psql's INHERIT mechanism*, since this concerns fields to ir.actions.actions (as a Polymorphism mechanism), and not its child classes.

*: I've just tested creating a new model inheriting from ir.actions.actions, similar to the standard ones, and this new model's table does not have Inherits: ir_actions, whereas the ones from standard do have.

So I understand that list of models returned by for_each_inherit should be equal to the ones to be hard-coded (or used in a hard-coded list) for a specific version.

That said, having in mind that for_each_inherit performs a SQL query itself, I believe that:

• hard-coding > for_each_inherit: more performatic
• for_each_inherit > hard-coding: easier to maintain

[KangOl]

You should also search the one linking to the xmlid's model

Suggested change

	cr.execute("SELECT name, model FROM ir_model_fields WHERE relation = 'ir.actions.actions' AND on_delete ='cascade'")
	cr.execute("SELECT name, model FROM ir_model_fields WHERE relation IN %s AND on_delete ='cascade'", [tuple({model, "ir.actions.actions"})])

[pauloamed]

I'm not sure I understood, maybe this is linked to the comment above?

I took the example of website.snippet.filter that has a M2O field to ir.actions.server. By examining it's table, we have:

Foreign-key constraints:
    "website_snippet_filter_action_server_id_fkey" FOREIGN KEY (action_server_id) REFERENCES ir_act_server(id) ON DELETE CASCADE
    "website_snippet_filter_create_uid_fkey" FOREIGN KEY (create_uid) REFERENCES res_users(id) ON DELETE SET NULL
    "website_snippet_filter_filter_id_fkey" FOREIGN KEY (filter_id) REFERENCES ir_filters(id) ON DELETE CASCADE
    "website_snippet_filter_website_id_fkey" FOREIGN KEY (website_id) REFERENCES website(id) ON DELETE CASCADE
    "website_snippet_filter_write_uid_fkey" FOREIGN KEY (write_uid) REFERENCES res_users(id) ON DELETE SET NULL

That is, the foreign-key to ir.actions.server is there (first line), so we can expect correct behaviour.

The issue is when we have a field to ir.actions.actions for polymorphism.

If indeed a confusion happened, please let me know so I can improve the commit message.

[KangOl]

Can you add a test?

[pauloamed]

Hey I believe we can revert the following PR https://github.com/odoo/upgrade/pull/5869 before this once is merged

[KangOl]

Hey I believe we can revert the following PR odoo/upgrade#5869 before this once is merged

No, this is still needed for actions not deleted via this new helper.

[pauloamed]

@KangOl @aj-fuentes ping 🏓

[erl-odoo]

Any news here? I've discovered another issue with filters on an upgrade project, though it's not related to upgrading technically -> install a module, like hr_holidays, save a filter, uninstall the module = the filter is now corrupt because of the missing model in registry:

IMO, apart from this util helper, either:
1 - have a base script in upgrade that fixes the filters on upgrade, removing ones that have missing actions like the example in this PR and were NOT deleted with this helper, and removes also ones with missing model like I describe above

2 - I fix this in a PR to odoo to cleanup the filters on module / model removal

wdyt? @aj-fuentes @KangOl

[aj-fuentes]

@erl-odoo The uninstall process of standard Odoo is full of corner cases where data is left in a wrong state, this is yet another example :)

IMO yes you can try to fix the standard process to cleanup those filters in master odoo. During upgrades, removing what seems to be broken filters may be acceptable, but we usually try to avoid removing data (they may work in case the model re-appears after a reinstall). Generally speaking, if it was broken before the upgrade and doesn't fail the upgrade, we don't fix it.