Skip to content

Commit

Permalink
[FIX] models: make onchange return a smaller diff
Browse files Browse the repository at this point in the history 
Large diffs (specifically in x2many fields) cause performance issues.  They
overload the web client, which considers all returned fields as dirty.
  • Loading branch information
@rco-odoo
rco-odoo committed Nov 30, 2018
1 parent bfb0f1e commit 95f4287
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 80 deletions.
81 changes: 41 additions & 40 deletions odoo/addons/test_new_api/tests/test_onchange.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,12 @@ def test_onchange_many2one(self):

def test_onchange_one2many(self):
""" test the effect of onchange() on one2many fields """
BODY = "What a beautiful day!"
USER = self.env.user

# create an independent message
message = self.Message.create({'body': BODY})
self.assertEqual(message.name, "[%s] %s" % ('', USER.name))
message1 = self.Message.create({'body': "ABC"})
message2 = self.Message.create({'body': "ABC"})
self.assertEqual(message1.name, "[%s] %s" % ('', USER.name))

field_onchange = self.Discussion._onchange_spec()
self.assertEqual(field_onchange.get('name'), '1')
Expand All @@ -130,33 +130,35 @@ def test_onchange_one2many(self):
'moderator': False,
'participants': [],
'messages': [
(4, message.id),
(4, message1.id),
(4, message2.id),
(1, message2.id, {'body': "XYZ"}),
(0, 0, {
'name': "[%s] %s" % ('', USER.name),
'body': BODY,
'body': "ABC",
'author': USER.id,
'size': len(BODY),
'size': 3,
'important': False,
}),
],
}
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, {
(1, message1.id, {
'name': "[%s] %s" % ("Foo", USER.name),
'body': message.body,
'author': message.author.name_get()[0],
'size': message.size,
'important': message.important,
}),
(1, message2.id, {
'name': "[%s] %s" % ("Foo", USER.name),
'body': "XYZ", # this must be sent back
}),
(0, 0, {
'name': "[%s] %s" % ("Foo", USER.name),
'body': BODY,
'body': "ABC",
'author': USER.name_get()[0],
'size': len(BODY),
'size': 3,
'important': False,
}),
])
Expand All @@ -167,7 +169,8 @@ def test_onchange_one2many(self):
result = self.Discussion.with_context(generate_dummy_message=True).onchange(values, 'name', one_level_fields)
self.assertEqual(result['value']['messages'], [
(5,),
(4, message.id),
(4, message1.id),
(4, message2.id),
(0, 0, {}),
(0, 0, {}),
])
Expand Down Expand Up @@ -260,8 +263,7 @@ def test_onchange_one2many_multi(self):
'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,)]}),
Expand All @@ -280,18 +282,30 @@ def test_onchange_one2many_multi(self):
self.env.cache.invalidate()

result = multi.onchange(values, 'partner', field_onchange)
self.assertEqual(result['value'], {
expected_value = {
'name': partner2.name,
'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'})]}),
],
})
}
self.assertEqual(result['value'], expected_value)

# ensure ID is not returned when asked and a many2many record is set to be created
self.env.cache.invalidate()

result = multi.onchange(values, 'partner', dict(field_onchange, **{'lines.tags.id': None}))
self.assertEqual(result['value'], expected_value)

# ensure inverse of one2many field is not returned
self.env.cache.invalidate()

result = multi.onchange(values, 'partner', dict(field_onchange, **{'lines.multi': None}))
self.assertEqual(result['value'], expected_value)

def test_onchange_specific(self):
""" test the effect of field-specific onchange method """
Expand Down Expand Up @@ -323,8 +337,7 @@ def test_onchange_specific(self):
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):
Expand Down Expand Up @@ -360,6 +373,8 @@ def test_onchange_one2many_value(self):
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,
Expand All @@ -370,8 +385,7 @@ def test_onchange_one2many_value(self):
}
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"""
Expand Down Expand Up @@ -417,28 +431,15 @@ def test_onchange_one2many_with_domain_on_related_field(self):
'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):
Expand Down
129 changes: 89 additions & 40 deletions odoo/models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5044,31 +5044,98 @@ def onchange(self, values, field_name, field_onchange):
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
def PrefixTree(model, dotnames):
""" Return a prefix tree for sequences of field names. """
if not dotnames:
return {}
# group dotnames by prefix
suffixes = defaultdict(list)
for dotname in dotnames:
# name, *names = dotname.split('.', 1)
names = dotname.split('.', 1)
name = names.pop(0)
suffixes[name].extend(names)
# fill in prefix tree in fields order
tree = OrderedDict()
for name, field in model._fields.items():
if name in suffixes:
tree[name] = subtree = PrefixTree(model[name], suffixes[name])
if subtree and field.type == 'one2many':
subtree.pop(field.inverse_name, None)
return tree

class Snapshot(dict):
""" A dict with the values of a record, following a prefix tree. """
__slots__ = ()

def __init__(self, record, tree):
# put record in dict to include it when comparing snapshots
super(Snapshot, self).__init__({'<record>': record, '<tree>': tree})
for name, subnames in tree.items():
# x2many fields are serialized as a list of line snapshots
self[name] = (
[Snapshot(line, subnames) for line in record[name]]
if subnames else record[name]
)

def diff(self, other):
""" Return the values in ``self`` that differ from ``other``.
Requires record cache invalidation for correct output!
"""
record = self['<record>']
result = {}
for name, subnames in self['<tree>'].items():
if (name == 'id') or (other.get(name) == self[name]):
continue
if not subnames:
field = record._fields[name]
result[name] = field.convert_to_onchange(self[name], record, {})
else:
# x2many fields: serialize value as commands
result[name] = commands = [(5,)]
for line_snapshot in self[name]:
line = line_snapshot['<record>']
if not line.id:
# new line: send diff from scratch
line_diff = line_snapshot.diff({})
commands.append((0, line.id.ref or 0, line_diff))
else:
# existing line: send diff from database
# (requires a clean record cache!)
line_diff = line_snapshot.diff(Snapshot(line, subnames))
if line_diff:
commands.append((1, line.id, line_diff))
else:
commands.append((4, line.id))
return result

nametree = PrefixTree(self.browse(), field_onchange)

# 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}
# 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)
snapshot0 = snapshot1 = Snapshot(record, nametree)

# determine which field(s) should be triggered an onchange
todo = list(names) or list(values)
todo = list(names or nametree)
done = set()

# dummy assignment: trigger invalidations on the record
Expand All @@ -5084,7 +5151,6 @@ def onchange(self, values, field_name, field_onchange):
record[name] = value

result = {}
dirty = set()

# process names in order (or the keys of values if no name given)
while todo:
Expand All @@ -5098,34 +5164,17 @@ def onchange(self, values, field_name, field_onchange):
if field_onchange.get(name):
record._onchange_eval(name, field_onchange[name], result)

# force re-evaluation of function fields on secondary records
for dotname in dotnames:
record.mapped(dotname)
# make a snapshot (this forces evaluation of computed fields)
snapshot1 = Snapshot(record, nametree)

# determine which fields have been modified
for name, oldval in values.items():
field = self._fields[name]
newval = record[name]
if newval != oldval or (
field.type in ('one2many', 'many2many') and newval._is_dirty()
):
for name in nametree:
if snapshot1[name] != snapshot0[name]:
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
with env.do_in_onchange():
result['value'] = {
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'] = snapshot1.diff(snapshot0)

return result

Expand Down

0 comments on commit 95f4287

Please sign in to comment.