From f916298bdf28e1da011789c36903eabcd43a9e63 Mon Sep 17 00:00:00 2001 From: Vincent Schippefilt Date: Tue, 25 Apr 2023 10:52:46 +0000 Subject: [PATCH] [FIX] base: do not fetch computed field already in cache Before this commit if a computed field is already in cache, but not its dependencies, `fetch` would fetch those dependencies. This commit ensures that fetch checks first if a computed is in cache before fetching its dependencies. This commit also follows dependencies of computed fields, if they depend on other computed fields. Finally, this commit consolidates `fetch` and `search_fetch`: they should use the same heuristics to know which fields to fetch. closes odoo/odoo#120001 X-original-commit: 6b680c463956f929db10d4c3058c36112a67e674 Signed-off-by: Vincent Schippefilt (vsc) Signed-off-by: Raphael Collet Co-authored-by: rco-odoo --- odoo/addons/base/tests/test_acl.py | 4 + odoo/addons/test_performance/models/models.py | 12 +++ .../tests/test_performance.py | 23 +++++ odoo/models.py | 89 ++++++++++--------- 4 files changed, 86 insertions(+), 42 deletions(-) diff --git a/odoo/addons/base/tests/test_acl.py b/odoo/addons/base/tests/test_acl.py index d67b0adf344..3926a395a6d 100644 --- a/odoo/addons/base/tests/test_acl.py +++ b/odoo/addons/base/tests/test_acl.py @@ -104,6 +104,10 @@ class TestACL(TransactionCaseWithUserDemo): # Now restrict access to the field and check it's forbidden self._set_field_groups(partner, 'bank_ids', GROUP_SYSTEM) + with self.assertRaises(AccessError): + partner.search_fetch([], ['bank_ids']) + with self.assertRaises(AccessError): + partner.fetch(['bank_ids']) with self.assertRaises(AccessError): partner.read(['bank_ids']) with self.assertRaises(AccessError): diff --git a/odoo/addons/test_performance/models/models.py b/odoo/addons/test_performance/models/models.py index b6ad7ec142f..a954e86872d 100644 --- a/odoo/addons/test_performance/models/models.py +++ b/odoo/addons/test_performance/models/models.py @@ -12,6 +12,8 @@ class BaseModel(models.Model): value = fields.Integer(default=0) value_pc = fields.Float(compute="_value_pc", store=True) value_ctx = fields.Float(compute="_value_ctx") + computed_value = fields.Float(compute="_computed_value") + indirect_computed_value = fields.Float(compute="_indirect_computed_value") partner_id = fields.Many2one('res.partner', string='Customer') line_ids = fields.One2many('test_performance.line', 'base_id') @@ -23,6 +25,16 @@ class BaseModel(models.Model): for record in self: record.value_pc = float(record.value) / 100 + @api.depends('value') + def _computed_value(self): + for record in self: + record.computed_value = float(record.value) / 100 + + @api.depends('computed_value') + def _indirect_computed_value(self): + for record in self: + record.indirect_computed_value = record.computed_value / 100 + @api.depends_context('key') def _value_ctx(self): self.env.cr.execute('SELECT 42') # one dummy query per batch diff --git a/odoo/addons/test_performance/tests/test_performance.py b/odoo/addons/test_performance/tests/test_performance.py index 45058e30470..b2bf41ac164 100644 --- a/odoo/addons/test_performance/tests/test_performance.py +++ b/odoo/addons/test_performance/tests/test_performance.py @@ -46,6 +46,7 @@ class TestPerformance(SavepointCaseWithUserDemo): 'value': 40, 'partner_id': partner12.id, }]) + cls.env.invalidate_all() @users('__system__', 'demo') @warmup @@ -145,6 +146,28 @@ class TestPerformance(SavepointCaseWithUserDemo): # 'display_name' depends on name that should already be fetched records.fetch(['id', 'display_name']) + with self.assertQueryCount(0): + # touch the field display_name, they are now in cache + records.mapped('display_name') + + # remove the dependencies of display_name (only name) + records.invalidate_recordset(['name']) + + # 'display_name' is in cache, so we should not load its dependencies + records.fetch(['display_name']) + + with self.assertQueryCount(0): + # 'indirect_computed_value' depends 'computed_value', which depends + # on 'value', and the latter is in cache + records.fetch(['indirect_computed_value']) + + with self.assertQueryCount(1): + records.invalidate_recordset(['value', 'computed_value', 'indirect_computed_value']) + + # 'indirect_computed_value' depends 'computed_value', which depends + # on 'value', and none of them are in cache + records.fetch(['indirect_computed_value']) + @warmup def test_search_fetch(self): """ Search and fetch all at once. """ diff --git a/odoo/models.py b/odoo/models.py index 67f43e04c9a..0e131ef1f7c 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -37,11 +37,12 @@ import pytz import re import uuid import warnings -from collections import defaultdict, OrderedDict +from collections import defaultdict, OrderedDict, deque from collections.abc import MutableMapping from contextlib import closing from inspect import getmembers, currentframe from operator import attrgetter, itemgetter +from typing import List import babel import babel.dates @@ -1591,22 +1592,7 @@ class BaseModel(metaclass=MetaModel): # optimization: don't execute the query at all return self.browse() - # determine fields to fetch - fields_to_fetch = OrderedSet() - if field_names: - field_names = self.check_field_access_rights('read', field_names) - for field_name in field_names: - field = self._fields.get(field_name) - if not field: - raise ValueError(f"Invalid field {field_name!r} on model {self._name!r}") - if field.store: - fields_to_fetch.add(field) - elif field.compute: - # optimization: fetch direct field dependencies - for dotname in self.pool.field_depends[field]: - dep = self._fields[dotname.split('.', 1)[0]] - if dep.prefetch is True and (not dep.groups or self.user_has_groups(dep.groups)): - fields_to_fetch.add(dep) + fields_to_fetch = self._determine_fields_to_fetch(field_names) return self._fetch_query(query, fields_to_fetch) @@ -3410,31 +3396,7 @@ class BaseModel(metaclass=MetaModel): if not self or not field_names: return - # determine fields to fetch - fields_to_verify = OrderedSet() - field_names = self.check_field_access_rights('read', field_names) - - for field_name in field_names: - if field_name == 'id': - continue - field = self._fields.get(field_name) - if not field: - raise ValueError(f"Invalid field {field_name!r} on model {self._name!r}") - if field.store: - fields_to_verify.add(field) - elif field.compute: - # optimization: fetch direct field dependencies - for dotname in self.pool.field_depends[field]: - dep = self._fields[dotname.split('.', 1)[0]] - if dep.prefetch is True and (not dep.groups or self.user_has_groups(dep.groups)): - fields_to_verify.add(dep) - - # verifies only once for each stored field or dependency if they are already in cache - cache = self.env.cache - fields_to_fetch = [ - field for field in fields_to_verify - if any(cache.get_missing_ids(self, field)) - ] + fields_to_fetch = self._determine_fields_to_fetch(field_names, ignore_when_in_cache=True) if not fields_to_fetch: # there is nothing to fetch, but we expect an error anyway in case @@ -3475,6 +3437,49 @@ class BaseModel(metaclass=MetaModel): if forbidden: raise self.env['ir.rule']._make_access_error('read', forbidden) + def _determine_fields_to_fetch(self, field_names, ignore_when_in_cache=False) -> List["Field"]: + """ + Return the fields to fetch from database among the given field names, + and following the dependencies of computed fields. The method is used + by :meth:`fetch` and :meth:`search_fetch`. + + :param field_names: the list of fields requested + :param ignore_when_in_cache: whether to ignore fields that are alreay in cache for ``self`` + :return: the list of fields that must be fetched + """ + if not field_names: + return [] + + cache = self.env.cache + + fields_to_fetch = [] + field_names_todo = deque(self.check_field_access_rights('read', field_names)) + field_names_done = {'id'} # trick: ignore 'id' + + while field_names_todo: + field_name = field_names_todo.popleft() + if field_name in field_names_done: + continue + field_names_done.add(field_name) + field = self._fields.get(field_name) + if not field: + raise ValueError(f"Invalid field {field_name!r} on model {self._name!r}") + if ignore_when_in_cache and not any(cache.get_missing_ids(self, field)): + # field is already in cache: don't fetch it + continue + if field.store: + fields_to_fetch.append(field) + else: + # optimization: fetch field dependencies + for dotname in self.pool.field_depends[field]: + dep_field = self._fields[dotname.split('.', 1)[0]] + if (not dep_field.store) or (dep_field.prefetch is True and ( + not dep_field.groups or self.user_has_groups(dep_field.groups) + )): + field_names_todo.append(dep_field.name) + + return fields_to_fetch + def _fetch_query(self, query, fields): """ Fetch the given fields (iterable of :class:`Field` instances) from the given query, put them in cache, and return the fetched records.