From 65a2c2ebc860fd7b2d733f66aef4d9ef7058b1cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Tue, 16 Jan 2024 17:04:31 +0100 Subject: [PATCH] [FIX] core: new record shouldn't force fetching inverse x2many fields In e0297bdac4eac165a79680de3b1139c2f554d5f5, the creation of a new record always patches the inverse fields of relational fields in order to make the cache of those inverse fields consistent. For instance, when creating a new record like user = model.new({'group_ids': [Command.link(group.id)]}) The inverse of field 'group_ids' on the new record having 'group' as origin is patched so that its value includes record. A side effect of this mechanism is that it fetches group.user_ids in order to patch the value of new_group.user_ids, where 'new_group' is the new record having 'group' as origin. The side effect described above is problematic when that inverse field has huge cardinality, like hundreds of thousands of records, and this performance overhead is unacceptable when the inverse field is actually not used at all. We address this performance issue by patching the value of x2many fields only when they are used. If the value of the field is not in cache yet, the patch is applied once a value is put in cache. If the field is not used, the patch is simply never applied. Part-of: odoo/odoo#149624 --- .../test_new_api/tests/test_new_fields.py | 29 ++++++++++++++++ odoo/api.py | 33 +++++++++++++++++++ odoo/fields.py | 9 ++--- 3 files changed, 65 insertions(+), 6 deletions(-) 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 7c54642560b..da7a2f2534a 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -2195,6 +2195,35 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(new_move.line_ids._origin, line) self.assertEqual(new_move.line_ids.move_id, new_move) + def test_41_new_many2many(self): + group = self.env['test_new_api.group'].create({}) + user0 = self.env['test_new_api.user'].create({'group_ids': [Command.link(group.id)]}) + new_user0 = user0.new(origin=user0) + new_group = group.new(origin=group) + + self.env.invalidate_all() + + # creating new_user1 shoud not fetch new_group.user_ids, which is the + # inverse of field new_user1.group_ids + with self.assertQueryCount(0): + new_user1 = self.env['test_new_api.user'].new({'group_ids': [Command.link(group.id)]}) + self.assertEqual(new_user1.group_ids, new_group) + + # accessing new_group.user_ids should fetch group.user_ids and patch + # new_group.user_ids + with self.assertQueryCount(1): + self.assertEqual(new_group.user_ids, new_user0 + new_user1) + + # creating new_user2 should patch new_group.user_ids immediately, since + # it is in cache + with self.assertQueryCount(0): + new_user2 = self.env['test_new_api.user'].new({'group_ids': [Command.link(group.id)]}) + self.assertEqual(new_user2.group_ids, new_group) + self.assertEqual(new_group.user_ids, new_user0 + new_user1 + new_user2) + + # the patches on new_group.user_ids should not have changed group.user_ids + self.assertEqual(group.user_ids, user0) + @mute_logger('odoo.addons.base.models.ir_model') def test_41_new_related(self): """ test the behavior of related fields starting on new records. """ diff --git a/odoo/api.py b/odoo/api.py index 0e069c56d81..de7e073a21b 100644 --- a/odoo/api.py +++ b/odoo/api.py @@ -910,6 +910,10 @@ class Cache(object): # in `_data` self._dirty = defaultdict(OrderedSet) + # {field: {record_id: ids}} record ids to be added to the values of + # x2many fields if they are not in cache yet + self._patches = defaultdict(lambda: defaultdict(list)) + def __repr__(self): # for debugging: show the cache content and dirty flags as stars data = {} @@ -1099,6 +1103,34 @@ class Cache(object): for id_, val in zip(records._ids, values): field_cache.setdefault(id_, val) + def patch(self, records, field, new_id): + """ Apply a patch to an x2many field on new records. The patch consists + in adding new_id to its value in cache. If the value is not in cache + yet, it will be applied once the value is put in cache with method + :meth:`patch_and_set`. + """ + assert not new_id, "Cache.patch can only be called with a new id" + field_cache = self._set_field_cache(records, field) + for id_ in records._ids: + assert not id_, "Cache.patch can only be called with new records" + if id_ in field_cache: + field_cache[id_] = tuple(dict.fromkeys(field_cache[id_] + (new_id,))) + else: + self._patches[field][id_].append(new_id) + + def patch_and_set(self, record, field, value): + """ Set the value of ``field`` for ``record``, like :meth:`set`, but + apply pending patches to ``value`` and return the value actually put + in cache. + """ + field_patches = self._patches.get(field) + if field_patches: + ids = field_patches.pop(record.id, ()) + if ids: + value = tuple(dict.fromkeys(value + tuple(ids))) + self.set(record, field, value) + return value + def remove(self, record, field): """ Remove the value of ``field`` for ``record``. """ assert record.id not in self._dirty.get(field, ()) @@ -1257,6 +1289,7 @@ class Cache(object): """ Invalidate the cache and its dirty flags. """ self._data.clear() self._dirty.clear() + self._patches.clear() def check(self, env): """ Check the consistency of the cache for the given environment. """ diff --git a/odoo/fields.py b/odoo/fields.py index 601cbaa7009..abab0f4c85b 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -1194,7 +1194,7 @@ class Field(MetaField('DummyField', (object,), {})): elif self.store and record._origin and not (self.compute and self.readonly): # new record with origin: fetch from origin value = self.convert_to_cache(record._origin[self.name], record, validate=False) - env.cache.set(record, self, value) + value = env.cache.patch_and_set(record, self, value) elif self.compute: #pylint: disable=using-constant-test # non-stored field or new record without origin: compute @@ -1240,7 +1240,7 @@ class Field(MetaField('DummyField', (object,), {})): else: # non-stored field or stored field on new record: default value value = self.convert_to_cache(False, record, validate=False) - env.cache.set(record, self, value) + value = env.cache.patch_and_set(record, self, value) defaults = record.default_get([self.name]) if self.name in defaults: # The null value above is necessary to convert x2many field @@ -4167,10 +4167,7 @@ class _RelationalMulti(_Relational): def _update(self, records, value): """ Update the cached value of ``self`` for ``records`` with ``value``. """ - cache = records.env.cache - for record in records.with_context(active_test=False): - val = self.convert_to_cache(record[self.name] | value, record, validate=False) - cache.set(record, self, val) + records.env.cache.patch(records, self, value.id) records.modified([self.name]) def convert_to_cache(self, value, record, validate=True):