[IMP] base: improve the performance of properties when used in batch
Purpose ======= Check the existence of the relational properties (many2one / many2many) in batch and prefetch the values in batch as well to reduce the number of SQL queries. Technical ========= The existence is checked in the read method of the properties field, because we have the entire recordset. Then, the non-existing ids are remove from the properties values and the cache is updated. Task-2965523 X-original-commit: dba9b684d29c0041a32a5508284573851b8dd097 Part-of: odoo/odoo#102243
This commit is contained in:
std-odoo
@@ -161,6 +161,7 @@ class PropertiesCase(TransactionCase):
|
||||
self.assertEqual(sql_values_1, {'discussion_color_code': 'orange', 'moderator_partner_id': self.partner_2.id, 'state': 'done'})
|
||||
self.assertEqual(sql_values_3, {'discussion_color_code': 'orange', 'moderator_partner_id': self.partner_2.id, 'state': 'done'})
|
||||
|
||||
@mute_logger('odoo.models.unlink', 'odoo.fields')
|
||||
def test_properties_field_read_batch(self):
|
||||
values = self.message_1.read(['attributes'])[0]['attributes']
|
||||
self.assertEqual(len(values), 2)
|
||||
@@ -186,11 +187,35 @@ class PropertiesCase(TransactionCase):
|
||||
with self.assertQueryCount(5), self.assertQueries(expected_queries):
|
||||
self.message_1.read(['attributes'])
|
||||
|
||||
# read in batch a lot of records
|
||||
discussions = [self.discussion_1, self.discussion_2]
|
||||
partners = self.env['test_new_api.partner'].create([{'name': f'Test {i}'} for i in range(50)])
|
||||
messages = self.env['test_new_api.message'].create([{
|
||||
'name': f'Test Message {i}',
|
||||
'discussion': discussions[i % 2].id,
|
||||
'author': self.user.id,
|
||||
'attributes': [{
|
||||
'name': 'partner_id',
|
||||
'type': 'many2one',
|
||||
'comodel': 'test_new_api.partner',
|
||||
'value': partner.id,
|
||||
'definition_changed': True,
|
||||
}]
|
||||
} for i, partner in enumerate(partners)])
|
||||
|
||||
self.env.invalidate_all()
|
||||
expected_queries += expected_queries[-2:]
|
||||
with self.assertQueryCount(7), self.assertQueries(expected_queries):
|
||||
# 2 more queries for message 2 to verify his partner existence / name_get
|
||||
(self.message_1 | self.message_2).read(['attributes'])
|
||||
|
||||
with self.assertQueryCount(5), self.assertQueries(expected_queries):
|
||||
values = messages.read(['attributes'])
|
||||
|
||||
# remove some partners in the list
|
||||
partners[:20].unlink()
|
||||
self.env.invalidate_all()
|
||||
# 5 queries instead of 25 queries, thanks to the cache values that has been
|
||||
# cleaned (the properties field can trust the cached value, the deleted ids
|
||||
# are not in the cache even if they still exists in the database)
|
||||
with self.assertQueryCount(5):
|
||||
values = messages.read(['attributes'])
|
||||
|
||||
@mute_logger('odoo.fields')
|
||||
def test_properties_field_delete(self):
|
||||
@@ -824,7 +849,7 @@ class PropertiesCase(TransactionCase):
|
||||
'comodel': 'test_new_api.partner',
|
||||
}]
|
||||
|
||||
with self.assertQueryCount(4):
|
||||
with self.assertQueryCount(2):
|
||||
self.message_1.attributes = [
|
||||
{
|
||||
"name": "moderator_partner_ids",
|
||||
@@ -837,7 +862,7 @@ class PropertiesCase(TransactionCase):
|
||||
self.assertEqual(self.message_1.attributes[0]['value'], partners[:10].ids)
|
||||
|
||||
partners[:5].unlink()
|
||||
with self.assertQueryCount(4):
|
||||
with self.assertQueryCount(5):
|
||||
self.assertEqual(self.message_1.attributes[0]['value'], partners[5:10].ids)
|
||||
|
||||
partners[5].unlink()
|
||||
@@ -847,7 +872,9 @@ class PropertiesCase(TransactionCase):
|
||||
|
||||
# need to wait next write to clean data in database
|
||||
# a single read won't clean the removed many2many
|
||||
self.message_1.attributes = self.message_1.read(['attributes'])[0]['attributes']
|
||||
attributes = self.message_1.read(['attributes'])[0]['attributes']
|
||||
self.message_1.invalidate_recordset()
|
||||
self.message_1.attributes = attributes
|
||||
|
||||
sql_values = self._get_sql_properties(self.message_1)
|
||||
self.assertEqual(sql_values, {'moderator_partner_ids': partners[6:10].ids})
|
||||
@@ -915,7 +942,7 @@ class PropertiesCase(TransactionCase):
|
||||
}])
|
||||
|
||||
@users('test')
|
||||
@mute_logger('odoo.addons.base.models.ir_rule')
|
||||
@mute_logger('odoo.addons.base.models.ir_rule', 'odoo.fields')
|
||||
def test_properties_field_many2many_filtering(self):
|
||||
# a user read a properties with a many2many and he doesn't have access to all records
|
||||
tags = self.env['test_new_api.multi.tag'].create(
|
||||
@@ -949,14 +976,12 @@ class PropertiesCase(TransactionCase):
|
||||
self.assertEqual(values, [(tag.id, None if i >= 5 else tag.name) for i, tag in enumerate(tags.sudo())])
|
||||
|
||||
def test_properties_field_performance(self):
|
||||
with self.assertQueryCount(4):
|
||||
self.message_1.attributes
|
||||
self.env.invalidate_all()
|
||||
with self.assertQueryCount(5):
|
||||
# read to put the partner name in cache
|
||||
self.message_1.read(['attributes'])
|
||||
|
||||
expected = ['SELECT "test_new_api_partner".id FROM "test_new_api_partner" WHERE "test_new_api_partner".id IN %s']
|
||||
with self.assertQueryCount(1, msg='Must read value from cache'), self.assertQueries(expected):
|
||||
# still cost 1 SQL query to check existence because the ORM stores
|
||||
# the raw SQL response in cache (not the result of convert_to_cache)
|
||||
# so the value in cache is not verified (see models.py@_read)
|
||||
with self.assertQueryCount(0, msg='Must read value from cache'):
|
||||
self.message_1.attributes
|
||||
|
||||
expected = ['UPDATE "test_new_api_message" SET "attributes" = %s, "write_date" = %s, "write_uid" = %s WHERE id IN %s']
|
||||
@@ -1086,6 +1111,7 @@ class PropertiesCase(TransactionCase):
|
||||
|
||||
# change the definition record, change the definition and add default values
|
||||
self.assertEqual(message.discussion, self.discussion_2)
|
||||
|
||||
with self.assertQueryCount(4):
|
||||
message.discussion = self.discussion_1
|
||||
self.assertEqual(
|
||||
@@ -1265,8 +1291,10 @@ class PropertiesCase(TransactionCase):
|
||||
"""Check the access right related to the Properties fields."""
|
||||
MultiTag = type(self.env['test_new_api.multi.tag'])
|
||||
|
||||
def _mocked_check_access_rights(*args, **kwargs):
|
||||
raise AccessError('')
|
||||
def _mocked_check_access_rights(operation, raise_exception=True):
|
||||
if raise_exception:
|
||||
raise AccessError('')
|
||||
return False
|
||||
|
||||
# a user read a properties with a many2one to a record he doesn't have access to
|
||||
tag = self.env['test_new_api.multi.tag'].create({'name': 'Test Tag'})
|
||||
|
||||
+75
-10
@@ -3257,7 +3257,7 @@ class Properties(Field):
|
||||
if not value or not definition:
|
||||
return definition or []
|
||||
|
||||
assert isinstance(value, dict)
|
||||
assert isinstance(value, dict), f"Wrong type {value!r}"
|
||||
value = self._dict_to_list(value, definition)
|
||||
self._parse_json_types(value, record.env)
|
||||
|
||||
@@ -3299,6 +3299,79 @@ class Properties(Field):
|
||||
self._add_display_name(value, record.env)
|
||||
return value
|
||||
|
||||
def read(self, records):
|
||||
"""Read everything needed in batch for the given records.
|
||||
|
||||
To retrieve relational properties names, or to check their existence,
|
||||
we need to do some SQL queries. To reduce the number of queries when we read
|
||||
in batch, we put in cache everything needed before calling
|
||||
convert_to_record / convert_to_read.
|
||||
"""
|
||||
definition_records_map = {
|
||||
record: record[self.definition_record][self.definition_record_field]
|
||||
for record in records
|
||||
}
|
||||
|
||||
# ids per model we need to fetch in batch to put in cache
|
||||
ids_per_model = defaultdict(OrderedSet)
|
||||
|
||||
records_cached_values = list(records.env.cache.get_values(records, self))
|
||||
|
||||
for record, record_values in zip(records, records_cached_values):
|
||||
definition = definition_records_map.get(record)
|
||||
if not record_values or not definition:
|
||||
continue
|
||||
for property_definition in definition:
|
||||
comodel = property_definition.get('comodel')
|
||||
type_ = property_definition.get('type')
|
||||
name = property_definition.get('name')
|
||||
if not comodel or type_ not in ('many2one', 'many2many') or name not in record_values:
|
||||
continue
|
||||
|
||||
default = property_definition.get('default') or []
|
||||
property_value = record_values[name] or []
|
||||
|
||||
if type_ == 'many2one':
|
||||
default = [default] if default else []
|
||||
property_value = [property_value] if property_value else []
|
||||
|
||||
ids_per_model[comodel].update(default)
|
||||
ids_per_model[comodel].update(property_value)
|
||||
|
||||
# check existence and pre-fetch in batch
|
||||
existing_ids_per_model = {}
|
||||
for model, ids in ids_per_model.items():
|
||||
recs = records.env[model].browse(ids).exists()
|
||||
existing_ids_per_model[model] = set(recs.ids)
|
||||
for record in recs:
|
||||
# read a field to pre-fetch the recordset
|
||||
try:
|
||||
record.display_name
|
||||
except AccessError:
|
||||
pass
|
||||
|
||||
# update the cache and remove non-existing ids
|
||||
for record, record_values in zip(records, records_cached_values):
|
||||
definition = definition_records_map.get(record)
|
||||
if not record_values or not definition:
|
||||
continue
|
||||
|
||||
for property_definition in definition:
|
||||
comodel = property_definition.get('comodel')
|
||||
type_ = property_definition.get('type')
|
||||
name = property_definition.get('name')
|
||||
if not comodel or type_ not in ('many2one', 'many2many') or not record_values.get(name):
|
||||
continue
|
||||
|
||||
property_value = record_values[name]
|
||||
|
||||
if type_ == 'many2one':
|
||||
record_values[name] = property_value if property_value in existing_ids_per_model[comodel] else False
|
||||
else:
|
||||
record_values[name] = [id_ for id_ in property_value if id_ in existing_ids_per_model[comodel]]
|
||||
|
||||
records.env.cache.update(record, self, [record_values], check_dirty=False)
|
||||
|
||||
def write(self, records, value):
|
||||
"""Check if the properties definition has been changed.
|
||||
|
||||
@@ -3487,7 +3560,7 @@ class Properties(Field):
|
||||
definition['name'] = str(uuid.uuid4()).replace('-', '')[:16]
|
||||
|
||||
@classmethod
|
||||
def _parse_json_types(cls, values_list, env, check_existence=True):
|
||||
def _parse_json_types(cls, values_list, env):
|
||||
"""Parse the value stored in the JSON.
|
||||
|
||||
Check for records existence, if we removed a selection option, ...
|
||||
@@ -3529,10 +3602,6 @@ class Properties(Field):
|
||||
if not isinstance(property_value, int):
|
||||
raise ValueError(f'Wrong many2one value: {property_value!r}.')
|
||||
|
||||
if check_existence and property_value:
|
||||
# many2one might have been deleted
|
||||
property_value = env[res_model].browse(property_value).exists().id
|
||||
|
||||
elif property_type == 'many2many' and property_value and res_model in env:
|
||||
if not is_list_of(property_value, int):
|
||||
raise ValueError(f'Wrong many2many value: {property_value!r}.')
|
||||
@@ -3541,10 +3610,6 @@ class Properties(Field):
|
||||
# remove duplicated value and preserve order
|
||||
property_value = list(dict.fromkeys(property_value))
|
||||
|
||||
elif check_existence and property_value:
|
||||
# many2many might have been deleted
|
||||
property_value = env[res_model].browse(property_value).exists().ids
|
||||
|
||||
property_definition['value'] = property_value
|
||||
|
||||
@classmethod
|
||||
|
||||
@@ -3185,6 +3185,12 @@ class BaseModel(metaclass=MetaModel):
|
||||
if field.store and field.translate:
|
||||
translated_field_names.append(field.name)
|
||||
|
||||
if field.type == 'properties':
|
||||
# force calling fields.read for properties field because
|
||||
# we want to read all relational properties in batch
|
||||
# (and check their existence in batch as well)
|
||||
other_fields.append(field)
|
||||
|
||||
if column_fields:
|
||||
cr, context = self.env.cr, self.env.context
|
||||
|
||||
|
||||
Reference in New Issue
Block a user