[FIX] models: make onchange return a smaller diff

Large diffs (specifically in x2many fields) cause performance issues.  They
overload the web client, which considers all returned fields as dirty.

odoo/addons/test_new_api/tests/test_onchange.py

@@ -139,14 +139,10 @@ class TestOnChange(common.TransactionCase):
         self.env.cache.invalidate()
         result = self.Discussion.onchange(values, 'name', field_onchange)
         self.assertIn('messages', result['value'])
-        self.assertItemsEqual(result['value']['messages'], [
+        self.assertEqual(result['value']['messages'], [
             (5,),
             (1, message.id, {
                 'name': "[%s] %s" % ("Foo", USER.name),
-                'body': message.body,
-                'author': message.author.name_get()[0],
-                'size': message.size,
-                'important': message.important,
             }),
             (0, 0, {
                 'name': "[%s] %s" % ("Foo", USER.name),
@@ -245,8 +241,7 @@ class TestOnChange(common.TransactionCase):
             'lines': [
                 (5,),
                 (1, line1.id, {'name': partner2.name,
-                               'partner': (partner2.id, partner2.name),
+                               'partner': (partner2.id, partner2.name)}),
-                               'tags': [(5,)]}),
                 (0, 0, {'name': partner2.name,
                         'partner': (partner2.id, partner2.name),
                         'tags': [(5,)]}),
@@ -270,8 +265,7 @@ class TestOnChange(common.TransactionCase):
             'lines': [
                 (5,),
                 (1, line1.id, {'name': partner2.name,
-                               'partner': (partner2.id, partner2.name),
+                               'partner': (partner2.id, partner2.name)}),
-                               'tags': [(5,)]}),
                 (0, 0, {'name': partner2.name,
                         'partner': (partner2.id, partner2.name),
                         'tags': [(5,), (0, 0, {'name': 'Tag'})]}),
@@ -308,8 +302,7 @@ class TestOnChange(common.TransactionCase):
         self.assertIn('participants', result['value'])
         self.assertItemsEqual(
             result['value']['participants'],
-            [(5,)] + [(1, user.id, {'display_name': user.display_name})
+            [(5,)] + [(4, user.id) for user in discussion.participants + demo],
-                      for user in discussion.participants + demo],
         )
 
     def test_onchange_default(self):
@@ -345,6 +338,8 @@ class TestOnChange(common.TransactionCase):
         self.assertEqual(len(discussion.messages), 3)
         messages = [(4, msg.id) for msg in discussion.messages]
         messages[0] = (1, messages[0][1], {'body': 'test onchange'})
+        lines = ["%s:%s" % (m.name, m.body) for m in discussion.messages]
+        lines[0] = "%s:%s" % (discussion.messages[0].name, 'test onchange')
         values = {
             'name': discussion.name,
             'moderator': demo.id,
@@ -355,8 +350,7 @@ class TestOnChange(common.TransactionCase):
         }
         result = discussion.onchange(values, 'messages', field_onchange)
         self.assertIn('message_concat', result['value'])
-        self.assertEqual(result['value']['message_concat'],
+        self.assertEqual(result['value']['message_concat'], "\n".join(lines))
-                         "\n".join(["%s:%s" % (m.name, m.body) for m in discussion.messages]))
 
     def test_onchange_one2many_with_domain_on_related_field(self):
         """ test the value of the one2many field when defined with a domain on a related field"""
@@ -402,28 +396,15 @@ class TestOnChange(common.TransactionCase):
             'categories': [(4, cat.id) for cat in discussion.categories],
             'messages': [(4, msg.id) for msg in discussion.messages],
             'participants': [(4, usr.id) for usr in discussion.participants],
-            'message_changes': 0,
             'important_messages': [(4, msg.id) for msg in discussion.important_messages],
             'important_emails': [(4, eml.id) for eml in discussion.important_emails],
         }
+        self.env.cache.invalidate()
         result = discussion.onchange(values, 'name', field_onchange)
 
-        # When one2many domain contains non-computed field, things are ok
-        self.assertEqual(result['value']['important_messages'],
-                         [(5,)] + [(4, msg.id) for msg in discussion.important_messages])
-
-        # But here with commit 5676d81, we get value of: [(2, email.id)]
         self.assertEqual(
             result['value']['important_emails'],
-            [(5,),
+            [(5,), (1, email.id, {'name': u'[Foo Bar] %s' % USER.name})],
-             (1, email.id, {
-                 'name': u'[Foo Bar] %s' % USER.name,
-                 'body': email.body,
-                 'author': USER.name_get()[0],
-                 'important': True,
-                 'email_to': demo.email,
-                 'size': email.size,
-             })]
         )
 
     def test_onchange_related(self):

odoo/models.py

@@ -5040,28 +5040,111 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})):
         if not all(name in self._fields for name in names):
             return {}
 
-        # filter out keys in field_onchange that do not refer to actual fields
+        class PrefixTree(OrderedDict):
-        dotnames = []
+            """ A prefix tree for sequences of field names. The tree is a
-        for dotname in field_onchange:
+                dictionary that associates each given field name to its
-            try:
+                corresponding subtree (in fields order)::
-                model = self.browse()
+
-                for name in dotname.split('.'):
+                    # tree corresponding to dotnames
-                    model = model[name]
+                    # ['name', 'line_ids.product_id', 'line_ids.tags_ids.name']
-                dotnames.append(dotname)
+                    {
-            except Exception:
+                        'name': {},
-                pass
+                        'line_ids': {
+                            'product_id': {},
+                            'tags_ids': {
+                                'name': {},
+                            },
+                        },
+                    }
+            """
+            def __init__(self, model, dotnames):
+                if not dotnames:
+                    return
+                # group dotnames by prefix
+                suffixes = defaultdict(list)
+                for dotname in dotnames:
+                    names = dotname.split('.', 1)
+                    name_suffixes = suffixes[names[0]]
+                    if len(names) > 1:
+                        name_suffixes.append(names[1])
+                # fill in self in fields order
+                for name in model._fields:
+                    if name in suffixes:
+                        self[name] = PrefixTree(model[name], suffixes[name])
+
+            def dotnames(self):
+                """ Iterate over the sequences of field names. """
+                for name, subnames in self.items():
+                    yield name
+                    for dotname in subnames.dotnames():
+                        yield "%s.%s" % (name, dotname)
+
+        nametree = PrefixTree(self.browse(), field_onchange)
+        dotnames = list(nametree.dotnames())
+
+        def snapshot(record, tree=nametree):
+            """ Return a dict with the values of record, following nametree. """
+            vals = {}
+            for name, subnames in tree.items():
+                if subnames:
+                    # x2many fields as {line: snapshot(line), ...}
+                    vals[name] = OrderedDict(
+                        (line, snapshot(line, subnames))
+                        for line in record[name]
+                    )
+                else:
+                    vals[name] = record[name]
+            return vals
+
+        def diff(record, old, new, tree=nametree):
+            """ Return the values that differ between snapshots.
+                The snapshot ``old`` may be empty (for new records).
+            """
+            result = {}
+            for name, subnames in tree.items():
+                if old and old[name] == new[name]:
+                    continue
+                field = record._fields[name]
+                if not subnames:
+                    result[name] = field.convert_to_onchange(new[name], record, subnames)
+                    continue
+                # x2many fields: serialize value as commands
+                result[name] = commands = [(5,)]
+                old_val = old.get(name) or {}
+                for line, vals in new[name].items():
+                    vals0 = (old_val.get(line) or snapshot(line, subnames)) if line.id else {}
+                    line_diff = diff(line, vals0, vals, subnames)
+                    if not line.id:
+                        commands.append((0, line.id.ref or 0, line_diff))
+                    elif line_diff:
+                        commands.append((1, line.id, line_diff))
+                    else:
+                        commands.append((4, line.id))
+            return result
+
+        # prefetch x2many lines without data (for the initial snapshot)
+        for name, subnames in nametree.items():
+            if subnames and values.get(name):
+                # retrieve all ids in commands, and read the expected fields
+                line_ids = []
+                for cmd in values[name]:
+                    if cmd[0] in (1, 4):
+                        line_ids.append(cmd[1])
+                    elif cmd[0] == 6:
+                        line_ids.extend(cmd[2])
+                lines = self.browse()[name].browse(line_ids)
+                lines.read(list(subnames), load='_classic_write')
 
         # create a new record with values, and attach ``self`` to it
         with env.do_in_onchange():
             record = self.new(values)
-            values = {name: record[name] for name in record._cache}
+            values = {name: record[name] for name in nametree}
             # attach ``self`` with a different context (for cache consistency)
             record._origin = self.with_context(__onchange=True)
 
-        # load fields on secondary records, to avoid false changes
+        # make a snapshot based on the initial values of record
         with env.do_in_onchange():
-            for dotname in dotnames:
+            before = snapshot(record)
-                record.mapped(dotname)
 
         # determine which field(s) should be triggered an onchange
         todo = list(names) or list(values)
@@ -5080,7 +5163,6 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})):
                 record[name] = value
 
         result = {}
-        dirty = set()
 
         # process names in order (or the keys of values if no name given)
         while todo:
@@ -5106,22 +5188,14 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})):
                         field.type in ('one2many', 'many2many') and newval._is_dirty()
                     ):
                         todo.append(name)
-                        dirty.add(name)
 
-        # determine subfields for field.convert_to_onchange() below
+        # make a snapshot based on the final values of record
-        Tree = lambda: defaultdict(Tree)
-        subnames = Tree()
-        for dotname in dotnames:
-            subtree = subnames
-            for name in dotname.split('.'):
-                subtree = subtree[name]
-
-        # collect values from dirty fields
         with env.do_in_onchange():
-            result['value'] = {
+            after = snapshot(record)
-                name: self._fields[name].convert_to_onchange(record[name], record, subnames[name])
+
-                for name in dirty
+        # determine values that have changed by comparing snapshots
-            }
+        self.invalidate_cache()
+        result['value'] = diff(record, before, after)
 
         return result