[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.
This commit is contained in:
Raphael Collet
@@ -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):
|
||||
|
||||
+103
-29
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user