From da48700b59da49cdc810eac31a8617fcaa0dc2a8 Mon Sep 17 00:00:00 2001 From: std-odoo Date: Tue, 4 Oct 2022 09:09:46 +0000 Subject: [PATCH] [IMP] base: do not modify the cache when reading properties fields in batch Purpose ======= Do not modify the cache in order to improve the performance when we read in batch. Instead we create a new batched method "convert_to_read" that check existence in batch, and generate a dict with the result. Now, all the properties field checks (many2one existence, selection option still exists, tag value stiff exist, etc) and done in "convert_to_read". It means that doing `record.properties` won't do all those checks. Having the batched field fetch at the convert_to_record required too many changes for the scope of this task, so "record.properties" having the cache values unchecked is an acceptable tradeoff currently, hence we batch convert_to_read. Task-2980121 Part-of: odoo/odoo#101901 --- .../test_new_api/tests/test_properties.py | 24 +++-- odoo/fields.py | 95 +++++++++---------- odoo/models.py | 21 +++- 3 files changed, 80 insertions(+), 60 deletions(-) diff --git a/odoo/addons/test_new_api/tests/test_properties.py b/odoo/addons/test_new_api/tests/test_properties.py index 0bd1bc53507..eed97fbf02d 100644 --- a/odoo/addons/test_new_api/tests/test_properties.py +++ b/odoo/addons/test_new_api/tests/test_properties.py @@ -592,7 +592,7 @@ class PropertiesCase(TransactionCase): # 1 query to read the field # 1 query to read the definition # 2 queries to check if the many2one still exists / name_get - self.assertFalse(self.message_2.attributes[0]['value']) + self.assertFalse(self.message_2.read(['attributes'])[0]['attributes'][0]['value']) # remove the partner, and use the read method self.message_2.attributes = [{ @@ -844,9 +844,9 @@ class PropertiesCase(TransactionCase): # the value must remain in the database until the next write on the child self.assertEqual(self._get_sql_properties(message), {'my_tags': ['be', 'de']}) - + attributes = message.read(['attributes'])[0]['attributes'] self.assertEqual( - message.attributes[0]['value'], + attributes[0]['value'], ['be'], msg='The tag has been removed on the definition, should be removed when reading the child') self.assertEqual( @@ -854,7 +854,7 @@ class PropertiesCase(TransactionCase): [['be', 'BE', 1], ['fr', 'FR', 2], ['it', 'IT', 1]]) # next write on the child must update the value - message.attributes = message.attributes + message.attributes = message.read(['attributes'])[0]['attributes'] self.assertEqual(self._get_sql_properties(message), {'my_tags': ['be']}) @@ -890,7 +890,7 @@ class PropertiesCase(TransactionCase): 'comodel': 'test_new_api.partner', }] - with self.assertQueryCount(2): + with self.assertQueryCount(5): self.message_1.attributes = [ { "name": "moderator_partner_ids", @@ -900,11 +900,13 @@ class PropertiesCase(TransactionCase): "value": partners[:10].name_get(), } ] - self.assertEqual(self.message_1.attributes[0]['value'], partners[:10].ids) + attributes = self.message_1.read(['attributes'], load=None)[0]['attributes'] + self.assertEqual(attributes[0]['value'], partners[:10].ids) partners[:5].unlink() - with self.assertQueryCount(5): - self.assertEqual(self.message_1.attributes[0]['value'], partners[5:10].ids) + with self.assertQueryCount(4): + attributes = self.message_1.read(['attributes'], load=None)[0]['attributes'] + self.assertEqual(attributes[0]['value'], partners[5:10].ids) partners[5].unlink() with self.assertQueryCount(5): @@ -1045,6 +1047,12 @@ class PropertiesCase(TransactionCase): ] self.message_1.flush_recordset() + last_message_id = self.env['test_new_api.message'].search([], order="id DESC", limit=1).id + # based on batch optimization, _read_format should not crash on non existing records + values = self.env['test_new_api.message'].browse((self.message_1.id, last_message_id + 1))._read_format(['attributes']) + self.assertEqual(len(values), 1) + self.assertEqual(values[0]['id'], self.message_1.id) + def test_properties_field_change_definition(self): """Test the behavior of the field when changing the definition.""" diff --git a/odoo/fields.py b/odoo/fields.py index fd91d6d849e..cdfe7d271f1 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -3290,7 +3290,6 @@ class Properties(Field): assert isinstance(value, dict), f"Wrong type {value!r}" value = self._dict_to_list(value, definition) - self._parse_json_types(value, record.env) return value @@ -3312,10 +3311,22 @@ class Properties(Field): # }] # def convert_to_read(self, value, record, use_name_get=True): + return self.convert_to_read_multi([value], record, use_name_get)[0] + + def convert_to_read_multi(self, values, records, use_name_get=True): + assert len(values) == len(records) + + res_ids_per_model = self._get_res_ids_per_model(records, values, use_name_get) + # value is in record format + for value in values: + self._parse_json_types(value, records.env, res_ids_per_model) + if use_name_get: - self._add_display_name(value, record.env) - return value + for value in values: + self._add_display_name(value, records.env) + + return values def convert_to_write(self, value, record): """If we write a list on the child, update the definition record.""" @@ -3330,37 +3341,29 @@ class Properties(Field): self._add_display_name(value, record.env) return value - def read(self, records): + def _get_res_ids_per_model(self, records, values_list, use_name_get=True): """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 + in batch, we prefetch 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 - } + Return a dict {model: record_ids} that contains + the existing ids for each needed models. + """ # 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: + for record, record_values in zip(records, values_list): + for property_definition in record_values: 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 - + property_value = property_definition.get('value') or [] default = property_definition.get('default') or [] - property_value = record_values[name] or [] + + if type_ not in ('many2one', 'many2many') or comodel not in records.env: + continue if type_ == 'many2one': default = [default] if default else [] @@ -3370,38 +3373,20 @@ class Properties(Field): ids_per_model[comodel].update(property_value) # check existence and pre-fetch in batch - existing_ids_per_model = {} + res_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 + res_ids_per_model[model] = set(recs.ids) - # 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 + if use_name_get: + for record in recs: + # read a field to pre-fetch the recordset + try: + record.display_name + except AccessError: + pass - 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) + return res_ids_per_model def write(self, records, value): """Check if the properties definition has been changed. @@ -3591,7 +3576,7 @@ class Properties(Field): definition['name'] = str(uuid.uuid4()).replace('-', '')[:16] @classmethod - def _parse_json_types(cls, values_list, env): + def _parse_json_types(cls, values_list, env, res_ids_per_model): """Parse the value stored in the JSON. Check for records existence, if we removed a selection option, ... @@ -3633,6 +3618,9 @@ class Properties(Field): if not isinstance(property_value, int): raise ValueError(f'Wrong many2one value: {property_value!r}.') + if property_value not in res_ids_per_model[res_model]: + property_value = False + 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}.') @@ -3641,6 +3629,11 @@ class Properties(Field): # remove duplicated value and preserve order property_value = list(dict.fromkeys(property_value)) + property_value = [ + id_ for id_ in property_value + if id_ in res_ids_per_model[res_model] + ] + property_definition['value'] = property_value @classmethod diff --git a/odoo/models.py b/odoo/models.py index a91e57d7d45..1213b3ab4d3 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -3144,13 +3144,32 @@ class BaseModel(metaclass=MetaModel): The output format is the one expected from the `read` method, which uses this method as its implementation for formatting values. + For the properties fields, call convert_to_read_multi instead of convert_to_read + to prepare everything (record existences, display name, etc) in batch. + The current method is different from `read` because it retrieves its values from the cache without doing a query when it is avoidable. """ data = [(record, {'id': record._ids[0]}) for record in self] use_name_get = (load == '_classic_read') for name in fnames: - convert = self._fields[name].convert_to_read + field = self._fields[name] + if field.type == 'properties': + values_list = [] + records = [] + for record, vals in data: + try: + values_list.append(record[name]) + records.append(record.id) + except MissingError: + vals.clear() + + results = field.convert_to_read_multi(values_list, self.browse(records), use_name_get) + for record_read_vals, convert_result in zip(data, results): + record_read_vals[1][name] = convert_result + continue + + convert = field.convert_to_read for record, vals in data: # missing records have their vals empty if not vals: