From 2054b5928574affbb7efcd8a9ab146ee4fa34938 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 7 Jun 2018 10:33:56 +0200 Subject: [PATCH] [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. --- .../test_new_api/tests/test_onchange.py | 37 ++--- odoo/models.py | 132 ++++++++++++++---- 2 files changed, 112 insertions(+), 57 deletions(-) diff --git a/odoo/addons/test_new_api/tests/test_onchange.py b/odoo/addons/test_new_api/tests/test_onchange.py index 7caf433a467..52fc0d06170 100644 --- a/odoo/addons/test_new_api/tests/test_onchange.py +++ b/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), - 'tags': [(5,)]}), + 'partner': (partner2.id, partner2.name)}), (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), - 'tags': [(5,)]}), + 'partner': (partner2.id, partner2.name)}), (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}) - for user in discussion.participants + demo], + [(5,)] + [(4, user.id) 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'], - "\n".join(["%s:%s" % (m.name, m.body) for m in discussion.messages])) + self.assertEqual(result['value']['message_concat'], "\n".join(lines)) 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,), - (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, - })] + [(5,), (1, email.id, {'name': u'[Foo Bar] %s' % USER.name})], ) def test_onchange_related(self): diff --git a/odoo/models.py b/odoo/models.py index 964dbcbf5d9..5a85708e0ea 100644 --- a/odoo/models.py +++ b/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 - dotnames = [] - for dotname in field_onchange: - try: - model = self.browse() - for name in dotname.split('.'): - model = model[name] - dotnames.append(dotname) - except Exception: - pass + class PrefixTree(OrderedDict): + """ A prefix tree for sequences of field names. The tree is a + dictionary that associates each given field name to its + corresponding subtree (in fields order):: + + # tree corresponding to dotnames + # ['name', 'line_ids.product_id', 'line_ids.tags_ids.name'] + { + 'name': {}, + '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: - record.mapped(dotname) + before = snapshot(record) # 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 - 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 + # make a snapshot based on the final values of record with env.do_in_onchange(): - result['value'] = { - name: self._fields[name].convert_to_onchange(record[name], record, subnames[name]) - for name in dirty - } + after = snapshot(record) + + # determine values that have changed by comparing snapshots + self.invalidate_cache() + result['value'] = diff(record, before, after) return result