From ed762a3cef62eed1996056b18fe750604f59aaf2 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Tue, 9 Aug 2022 13:38:26 +0000 Subject: [PATCH] [FIX] core: field recomputed on more records than expected The issue occurs when a computed field depends on a many2many field with a corresponding inverse field on its comodel. Consider two models like class User(models.Model): _name = _description = 'test_new_api.user' group_ids = fields.Many2many('test_new_api.group') group_count = fields.Integer(compute='_compute_group_count', store=True) @api.depends('group_ids') def _compute_group_count(self): for user in self: user.group_count = len(user.group_ids) class Group(models.Model): _name = _description = 'test_new_api.group' user_ids = fields.Many2many('test_new_api.user') When a user is added to a group with group.write({'user_ids': [Command.link(user.id)]}) we expect the field `group_count` to be recomputed on `user` only, but it is actually triggered on *all* the records in `group.user_ids`. This is a real performance issue when there are many records in the relation. The explanation comes from the fact that - the framework considers the field `user_ids` is modified on `group`; - the field `group_count` implicitly depends on `group_ids.user_ids`, which makes it triggered on the users `u` such that `u.group_ids` intersects `group`. The solution consists in handling the dependencies on inverse many2many field in the field itself. The field no longer adds the implicit dependency on its inverse field in the trigger tree, but instead determines which records in the comodel are actually impacted by the relation change in the method field.write(). closes odoo/odoo#111943 X-original-commit: bb3a6e378b5f6b2e14b74ce4efb6d749ad54cb1e Signed-off-by: Raphael Collet --- .../test_new_api/models/test_new_api.py | 18 +++++++++ .../test_new_api/security/ir.model.access.csv | 2 + .../test_new_api/tests/test_new_fields.py | 38 +++++++++++++++++++ odoo/fields.py | 32 +++++++++++++++- 4 files changed, 89 insertions(+), 1 deletion(-) 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 b274f43fe93..20713b64c9a 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -1379,6 +1379,24 @@ class ComputeMember(models.Model): member.container_id = container.search([('name', '=', member.name)], limit=1) +class User(models.Model): + _name = _description = 'test_new_api.user' + + group_ids = fields.Many2many('test_new_api.group') + group_count = fields.Integer(compute='_compute_group_count', store=True) + + @api.depends('group_ids') + def _compute_group_count(self): + for user in self: + user.group_count = len(user.group_ids) + + +class Group(models.Model): + _name = _description = 'test_new_api.group' + + user_ids = fields.Many2many('test_new_api.user') + + class ComputeEditable(models.Model): _name = _description = 'test_new_api.compute_editable' 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 dc2b7b3adbe..6a96afcac72 100644 --- a/odoo/addons/test_new_api/security/ir.model.access.csv +++ b/odoo/addons/test_new_api/security/ir.model.access.csv @@ -76,6 +76,8 @@ access_test_new_api_model_shared_cache_compute_parent,access_test_new_api.model_ access_test_new_api_model_shared_cache_compute_line,access_test_new_api.model_shared_cache_compute_line,model_test_new_api_model_shared_cache_compute_line,,1,1,1,1 access_test_new_api_compute_container,access_test_new_api_compute_container,model_test_new_api_compute_container,,1,1,1,1 access_test_new_api_compute_member,access_test_new_api_compute_member,model_test_new_api_compute_member,,1,1,1,1 +access_test_new_api_user,access_test_new_api_user,model_test_new_api_user,,1,1,1,1 +access_test_new_api_group,access_test_new_api_group,model_test_new_api_group,,1,1,1,1 access_test_new_api_compute_editable,access_test_new_api_compute_editable,model_test_new_api_compute_editable,,1,1,1,1 access_test_new_api_compute_editable_line,access_test_new_api_compute_editable_line,model_test_new_api_compute_editable_line,,1,1,1,1 access_test_new_api_model_constrained_unlinks,access_test_new_api_model_constrained_unlinks,model_test_new_api_model_constrained_unlinks,,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 94a88355312..a7d2bff6677 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -820,6 +820,44 @@ class TestFields(TransactionCaseWithUserDemo): records[1].foo = "assign" self.env.flush_all() + def test_17_compute_depends_on_many2many(self): + user1, user2, user3 = self.env['test_new_api.user'].create([{}, {}, {}]) + group = self.env['test_new_api.group'].create({'user_ids': [Command.link(user1.id)]}) + self.env.flush_all() + + field = type(user1).group_count + self.assertFalse(self.env.records_to_compute(field)) + + # should mark user2 and user3 to compute only + group.write({'user_ids': [Command.link(user1.id), Command.link(user2.id), Command.link(user3.id)]}) + self.assertEqual(self.env.records_to_compute(field), user2 + user3) + + # should mark user2 to compute only + self.env.flush_all() + group.write({'user_ids': [Command.unlink(user2.id)]}) + self.assertEqual(self.env.records_to_compute(field), user2) + + # should mark user2 and user3 to compute only + self.env.flush_all() + group.write({'user_ids': [Command.set([user1.id, user2.id])]}) + self.assertEqual(self.env.records_to_compute(field), user2 + user3) + + # should mark user3 to compute only + self.env.flush_all() + user3.write({'group_ids': [Command.link(group.id)]}) + self.assertEqual(self.env.records_to_compute(field), user3) + + # similar with new records, but only check recomputation + user1 = self.env['test_new_api.user'].new({}) + user2 = self.env['test_new_api.user'].new({}) + group = self.env['test_new_api.group'].new({'user_ids': [user1.id]}) + self.assertEqual(user1.group_count, 1) + self.assertEqual(user2.group_count, 0) + + group.user_ids += user2 + self.assertEqual(user1.group_count, 1) + self.assertEqual(user2.group_count, 1) + def test_20_float(self): """ test rounding of float fields """ record = self.env['test_new_api.mixed'].create({}) diff --git a/odoo/fields.py b/odoo/fields.py index e83b88ea3e4..1ab3cd112f3 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -804,7 +804,7 @@ class Field(MetaField('DummyField', (object,), {})): if not (field is self and not index): yield tuple(field_seq) - if field.type in ('one2many', 'many2many'): + if field.type == 'one2many': for inv_field in Model.pool.field_inverses[field]: yield tuple(field_seq) + (inv_field,) @@ -4823,6 +4823,9 @@ class Many2many(_RelationalMulti): for record in records: cache.set(record, self, tuple(new_relation[record.id])) + # determine the corecords for which the relation has changed + modified_corecord_ids = set() + # process pairs to add (beware of duplicates) pairs = [(x, y) for x, ys in new_relation.items() for y in ys - old_relation[x]] if pairs: @@ -4838,6 +4841,7 @@ class Many2many(_RelationalMulti): y_to_xs = defaultdict(set) for x, y in pairs: y_to_xs[y].add(x) + modified_corecord_ids.add(y) for invf in records.pool.field_inverses[self]: domain = invf.get_domain_list(comodel) valid_ids = set(records.filtered_domain(domain)._ids) @@ -4858,6 +4862,7 @@ class Many2many(_RelationalMulti): y_to_xs = defaultdict(set) for x, y in pairs: y_to_xs[y].add(x) + modified_corecord_ids.add(y) if self.store: # express pairs as the union of cartesian products: @@ -4890,6 +4895,16 @@ class Many2many(_RelationalMulti): except KeyError: pass + if modified_corecord_ids: + # trigger the recomputation of fields that depend on the inverse + # fields of self on the modified corecords + corecords = comodel.browse(modified_corecord_ids) + corecords.modified([ + invf.name + for invf in model.pool.field_inverses[self] + if invf.model_name == self.comodel_name + ]) + return records.filtered( lambda record: new_relation[record.id] != old_relation[record.id] ) @@ -4949,6 +4964,9 @@ class Many2many(_RelationalMulti): for record in records: cache.set(record, self, tuple(new_relation[record.id])) + # determine the corecords for which the relation has changed + modified_corecord_ids = set() + # process pairs to add (beware of duplicates) pairs = [(x, y) for x, ys in new_relation.items() for y in ys - old_relation[x]] if pairs: @@ -4956,6 +4974,7 @@ class Many2many(_RelationalMulti): y_to_xs = defaultdict(set) for x, y in pairs: y_to_xs[y].add(x) + modified_corecord_ids.add(y) for invf in records.pool.field_inverses[self]: domain = invf.get_domain_list(comodel) valid_ids = set(records.filtered_domain(domain)._ids) @@ -4977,6 +4996,7 @@ class Many2many(_RelationalMulti): y_to_xs = defaultdict(set) for x, y in pairs: y_to_xs[y].add(x) + modified_corecord_ids.add(y) for invf in records.pool.field_inverses[self]: for y, xs in y_to_xs.items(): corecord = comodel.browse([y]) @@ -4987,6 +5007,16 @@ class Many2many(_RelationalMulti): except KeyError: pass + if modified_corecord_ids: + # trigger the recomputation of fields that depend on the inverse + # fields of self on the modified corecords + corecords = comodel.browse(modified_corecord_ids) + corecords.modified([ + invf.name + for invf in model.pool.field_inverses[self] + if invf.model_name == self.comodel_name + ]) + return records.filtered( lambda record: new_relation[record.id] != old_relation[record.id] )