From 1de5dc4b960e2efcff209c0cdd4994a87c99b76f Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 18 Nov 2021 17:06:22 +0000 Subject: [PATCH] [FIX] base_automation: recursive computed field cause too many calls to flush The use-case that motivated this fix is the deletion of a project task with many subtasks. The field 'project_id' on tasks is recursively computed, and some automated action must be executed when its value corresponds to a given project. The issue occurs when the domain of automated actions is evaluated by method search(), because the latter flushes the fields to search on, which are also the ones being recomputed. Combined with the fact that recursive fields are not computed in batch, this leads to a huge amount of recursive calls between the automated action and flush(). The execution of task.unlink() looks like this: - mark 'project_id' to compute on subtasks - delete task - flush() - recompute 'project_id' on subtask1 - call compute on subtask1 - in action, search([('id', 'in', subtask1.ids), ('project_id', '=', pid)]) - flush(['id', 'project_id']) - recompute 'project_id' on subtask2 - call compute on subtask2 - in action, search([('id', 'in', subtask2.ids), ('project_id', '=', pid)]) - flush(['id', 'project_id']) - recompute 'project_id' on subtask3 - call compute on subtask3 - in action, search([('id', 'in', subtask3.ids), ('project_id', '=', pid)]) - flush(['id', 'project_id']) - recompute 'project_id' on subtask4 ... closes odoo/odoo#80141 X-original-commit: e2788b580ef15ef3083ac919737a46d850143832 Signed-off-by: Raphael Collet (rco) --- .../base_automation/models/base_automation.py | 4 +- .../models/test_base_automation.py | 24 ++++++++ .../security/ir.model.access.csv | 2 + .../test_base_automation/tests/test_flow.py | 59 ++++++++++++++++++- 4 files changed, 86 insertions(+), 3 deletions(-) diff --git a/addons/base_automation/models/base_automation.py b/addons/base_automation/models/base_automation.py index 9c596578261..fc184f1c68c 100644 --- a/addons/base_automation/models/base_automation.py +++ b/addons/base_automation/models/base_automation.py @@ -218,8 +218,8 @@ class BaseAutomation(models.Model): """ Filter the records that satisfy the postcondition of action ``self``. """ self_sudo = self.sudo() if self_sudo.filter_domain and records: - domain = [('id', 'in', records.ids)] + safe_eval.safe_eval(self_sudo.filter_domain, self._get_eval_context()) - return records.sudo().search(domain).with_env(records.env), domain + domain = safe_eval.safe_eval(self_sudo.filter_domain, self._get_eval_context()) + return records.sudo().filtered_domain(domain).with_env(records.env), domain else: return records, None diff --git a/addons/test_base_automation/models/test_base_automation.py b/addons/test_base_automation/models/test_base_automation.py index 557cfa8e60d..ba25c86767b 100644 --- a/addons/test_base_automation/models/test_base_automation.py +++ b/addons/test_base_automation/models/test_base_automation.py @@ -65,3 +65,27 @@ class ModelWithoutAccess(models.Model): name = fields.Char() another_field = fields.Char() + + +class Project(models.Model): + _name = _description = 'test_base_automation.project' + + name = fields.Char() + task_ids = fields.One2many('test_base_automation.task', 'project_id') + + +class Task(models.Model): + _name = _description = 'test_base_automation.task' + + name = fields.Char() + parent_id = fields.Many2one('test_base_automation.task') + project_id = fields.Many2one( + 'test_base_automation.project', + compute='_compute_project_id', recursive=True, store=True, readonly=False, + ) + + @api.depends('parent_id.project_id') + def _compute_project_id(self): + for task in self: + if not task.project_id: + task.project_id = task.parent_id.project_id diff --git a/addons/test_base_automation/security/ir.model.access.csv b/addons/test_base_automation/security/ir.model.access.csv index 16be23cd17e..24605b68c54 100644 --- a/addons/test_base_automation/security/ir.model.access.csv +++ b/addons/test_base_automation/security/ir.model.access.csv @@ -3,3 +3,5 @@ access_base_automation_lead_test,access_base_automation_lead_test,model_base_aut access_base_automation_line_test,access_base_automation_line_test,model_base_automation_line_test,base.group_system,1,1,1,1 access_base_automation_link_test,access_base_automation_link_test,model_base_automation_link_test,,1,1,1,1 access_base_automation_linked_test,access_base_automation_linked_test,model_base_automation_linked_test,,1,1,1,1 +access_test_base_automation_project,access_test_base_automation_project,model_test_base_automation_project,,1,1,1,1 +access_test_base_automation_task,access_test_base_automation_task,model_test_base_automation_task,,1,1,1,1 diff --git a/addons/test_base_automation/tests/test_flow.py b/addons/test_base_automation/tests/test_flow.py index 107b167a838..49ebbdd662a 100644 --- a/addons/test_base_automation/tests/test_flow.py +++ b/addons/test_base_automation/tests/test_flow.py @@ -2,6 +2,7 @@ # # Part of Odoo. See LICENSE file for full copyright and licensing details. from unittest.mock import patch +import sys from odoo.addons.base.tests.common import TransactionCaseWithUserDemo from odoo.tests import common, tagged @@ -355,7 +356,8 @@ record['name'] = record.name + 'X'""", rec4 = Model.with_user(self.user_demo).create({'name': 'again another record'}) rec4.write({'name': 'another value'}) -@common.tagged('post_install','-at_install') + +@common.tagged('post_install', '-at_install') class TestCompute(common.TransactionCase): def test_inversion(self): """ If a stored field B depends on A, an update to the trigger for A @@ -404,3 +406,58 @@ class TestCompute(common.TransactionCase): }) r.parent_id = company2 self.assertEqual(r.display_name, 'Awiclo, Bob') + + def test_recursion(self): + project = self.env['test_base_automation.project'].create({}) + + # this action is executed every time a task is assigned to project + self.env['base.automation'].create({ + 'name': 'dummy', + 'model_id': self.env['ir.model']._get_id('test_base_automation.task'), + 'state': 'code', + 'trigger': 'on_create_or_write', + 'filter_domain': repr([('project_id', '=', project.id)]), + }) + + # create one task in project with 10 subtasks; all the subtasks are + # automatically assigned to project, too + task = self.env['test_base_automation.task'].create({'project_id': project.id}) + subtasks = task.create([{'parent_id': task.id} for _ in range(10)]) + subtasks.flush() + + # This test checks what happens when a stored recursive computed field + # is marked to compute on many records, and automated actions are + # triggered depending on that field. In this case, we trigger the + # recomputation of 'project_id' on 'subtasks' by deleting their parent + # task. + # + # An issue occurs when the domain of automated actions is evaluated by + # method search(), because the latter flushes the fields to search on, + # which are also the ones being recomputed. Combined with the fact + # that recursive fields are not computed in batch, this leads to a huge + # amount of recursive calls between the automated action and flush(). + # + # The execution of task.unlink() looks like this: + # - mark 'project_id' to compute on subtasks + # - delete task + # - flush() + # - recompute 'project_id' on subtask1 + # - call compute on subtask1 + # - in action, search([('id', 'in', subtask1.ids), ('project_id', '=', pid)]) + # - flush(['id', 'project_id']) + # - recompute 'project_id' on subtask2 + # - call compute on subtask2 + # - in action search([('id', 'in', subtask2.ids), ('project_id', '=', pid)]) + # - flush(['id', 'project_id']) + # - recompute 'project_id' on subtask3 + # - call compute on subtask3 + # - in action, search([('id', 'in', subtask3.ids), ('project_id', '=', pid)]) + # - flush(['id', 'project_id']) + # - recompute 'project_id' on subtask4 + # ... + limit = sys.getrecursionlimit() + try: + sys.setrecursionlimit(100) + task.unlink() + finally: + sys.setrecursionlimit(limit)