From 143cffe0016e4650d4e650c16f2a431a953d949c Mon Sep 17 00:00:00 2001 From: Adrian Torres Date: Thu, 11 Mar 2021 12:52:02 +0000 Subject: [PATCH] [FIX] base: reset _rec_name if x_name field is deleted Commit 6a0028f91944b1d9e4eac026e86ca249ef5bc7ee introduced a change that allowed field_triggers to be computed lazily per registry, this change introduced a behavioral change that is not easy to notice, in order to explain the behavioral change I will use the following example which was the original bug reported: - Install studio - Create an app (create a new custom model M) - Uninstall studio -> Uninstall fails because field display_name of the custom model M depends on field M.x_name which has been removed due to the uninstall process. To understand why it didn't happen before the aforementioned commit, we must first understand what happens with said commit applied: - We trigger the uninstall of studio, this triggers the uninstall of any modules that depend on it, namely studio_customizations which is the module in which all customizations done with studio live in. - We gather all data belonging to the studio_customization and we start deleting in the following order: ..., ir.model.fields.selection, ir.model.fields, ..., ir.model - During the unlink process, we first remove the actual fields from the model instances before deleting their database reflections, this is done in the ir.model.fields._drop_column() method, it is this method that will delete the x_name field but **not** the display_name field, since it is a base field and not a custom one. - After the deletion of the fields in memory, we call modified() to mark fields that might've depended on the fields we just modified so that they can be recomputed later. - The call to modified will in turn access field_triggers, but since we're in a new registry and field_triggers is a lazy property, it will be computed right at this moment, this means that it will call resolve_depends on the display_name field which still exists, and this field has a dependency on the x_name field that we just deleted! This is what will trigger the crash. With that context, we can now understand how it didn't crash before commit 6a0028f91944b1d9e4eac026e86ca249ef5bc7ee: - Before the aforementioned commit, the field_triggers attribute was computed during the registry's setup_models(), in the case of an uninstall this call to setup_models was done way before the uninstall step of the registry (Step 3 is the last to call setup_models before Step 5). - This means that the old behavior was technically a bug, because right after removing x_name from the model, the field_triggers still contained a dependency from display_name to x_name, the former being no-longer present in memory. With this commit, we simply reset the _rec_name and the dependencies of the display_name if the x_name field is being removed, this ensures that the computation of field_triggers won't crash and burn. opw-2452498 opw-2478589 closes odoo/odoo#67823 X-original-commit: e6d22a43ff9f40e5fc7b8c84cd4dfe43f926d872 Signed-off-by: Adrian Torres (adt) Co-authored-by: Raphael Collet --- odoo/addons/base/tests/test_ir_model.py | 14 ++++++++++++++ odoo/models.py | 13 +++++-------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/odoo/addons/base/tests/test_ir_model.py b/odoo/addons/base/tests/test_ir_model.py index 385bca0284e..0ea1341daa8 100644 --- a/odoo/addons/base/tests/test_ir_model.py +++ b/odoo/addons/base/tests/test_ir_model.py @@ -312,6 +312,20 @@ class TestIrModel(TransactionCase): }] self.assertEqual(groups, expected, 'should include 2 empty ripeness stages') + def test_rec_name_deletion(self): + """Check that deleting 'x_name' does not crash.""" + record = self.env['x_bananas'].create({'x_name': "Ifan Ben-Mezd"}) + self.assertEqual(record._rec_name, 'x_name') + self.assertEqual(record._fields['display_name'].depends, ('x_name',)) + self.assertEqual(record.display_name, "Ifan Ben-Mezd") + + # unlinking x_name should fixup _rec_name and display_name + self.env['ir.model.fields']._get('x_bananas', 'x_name').unlink() + record = self.env['x_bananas'].browse(record.id) + self.assertEqual(record._rec_name, None) + self.assertEqual(record._fields['display_name'].depends, ()) + self.assertEqual(record.display_name, f"x_bananas,{record.id}") + @tagged('test_eval_context') class TestEvalContext(TransactionCase): diff --git a/odoo/models.py b/odoo/models.py index 8b9c656cad0..5487606f1c3 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -412,6 +412,10 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): field = cls._fields.pop(name, None) if hasattr(cls, name): delattr(cls, name) + if cls._rec_name == name: + # fixup _rec_name and display_name's dependencies + cls._rec_name = None + cls.display_name.depends = tuple(dep for dep in cls.display_name.depends if dep != name) return field @api.model @@ -2862,14 +2866,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): raise for name in bad_fields: - del cls._fields[name] - delattr(cls, name) - - # fix up _rec_name - if 'x_name' in bad_fields and cls._rec_name == 'x_name': - cls._rec_name = None - field = cls._fields['display_name'] - field.depends = tuple(name for name in field.depends if name != 'x_name') + self._pop_field(name) @api.model def _setup_complete(self):