[FIX] core: avoid access error when flushing

This takes a former patch one step further.

Assume you recompute a field on two records, and the computation assigns
the first record but not the second one.  Before this patch, the
recomputation process was accessing the field on both records.  When
accessing the first record, both records are recomputed, and the value
of the first one is found in cache.  When accessing the second record,
no recomputation is made (it has been done already), and the cache is
empty. The field is thus fetched from database, and this crashes because
of insufficient access rights.

This scenario was causing some obscure nondeterministic crashes in
tests. The nondeterminism comes from the choice of the environment for
flushing (it should not be in superuser mode); the order of records in
the set to compute; the order in which records are assigned in the
compute method; the fact that the compute method should assign some
records and some not.

The fix consists in adding a method that processes pending computations,
without doing anything special for unassigned records.  The method is
used in field accessors (__get__ and mapped) and in method recompute().
The access error is gone because the recomputation no longer tries to
get the value of a field to compute.

closes odoo/odoo#64250

Signed-off-by: Raphael Collet (rco) <rco@openerp.com>

4 files changed, 79 insertions, 48 deletions

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 1f36dfe7bad..b02ef005015 100644
--- a/odoo/addons/test_new_api/models/test_new_api.py
+++ b/odoo/addons/test_new_api/models/test_new_api.py
@@ -663,19 +663,27 @@ class ComputeUnassigned(models.Model):
 
     @api.depends('foo')
     def _compute_bar(self):
-        pass
+        for record in self:
+            if record.foo == "assign":
+                record.bar = record.foo
 
     @api.depends('foo')
     def _compute_bare(self):
-        pass
+        for record in self:
+            if record.foo == "assign":
+                record.bare = record.foo
 
     @api.depends('foo')
     def _compute_bars(self):
-        pass
+        for record in self:
+            if record.foo == "assign":
+                record.bars = record.foo
 
     @api.depends('foo')
     def _compute_bares(self):
-        pass
+        for record in self:
+            if record.foo == "assign":
+                record.bares = record.foo
 
 
 class ModelBinary(models.Model):
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 2191bee7096..f77bcc261b0 100644
--- a/odoo/addons/test_new_api/tests/test_new_fields.py
+++ b/odoo/addons/test_new_api/tests/test_new_fields.py
@@ -703,27 +703,45 @@ class TestFields(TransactionCaseWithUserDemo):
         self.assertEqual(record.bares, False)
 
     def test_16_compute_unassigned_access_error(self):
-        # create a real record
-        record = self.env['test_new_api.compute.unassigned'].create({})
-        record.flush()
+        # create two records
+        records = self.env['test_new_api.compute.unassigned'].create([{}, {}])
+        records.flush()
 
-        # alter access rights: regular users cannot read 'record'
+        # alter access rights: regular users cannot read 'records'
         access = self.env.ref('test_new_api.access_test_new_api_compute_unassigned')
         access.perm_read = False
+        access.flush()
 
         # switch to environment with user demo
-        record = record.with_user(self.user_demo)
-        record.env.cache.invalidate()
+        records = records.with_user(self.user_demo)
+        records.env.cache.invalidate()
 
-        # check that the record is not accessible
+        # check that records are not accessible
         with self.assertRaises(AccessError):
-            record.bars
-
-        # modify the record and flush() changes with the current environment:
-        # this should not trigger an access error, even if unassigned computed
-        # fields are fetched from database
-        record.foo = "X"
-        record.flush()
+            records[0].bars
+        with self.assertRaises(AccessError):
+            records[1].bars
+
+        # Modify the records and flush() changes with the current environment:
+        # this should not trigger an access error, whatever the order in which
+        # records are considered.  It may fail in the following scenario:
+        #  - mark field 'bars' to compute on records
+        #  - access records[0].bars
+        #     - recompute bars on records (both) -> assign records[0] only
+        #     - return records[0].bars from cache
+        #  - access records[1].bars
+        #     - recompute nothing (done already)
+        #     - records[1].bars is not in cache
+        #     - fetch records[1].bars -> access error
+        records[0].foo = "assign"
+        records[1].foo = "x"
+        records.flush()
+
+        # try the other way around, too
+        records.env.cache.invalidate()
+        records[0].foo = "x"
+        records[1].foo = "assign"
+        records.flush()
 
     def test_20_float(self):
         """ test rounding of float fields """
diff --git a/odoo/fields.py b/odoo/fields.py
index 4514c62baab..710663866d0 100644
--- a/odoo/fields.py
+++ b/odoo/fields.py
@@ -929,20 +929,9 @@ class Field(MetaField('DummyField', (object,), {})):
         # only a single record may be accessed
         record.ensure_one()
 
-        recomputed = False
-        if self.compute and (record.id in env.all.tocompute.get(self, ())) \
-                and not env.is_protected(self, record):
-            # self must be computed on record
-            if self.recursive:
-                recs = record
-            else:
-                ids = expand_ids(record.id, env.all.tocompute[self])
-                recs = record.browse(itertools.islice(ids, PREFETCH_MAX))
-            try:
-                self.compute_value(recs)
-            except (AccessError, MissingError):
-                self.compute_value(record)
-            recomputed = True
+        if self.compute and self.store:
+            # process pending computations
+            self.recompute(record)
 
         try:
             value = env.cache.get(record, self)
@@ -970,8 +959,6 @@ class Field(MetaField('DummyField', (object,), {})):
             if self.store and record.id:
                 # real record: fetch from database
                 recs = record._in_cache_without(self)
-                if recomputed and self.compute_sudo:
-                    recs = recs.sudo()
                 try:
                     recs._fetch_field(self)
                 except AccessError:
@@ -1050,16 +1037,9 @@ class Field(MetaField('DummyField', (object,), {})):
             # not stored in cache
             return list(records._ids)
 
-        if self.compute:
-            # Force the computation of the subset of 'records' to compute. This
-            # is necessary because the values in cache are not valid for the
-            # records to compute. Note that this explicitly prevents an infinite
-            # loop upon recompute, which invokes mapped() on the records, which
-            # fetches record values, which calls flush, which calls recompute.
-            to_compute_ids = records.env.all.tocompute.get(self, ())
-            ids = [id_ for id_ in records._ids if id_ in to_compute_ids]
-            for record in records.browse(ids):
-                self.__get__(record, type(records))
+        if self.compute and self.store:
+            # process pending computations
+            self.recompute(records)
 
         # retrieve values in cache, and fetch missing ones
         vals = records.env.cache.get_until_miss(records, self)
@@ -1118,6 +1098,29 @@ class Field(MetaField('DummyField', (object,), {})):
     # Computation of field values
     #
 
+    def recompute(self, records):
+        """ Process the pending computations of ``self`` on ``records``. This
+        should be called only if ``self`` is computed and stored.
+        """
+        to_compute_ids = records.env.all.tocompute.get(self)
+        if not to_compute_ids:
+            return
+
+        if self.recursive:
+            for record in records:
+                if record.id in to_compute_ids:
+                    self.compute_value(record)
+            return
+
+        for record in records:
+            if record.id in to_compute_ids:
+                ids = expand_ids(record.id, to_compute_ids)
+                recs = record.browse(itertools.islice(ids, PREFETCH_MAX))
+                try:
+                    self.compute_value(recs)
+                except (AccessError, MissingError):
+                    self.compute_value(record)
+
     def compute_value(self, records):
         """ Invoke the compute method on ``records``; the results are in cache. """
         env = records.env
@@ -1131,14 +1134,16 @@ class Field(MetaField('DummyField', (object,), {})):
         # with _read(), which will flush() it. If the field is still to compute,
         # the latter flush() will recursively compute this field!
         for field in fields:
-            env.remove_to_compute(field, records)
+            if field.store:
+                env.remove_to_compute(field, records)
 
         try:
             with records.env.protecting(fields, records):
                 records._compute_field_value(self)
         except Exception:
             for field in fields:
-                env.add_to_compute(field, records)
+                if field.store:
+                    env.add_to_compute(field, records)
             raise
 
     def determine_inverse(self, records):
diff --git a/odoo/models.py b/odoo/models.py
index ae7b6f2f995..be36edcdc79 100644
--- a/odoo/models.py
+++ b/odoo/models.py
@@ -5869,10 +5869,10 @@ Fields:
                 # recomputed by accessing the field on the records
                 recs = recs.filtered('id')
                 try:
-                    recs.mapped(field.name)
+                    field.recompute(recs)
                 except MissingError:
                     existing = recs.exists()
-                    existing.mapped(field.name)
+                    field.recompute(existing)
                     # mark the field as computed on missing records, otherwise
                     # they remain forever in the todo list, and lead to an
                     # infinite loop...