From 3a4a7b161b47439ffda53c8aefc5aee2199211d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20De=20Caluw=C3=A9?= Date: Tue, 17 May 2022 08:31:53 +0000 Subject: [PATCH] [FIX] core: recompute fields triggered by indirectly modified relational fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache currectly fails to correctly invalidate relational fields that depend on a non-relational field. Two passes of invalidation are done, to reflect dependencies on both the old and the new written values. In the first pass only relational fields are considered, as explained in the comments: > It is best explained with a simple example: consider two sales orders SO1 and SO2. The computed total amount on sales orders indirectly depends on the many2one field 'order_id' linking lines to their sales order. Now consider the following code: > > line = so1.line_ids[0] # pick a line from SO1 > line.order_id = so2 # move the line to SO2 > > In this situation, the total amount must be recomputed on *both* sales order: the line's order before the modification, and the line's order after the modification. The written values can be seen as the roots of a dependency forest (a collection of dependency trees). Before this commit all non-relational roots and their corresponding trees were filtered out during the first pass. However, this approach is wrong, as relational fields can also depend on non-relational fields. Instead, the complete dependency forest has to be traversed, skipping invalidation for non-relational fields during the first pass. The test that was previously included accidentally succeeded because of a separate and unrelated bug in the orm domain parser: in certain one2many or many2many leafs the domain parser would not take into consideration the domain included in the definition of the field. As a result, the test still passed by accident, because the records that no longer matched the domain after the write were still invalidated during the second pass. The problem can clearly be demonstrated, however, when the dependency is generated by a compute function. closes odoo/odoo#101038 X-original-commit: d4a5827b42d80f0f830455dcd2056701eb09aed1 Signed-off-by: Rémy Voet Signed-off-by: Raphael Collet Co-authored-by: Raphael Collet --- .../test_new_api/models/test_new_api.py | 6 +++++ .../test_new_api/tests/test_one2many.py | 12 ++++++++++ odoo/models.py | 13 +++++----- odoo/modules/registry.py | 24 +++++++++++++++++++ 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/odoo/addons/test_new_api/models/test_new_api.py b/odoo/addons/test_new_api/models/test_new_api.py index ffed8a9caa0..225e193b783 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -1351,6 +1351,12 @@ class ComputeContainer(models.Model): name = fields.Char() member_ids = fields.One2many('test_new_api.compute.member', 'container_id') + member_count = fields.Integer(compute='_compute_member_count', store=True) + + @api.depends('member_ids') + def _compute_member_count(self): + for record in self: + record.member_count = len(record.member_ids) class ComputeMember(models.Model): diff --git a/odoo/addons/test_new_api/tests/test_one2many.py b/odoo/addons/test_new_api/tests/test_one2many.py index 21e25bd7e79..8dfa81c18b6 100644 --- a/odoo/addons/test_new_api/tests/test_one2many.py +++ b/odoo/addons/test_new_api/tests/test_one2many.py @@ -256,6 +256,18 @@ class One2manyCase(TransactionCase): # at this point, member.container_id must be computed for member to # appear in container.member_ids self.assertEqual(container.member_ids, member) + self.assertEqual(container.member_count, 1) + + # Changing member.name will trigger recomputing member.container_id, + # container.member_ids and container.member_count. Since we are setting + # the name to bar, it will be detached from container, resulting in a + # member_count of zero on the container. + member.name = 'Bar' + self.assertEqual(container.member_count, 0) + + # Reattach member to container again + member.name = 'Foo' + self.assertEqual(container.member_count, 1) def test_reward_line_delete(self): order = self.env['test_new_api.order'].create({ diff --git a/odoo/models.py b/odoo/models.py index 9d68c790aed..a7d8567ed18 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -4134,7 +4134,7 @@ class BaseModel(metaclass=MetaModel): field_values = [] # [(field, value)] determine_inverses = defaultdict(list) # {inverse: fields} - relational_names = [] + fnames_modifying_relations = [] protected = set() check_company = False for fname, value in vals.items(): @@ -4152,8 +4152,8 @@ class BaseModel(metaclass=MetaModel): # order to avoid an inconsistent update. self[fname] determine_inverses[field.inverse].append(field) - if field.relational or self.pool.field_inverses[field]: - relational_names.append(fname) + if field in self.pool.fields_modifying_relations: + fnames_modifying_relations.append(fname) if field.inverse or (field.compute and not field.readonly): if field.store or field.type not in ('one2many', 'many2many'): # Protect the field from being recomputed while being @@ -4197,7 +4197,7 @@ class BaseModel(metaclass=MetaModel): # In this situation, the total amount must be recomputed on *both* # sales order: the line's order before the modification, and the # line's order after the modification. - self.modified(relational_names, before=True) + self.modified(fnames_modifying_relations, before=True) real_recs = self.filtered('id') @@ -6649,7 +6649,8 @@ class BaseModel(metaclass=MetaModel): # Generic onchange method # - def _dependent_fields(self, field): + @classmethod + def _dependent_fields(cls, field): """ Return an iterator on the fields that depend on ``field``. """ def traverse(node): for key, val in node.items(): @@ -6657,7 +6658,7 @@ class BaseModel(metaclass=MetaModel): yield from val else: yield from traverse(val) - return traverse(self.pool.field_triggers.get(field, {})) + return traverse(cls.pool.field_triggers.get(field, {})) def _has_onchange(self, field, other_fields): """ Return whether ``field`` should trigger an onchange event in the diff --git a/odoo/modules/registry.py b/odoo/modules/registry.py index d286c3c0494..74137c17dc2 100644 --- a/odoo/modules/registry.py +++ b/odoo/modules/registry.py @@ -367,6 +367,30 @@ class Registry(Mapping): return triggers + @lazy_property + def fields_modifying_relations(self): + ''' + Return the union of the set of relational fields that are dependencies + of other fields, with the set of non-relational fields that are + dependencies of relational fields. + ''' + result = set() + + for field in self.field_triggers: + # If the field is itself a relational field, it is also considered + # as triggering relational fields. + if field.relational or self.field_inverses[field]: + result.add(field) + continue + + Model = self.models[field.model_name] + for dep in Model._dependent_fields(field): + if dep.relational or self.field_inverses[dep]: + result.add(field) + break + + return result + def post_init(self, func, *args, **kwargs): """ Register a function to call at the end of :meth:`~.init_models`. """ self._post_init_queue.append(partial(func, *args, **kwargs))