[FIX] core: marking and traversing computed fields before modification

This fixes an issue that occurs when marking fields to compute in the
following situation:
 - it occurs before the actual modification,
 - a relational field is marked to compute,
 - the same field is traversed to inverse some dependency.

When the marking occurs after the modification, the traversal of the
field should actually recompute the field.  However, if the marking
occurs before the modification, the inversion of dependencies must be
based on the current value of the field.  This is the case with method
unlink(): we call method modified() to mark the fields that currently
depend on the records to be deleted, and the fields must be computed
only after the deletion!

X-original-commit: cd19c2d93db8911fe42802640ea32e5d87aadeec

addons/sale_coupon/models/sale_order.py

@@ -471,8 +471,8 @@ class SaleOrderLine(models.Model):
     # Another possibility is to add on product.product a one2many to sale.order.line 'order_line_ids',
     # and then add the depends @api.depends('discount_line_product_id.order_line_ids'),
     # but I am not sure this will as efficient as the below.
-    def modified(self, fnames, create=False):
-        super(SaleOrderLine, self).modified(fnames, create)
+    def modified(self, fnames, *args, **kwargs):
+        super(SaleOrderLine, self).modified(fnames, *args, **kwargs)
         if 'product_id' in fnames:
             Program = self.env['coupon.program'].sudo()
             field_order_count = Program._fields['order_count']

odoo/addons/test_new_api/models/test_new_api.py

@@ -493,8 +493,17 @@ class ComputeRecursive(models.Model):
 
     name = fields.Char(required=True)
     parent = fields.Many2one('test_new_api.recursive', ondelete='cascade')
+    full_name = fields.Char(compute='_compute_full_name')
     display_name = fields.Char(compute='_compute_display_name', store=True)
 
+    @api.depends('name', 'parent.full_name')
+    def _compute_full_name(self):
+        for rec in self:
+            if rec.parent:
+                rec.full_name = rec.parent.full_name + " / " + rec.name
+            else:
+                rec.full_name = rec.name
+
     @api.depends('name', 'parent.display_name')
     def _compute_display_name(self):
         for rec in self:
@@ -792,7 +801,7 @@ class Attachment(models.Model):
             rec.name = self.env[rec.res_model].browse(rec.res_id).display_name
 
     # DLE P55: `test_cache_invalidation`
-    def modified(self, fnames, create=False):
+    def modified(self, fnames, *args, **kwargs):
         if not self:
             return
         comodel = self.env[self.res_model]
@@ -801,7 +810,7 @@ class Attachment(models.Model):
             record = comodel.browse(self.res_id)
             self.env.cache.invalidate([(field, record._ids)])
             record.modified(['attachment_ids'])
-        return super(Attachment, self).modified(fnames, create)
+        return super(Attachment, self).modified(fnames, *args, **kwargs)
 
 
 class AttachmentHost(models.Model):
@@ -1209,3 +1218,32 @@ class ConstrainedUnlinks(models.Model):
         for rec in self:
             if rec.foo and rec.foo == 'prosciutto':
                 raise ValueError("You didn't say if you wanted it crudo or cotto...")
+
+
+class TriggerLeft(models.Model):
+    _name = 'test_new_api.trigger.left'
+    _description = 'model with a related many2one'
+
+    middle_ids = fields.One2many('test_new_api.trigger.middle', 'left_id')
+    right_id = fields.Many2one(related='middle_ids.right_id', store=True)
+
+
+class TriggerMiddle(models.Model):
+    _name = 'test_new_api.trigger.middle'
+    _description = 'model linking test_new_api.trigger.left and test_new_api.trigger.right'
+
+    left_id = fields.Many2one('test_new_api.trigger.left', required=True)
+    right_id = fields.Many2one('test_new_api.trigger.right', required=True)
+
+
+class TriggerRight(models.Model):
+    _name = 'test_new_api.trigger.right'
+    _description = 'model with a dependency on the inverse of the related many2one'
+
+    left_ids = fields.One2many('test_new_api.trigger.left', 'right_id')
+    left_size = fields.Integer(compute='_compute_left_size', store=True)
+
+    @api.depends('left_ids')
+    def _compute_left_size(self):
+        for record in self:
+            record.left_size = len(record.left_ids)

odoo/addons/test_new_api/security/ir.model.access.csv

@@ -69,3 +69,6 @@ access_test_new_api_compute_member,access_test_new_api_compute_member,model_test
 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
+access_test_new_api_trigger_left,access_test_new_api_trigger_left,model_test_new_api_trigger_left,,1,1,1,1
+access_test_new_api_trigger_middle,access_test_new_api_trigger_middle,model_test_new_api_trigger_middle,,1,1,1,1
+access_test_new_api_trigger_right,access_test_new_api_trigger_right,model_test_new_api_trigger_right,,1,1,1,1

odoo/addons/test_new_api/tests/test_new_fields.py

@@ -450,20 +450,42 @@ class TestFields(TransactionCaseWithUserDemo):
         b = self.env['test_new_api.recursive'].create({'name': 'B', 'parent': a.id})
         c = self.env['test_new_api.recursive'].create({'name': 'C', 'parent': b.id})
         d = self.env['test_new_api.recursive'].create({'name': 'D', 'parent': c.id})
+        self.assertEqual(a.full_name, 'A')
+        self.assertEqual(b.full_name, 'A / B')
+        self.assertEqual(c.full_name, 'A / B / C')
+        self.assertEqual(d.full_name, 'A / B / C / D')
         self.assertEqual(a.display_name, 'A')
         self.assertEqual(b.display_name, 'A / B')
         self.assertEqual(c.display_name, 'A / B / C')
         self.assertEqual(d.display_name, 'A / B / C / D')
 
+        a.name = 'A1'
+        self.assertEqual(a.full_name, 'A1')
+        self.assertEqual(b.full_name, 'A1 / B')
+        self.assertEqual(c.full_name, 'A1 / B / C')
+        self.assertEqual(d.full_name, 'A1 / B / C / D')
+        self.assertEqual(a.display_name, 'A1')
+        self.assertEqual(b.display_name, 'A1 / B')
+        self.assertEqual(c.display_name, 'A1 / B / C')
+        self.assertEqual(d.display_name, 'A1 / B / C / D')
+
         b.parent = False
-        self.assertEqual(a.display_name, 'A')
+        self.assertEqual(a.full_name, 'A1')
+        self.assertEqual(b.full_name, 'B')
+        self.assertEqual(c.full_name, 'B / C')
+        self.assertEqual(d.full_name, 'B / C / D')
+        self.assertEqual(a.display_name, 'A1')
         self.assertEqual(b.display_name, 'B')
         self.assertEqual(c.display_name, 'B / C')
         self.assertEqual(d.display_name, 'B / C / D')
 
         # rename several records to trigger several recomputations at once
         (d + c + b).write({'name': 'X'})
-        self.assertEqual(a.display_name, 'A')
+        self.assertEqual(a.full_name, 'A1')
+        self.assertEqual(b.full_name, 'X')
+        self.assertEqual(c.full_name, 'X / X')
+        self.assertEqual(d.full_name, 'X / X / X')
+        self.assertEqual(a.display_name, 'A1')
         self.assertEqual(b.display_name, 'X')
         self.assertEqual(c.display_name, 'X / X')
         self.assertEqual(d.display_name, 'X / X / X')
@@ -1131,6 +1153,32 @@ class TestFields(TransactionCaseWithUserDemo):
         self.assertEqual(bar.foo, oof)
         self.assertIn(bar, bar.search([('foo', 'in', oof.ids)]))
 
+    def test_25_one2many_inverse_related(self):
+        left = self.env['test_new_api.trigger.left'].create({})
+        right = self.env['test_new_api.trigger.right'].create({})
+        self.assertFalse(left.right_id)
+        self.assertFalse(right.left_ids)
+        self.assertFalse(right.left_size)
+
+        # create middle: this should trigger left.right_id by traversing
+        # middle.left_id, and right.left_size by traversing left.right_id
+        # after its computation!
+        middle = self.env['test_new_api.trigger.middle'].create({
+            'left_id': left.id,
+            'right_id': right.id,
+        })
+        self.assertEqual(left.right_id, right)
+        self.assertEqual(right.left_ids, left)
+        self.assertEqual(right.left_size, 1)
+
+        # delete middle: this should trigger left.right_id by traversing
+        # middle.left_id, and right.left_size by traversing left.right_id
+        # before its computation!
+        middle.unlink()
+        self.assertFalse(left.right_id)
+        self.assertFalse(right.left_ids)
+        self.assertFalse(right.left_size)
+
     def test_26_inherited(self):
         """ test inherited fields. """
         # a bunch of fields are inherited from res_partner

odoo/fields.py

@@ -1105,10 +1105,10 @@ class Field(MetaField('DummyField', (object,), {})):
             # new records: no business logic
             new_records = records.browse(new_ids)
             with records.env.protecting(records.pool.field_computed.get(self, [self]), records):
-                new_records.modified([self.name])
-                self.write(new_records, value)
                 if self.relational:
-                    new_records.modified([self.name])
+                    new_records.modified([self.name], before=True)
+                self.write(new_records, value)
+                new_records.modified([self.name])
 
             if self.inherited:
                 # special case: also assign parent records if they are new

odoo/models.py

@@ -3437,7 +3437,7 @@ Fields:
         # mark fields that depend on 'self' to recompute them after 'self' has
         # been deleted (like updating a sum of lines after deleting one line)
         self.flush()
-        self.modified(self._fields)
+        self.modified(self._fields, before=True)
 
         with self.env.norecompute():
             self.check_access_rule('unlink')
@@ -3618,8 +3618,25 @@ Fields:
 
         # protect fields being written against recomputation
         with env.protecting(protected, self):
-            # determine records depending on values
-            self.modified(relational_names)
+            # Determine records depending on values. When modifying a relational
+            # field, you have to recompute what depends on the field's values
+            # before and after modification.  This is because the modification
+            # has an impact on the "data path" between a computed field and its
+            # dependency.  Note that this double call to modified() is only
+            # necessary for relational fields.
+            #
+            # It is best explained with a simple example: consider two sales
+            # orders SO1 and SO2.  The computed total amount on sales orders
+            # indirectly depends on the many2one field 'order_id' linking lines
+            # to their sales order.  Now consider the following code:
+            #
+            #   line = so1.line_ids[0]      # pick a line from SO1
+            #   line.order_id = so2         # move the line to SO2
+            #
+            # In this situation, the total amount must be recomputed on *both*
+            # sales order: the line's order before the modification, and the
+            # line's order after the modification.
+            self.modified(relational_names, before=True)
 
             real_recs = self.filtered('id')
 
@@ -5707,14 +5724,14 @@ Fields:
                [(invf, None) for f in fields for invf in self._field_inverses[f]]
         self.env.cache.invalidate(spec)
 
-    def modified(self, fnames, create=False):
-        """ Notify that fields have been modified on ``self``. This invalidates
-            the cache, and prepares the recomputation of stored function fields
-            (new-style fields only).
+    def modified(self, fnames, create=False, before=False):
+        """ Notify that fields will be or have been modified on ``self``. This
+        invalidates the cache where necessary, and prepares the recomputation of
+        dependent stored fields.
 
-            :param fnames: iterable of field names that have been modified on
-                records ``self``
-            :param create: whether modified is called in the context of record creation
+        :param fnames: iterable of field names modified on records ``self``
+        :param create: whether called in the context of record creation
+        :param before: whether called before modifying records ``self``
         """
         if not self or not fnames:
             return
@@ -5747,34 +5764,59 @@ Fields:
                 node = self.pool.field_triggers.get(self._fields[fname])
                 if node:
                     trigger_tree_merge(tree, node)
+
         if tree:
-            self.sudo().with_context(active_test=False)._modified_triggers(tree, create)
+            # determine what to compute (through an iterator)
+            tocompute = self.sudo().with_context(active_test=False)._modified_triggers(tree, create)
+
+            # When called after modification, one should traverse backwards
+            # dependencies by taking into account all fields already known to be
+            # recomputed.  In that case, we mark fieds to compute as soon as
+            # possible.
+            #
+            # When called before modification, one should mark fields to compute
+            # after having inversed all dependencies.  This is because we
+            # determine what currently depends on self, and it should not be
+            # recomputed before the modification!
+            if before:
+                tocompute = list(tocompute)
+
+            # process what to compute
+            for field, records in tocompute:
+                records -= self.env.protected(field)
+                if not records:
+                    continue
+                # Dont force the recomputation of compute fields which are
+                # not stored as this is not really necessary.
+                recursive = not create and field.recursive
+                if field.compute and field.store:
+                    if recursive:
+                        marked_records = self.env.not_to_compute(field, records)
+                    self.env.add_to_compute(field, records)
+                else:
+                    if recursive:
+                        marked_records = records & self.env.cache.get_records(records, field)
+                    self.env.cache.invalidate([(field, records._ids)])
+                # recursively trigger recomputation of field's dependents
+                if recursive:
+                    marked_records.modified([field.name])
 
     def _modified_triggers(self, tree, create=False):
-        """ Process a tree of field triggers on ``self``. """
+        """ Return an iterator traversing a tree of field triggers on ``self``,
+        traversing backwards field dependencies along the way, and yielding
+        pairs ``(field, records)`` to recompute.
+        """
         if not self:
             return
+
+        # first yield what to compute
+        for field in tree.get(None, ()):
+            yield field, self
+
+        # then traverse dependencies backwards, and proceed recursively
         for key, val in tree.items():
             if key is None:
-                # val is a list of fields to mark as todo
-                for field in val:
-                    records = self - self.env.protected(field)
-                    if not records:
-                        continue
-                    # Dont force the recomputation of compute fields which are
-                    # not stored as this is not really necessary.
-                    recursive = not create and field.recursive
-                    if field.compute and field.store:
-                        if recursive:
-                            added = self.env.not_to_compute(field, records)
-                        self.env.add_to_compute(field, records)
-                    else:
-                        if recursive:
-                            added = self & self.env.cache.get_records(self, field)
-                        self.env.cache.invalidate([(field, records._ids)])
-                    # recursively trigger recomputation of field's dependents
-                    if recursive:
-                        added.modified([field.name])
+                continue
             elif create and key.type in ('many2one', 'many2one_reference'):
                 # upon creation, no other record has a reference to self
                 continue
@@ -5814,7 +5856,7 @@ Fields:
                     if new_records:
                         cache_records = self.env.cache.get_records(model, key)
                         records |= cache_records.filtered(lambda r: set(r[key.name]._ids) & set(self._ids))
-                records._modified_triggers(val)
+                yield from records._modified_triggers(val)
 
     @api.model
     def recompute(self, fnames=None, records=None):