From 232f34d2267f7fcdd358b14bbce2cff2781846c2 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Fri, 17 Nov 2023 12:15:50 +0000 Subject: [PATCH] [FIX] core: modified() on recursive fields before unlink() The following situation happened with module industry_fsm, when trying to delete a cancelled sales order corresponding to a task. When doing so, the server crashes with error "Could not find all values of X to flush them", which means that a dirty field (pending update) has lost its value from cache. The issue is related to recursive computed fields. Before deleting a record, method unlink() invokes modified(), which determines all the fields that depend on the record to be deleted, and marks them to recompute. Those fields should be recomputed after the record is deleted, and not before. We found out that the recursive call to modified() made for recursive fields can force the recomputation of the recursive field itself before the record is deleted, which causes unlink() to crash. The fix consists in marking the fields for recomputation at the very end of method modified(), after all the fields to recompute have been determined. This ensures that the processing of recursive fields always uses the current value of the field instead of its recomputed value. X-original-commit: 1f5293bc63ae7b06fd0df8d509dc7fafaa016a22 Part-of: odoo/odoo#143644 --- .../test_new_api/models/test_new_api.py | 39 +++++++ .../test_new_api/security/ir.model.access.csv | 3 + .../test_new_api/tests/test_new_fields.py | 19 ++++ odoo/models.py | 102 +++++++++++------- 4 files changed, 124 insertions(+), 39 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 42bfb114eec..f3217e8320e 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -654,6 +654,45 @@ class ComputeRecursiveTree(models.Model): rec.display_name = '%s(%s)' % (rec.name, ', '.join(children_names)) +class ComputeRecursiveOrder(models.Model): + _name = _description = 'test_new_api.recursive.order' + + value = fields.Integer() + + +class ComputeRecursiveLine(models.Model): + _name = _description = 'test_new_api.recursive.line' + + order_id = fields.Many2one('test_new_api.recursive.order') + task_ids = fields.One2many('test_new_api.recursive.task', 'line_id') + task_number = fields.Integer(compute='_compute_task_number', store=True) + + # line.task_number indirectly depends on recursive field task.line_id, and + # is triggered by the recursion in modified() on field task.line_id + @api.depends('task_ids') + def _compute_task_number(self): + for record in self: + record.task_number = len(record.task_ids) + + +class ComputeRecursiveTask(models.Model): + _name = _description = 'test_new_api.recursive.task' + + value = fields.Integer() + line_id = fields.Many2one('test_new_api.recursive.line', + compute='_compute_line_id', recursive=True, store=True) + + # the recursive nature of task.line_id is a bit artificial, but it makes + # line.task_number be triggered by a recursive call in modified() + @api.depends('value', 'line_id.order_id.value') + def _compute_line_id(self): + # this assignment forces the new value of record.line_id to be dirty in cache + self.line_id = False + for record in self: + domain = [('order_id.value', '=', record.value)] + record.line_id = record.line_id.search(domain, order='id desc', limit=1) + + class ComputeCascade(models.Model): _name = 'test_new_api.cascade' _description = 'Test New API Cascade' diff --git a/odoo/addons/test_new_api/security/ir.model.access.csv b/odoo/addons/test_new_api/security/ir.model.access.csv index 8207897d07e..012148d255e 100644 --- a/odoo/addons/test_new_api/security/ir.model.access.csv +++ b/odoo/addons/test_new_api/security/ir.model.access.csv @@ -23,6 +23,9 @@ access_test_new_api_compute_readonly,access_test_new_api_compute_readonly,model_ access_test_new_api_multi_compute_inverse,access_test_new_api_multi_compute_inverse,model_test_new_api_multi_compute_inverse,base.group_system,1,1,1,1 access_test_new_api_recursive,access_test_new_api_recursive,model_test_new_api_recursive,base.group_system,1,1,1,1 access_test_new_api_recursive_tree,access_test_new_api_recursive_tree,model_test_new_api_recursive_tree,base.group_system,1,1,1,1 +access_test_new_api_recursive_order,access_test_new_api_recursive_order,model_test_new_api_recursive_order,base.group_system,1,1,1,1 +access_test_new_api_recursive_line,access_test_new_api_recursive_line,model_test_new_api_recursive_line,base.group_system,1,1,1,1 +access_test_new_api_recursive_task,access_test_new_api_recursive_task,model_test_new_api_recursive_task,base.group_system,1,1,1,1 access_test_new_api_cascade,access_test_new_api_cascade,model_test_new_api_cascade,base.group_system,1,1,1,1 access_test_new_api_compute_readwrite,access_test_new_api_compute_readwrite,model_test_new_api_compute_readwrite,base.group_system,1,1,1,1 access_test_new_api_compute_onchange,access_test_new_api_compute_onchange,model_test_new_api_compute_onchange,base.group_system,1,1,1,1 diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index d2f5370f1e0..8a71df105c0 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -505,6 +505,25 @@ class TestFields(TransactionCaseWithUserDemo): baz = foo.create({'name': 'baz', 'parent_id': bar.id}) self.assertEqual(foo.display_name, 'foo(bar(baz()))') + def test_12_recursive_unlink(self): + order = self.env['test_new_api.recursive.order'].create({'value': 42}) + line = self.env['test_new_api.recursive.line'].create({'order_id': order.id}) + task = self.env['test_new_api.recursive.task'].create({'value': 42}) + self.assertEqual(task.line_id, line) + self.assertEqual(line.task_ids, task) + self.assertTrue(line.task_number) + + # Before deleting order, the following are marked to recompute: + # - task.line_id (recursive, depends on task.line_id.order_id.value) + # - line.task_number (implicitely depends on line.task_ids.line_id) + # + # If task.line_id is ever recomputed in order to mark line.task_number, + # its recomputed value will be lost in the cache invalidation, and + # there will be nothing left to write in the database afterwards! This + # makes the call to unlink() crash in that case. + # + order.unlink() + def test_12_cascade(self): """ test computed field depending on computed field """ message = self.env.ref('test_new_api.message_0_0') diff --git a/odoo/models.py b/odoo/models.py index a8e3c362e82..700c4055d2c 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -6677,6 +6677,63 @@ class BaseModel(metaclass=MetaModel): # - mark I to recompute on inverse(W, inverse(X, records)), # - mark J to recompute on inverse(Y, records). + if before: + # When called before modification, we should determine what + # currently depends on self, and it should not be recomputed before + # the modification. So we only collect what should be marked for + # recomputation. + marked = self.env.all.tocompute # {field: ids} + tomark = defaultdict(OrderedSet) # {field: ids} + else: + # When called after modification, one should traverse backwards + # dependencies by taking into account all fields already known to + # be recomputed. In that case, we mark fieds to compute as soon as + # possible. + marked = {} + tomark = self.env.all.tocompute + + # determine what to trigger (with iterators) + todo = [self._modified([self._fields[fname] for fname in fnames], create)] + + # process what to trigger by lazily chaining todo + for field, records, create in itertools.chain.from_iterable(todo): + records -= self.env.protected(field) + if not records: + continue + + if field.recursive: + # discard already processed records, in order to avoid cycles + if field.compute and field.store: + ids = (marked.get(field) or set()) | (tomark.get(field) or set()) + records = records.browse(id_ for id_ in records._ids if id_ not in ids) + else: + records = records & self.env.cache.get_records(records, field) + if not records: + continue + # recursively trigger recomputation of field's dependents + todo.append(records._modified([field], create)) + + # mark for recomputation (now or later, depending on 'before') + if field.compute and field.store: + tomark[field].update(records._ids) + else: + # Don't force the recomputation of compute fields which are + # not stored as this is not really necessary. + self.env.cache.invalidate([(field, records._ids)]) + + if before: + # effectively mark for recomputation now + for field, ids in tomark.items(): + records = self.env[field.model_name].browse(ids) + self.env.add_to_compute(field, records) + + def _modified(self, fields, create): + """ Return an iterator traversing a tree of field triggers on ``self``, + traversing backwards field dependencies along the way, and yielding + tuple ``(field, records, created)`` to recompute. + """ + cache = self.env.cache + # The fields' trigger trees are merged in order to evaluate all triggers # at once. For non-stored computed fields, `_modified_triggers` might # traverse the tree (at the cost of extra queries) only to know which @@ -6684,47 +6741,14 @@ class BaseModel(metaclass=MetaModel): # fields have no data in cache, so they can be ignored from the start. # This allows us to discard subtrees from the merged tree when they # only contain such fields. - cache = self.env.cache - tree = self.pool.get_trigger_tree( - [self._fields[fname] for fname in fnames], - select=lambda field: (field.compute and field.store) or cache.contains_field(field), - ) + def select(field): + return (field.compute and field.store) or cache.contains_field(field) + + tree = self.pool.get_trigger_tree(fields, select=select) if not tree: - return + return () - # determine what to compute (through an iterator) - tocompute = self.sudo().with_context(active_test=False)._modified_triggers(tree, create) - - # When called after modification, one should traverse backwards - # dependencies by taking into account all fields already known to be - # recomputed. In that case, we mark fieds to compute as soon as - # possible. - # - # When called before modification, one should mark fields to compute - # after having inversed all dependencies. This is because we - # determine what currently depends on self, and it should not be - # recomputed before the modification! - if before: - tocompute = list(tocompute) - - # process what to compute - for field, records, create in tocompute: - records -= self.env.protected(field) - if not records: - continue - if field.compute and field.store: - if field.recursive: - recursively_marked = self.env.not_to_compute(field, records) - self.env.add_to_compute(field, records) - else: - # Don't force the recomputation of compute fields which are - # not stored as this is not really necessary. - if field.recursive: - recursively_marked = records & self.env.cache.get_records(records, field) - self.env.cache.invalidate([(field, records._ids)]) - # recursively trigger recomputation of field's dependents - if field.recursive: - recursively_marked.modified([field.name], create) + return self.sudo().with_context(active_test=False)._modified_triggers(tree, create) def _modified_triggers(self, tree, create=False): """ Return an iterator traversing a tree of field triggers on ``self``,