[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) <vsc@odoo.com>
Signed-off-by: Raphael Collet <rco@odoo.com>
Co-authored-by: rco-odoo <rco@odoo.com>

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):

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

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. """

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.