diff --git a/odoo/addons/test_new_api/tests/test_properties.py b/odoo/addons/test_new_api/tests/test_properties.py index f2bb8df2534..913d59a3ae8 100644 --- a/odoo/addons/test_new_api/tests/test_properties.py +++ b/odoo/addons/test_new_api/tests/test_properties.py @@ -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'}) diff --git a/odoo/fields.py b/odoo/fields.py index f6a88aefc4a..98dc39c5151 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -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 diff --git a/odoo/models.py b/odoo/models.py index 180dc0dbb62..e684da6f886 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -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