From 384fda2c2adaf1a14942197f0a96b182b5db0345 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 23 Jun 2022 08:58:26 +0000 Subject: [PATCH] [REF] core: replace towrite by dirty flag in cache Merging both the memory of field values and suspended updates has several advantages: - avoid inconsistencies between cache and towrite - cache updates can be made safer w.r.t. dirty flag However, the dirty flag in cache does not go well with context-dependent fields. When a context-dependent field is dirty in cache, the value to store in the database is accessible through some context values. But when the model is flushed, the context values on the current environment may be different. When this happens, the method flush() fails to retrieve the data to flush. The proposed solution is to store the "dirty" value in cache under conventional context values, and to retrieve them under the same conventional context values to flush them. For instance, when storing the value of a binary field, it will be stored once under the context value `context.get('bin_size')`, and a second time under the context value `None`. The flush implementation will then retrieve the value using the context value `None`. Translated fields are also problematic when a value is put in cache with an environment where lang=False, and the value is retrieved with another environment where lang=None. This issue is addressed by normalizing the context key 'lang' to None when the context value is False. Part-of: odoo/odoo#95325 Co-authored-by: Vincent Schippefilt --- odoo/addons/base/models/ir_model.py | 6 +- odoo/addons/base/models/ir_translation.py | 2 +- odoo/api.py | 176 ++++++++++++++++++++-- odoo/fields.py | 52 +++---- odoo/models.py | 51 ++++--- 5 files changed, 218 insertions(+), 69 deletions(-) diff --git a/odoo/addons/base/models/ir_model.py b/odoo/addons/base/models/ir_model.py index f99a94b8fbf..c9cf5e87ff0 100644 --- a/odoo/addons/base/models/ir_model.py +++ b/odoo/addons/base/models/ir_model.py @@ -779,11 +779,11 @@ class IrModelFields(models.Model): return # remove pending write of this field - # DLE P16: if there are pending towrite of the field we currently try to unlink, pop them out from the towrite queue + # DLE P16: if there are pending updates of the field we currently try to unlink, pop them out from the cache # test `test_unlink_with_dependant` for record in self: - for record_values in self.env.all.towrite[record.model].values(): - record_values.pop(record.name, None) + field = self.pool[record.model]._fields[record.name] + self.env.cache.clear_dirty_field(field) # remove fields from registry, and check that views are not broken fields = [self.env[record.model]._pop_field(record.name) for record in self] domain = expression.OR([('arch_db', 'like', record.name)] for record in self) diff --git a/odoo/addons/base/models/ir_translation.py b/odoo/addons/base/models/ir_translation.py index 70aef57b756..cb74e377eae 100644 --- a/odoo/addons/base/models/ir_translation.py +++ b/odoo/addons/base/models/ir_translation.py @@ -593,7 +593,7 @@ class IrTranslation(models.Model): # When assigning a translation to a field # e.g. email.with_context(lang='fr_FR').label = "bonjour" # and then search on translations for this translation, must flush as the translation has not yet been written in database - if any(self.env[model]._fields[field].translate for model, ids in self.env.all.towrite.items() for record_id, fields in ids.items() for field in fields): + if any(field.translate for field in self.env.cache.get_dirty_fields()): self.env.flush_all() return super(IrTranslation, self)._search(args, offset=offset, limit=limit, order=order, count=count, access_rights_uid=access_rights_uid) diff --git a/odoo/api.py b/odoo/api.py index 4a1dc46d31c..854d597bb26 100644 --- a/odoo/api.py +++ b/odoo/api.py @@ -682,7 +682,7 @@ class Environment(Mapping): :rtype: str """ - return self.context.get('lang') + return self.context.get('lang') or None def clear(self): """ Clear all record caches, and discard all fields to recompute. @@ -720,7 +720,7 @@ class Environment(Mapping): def flush_all(self): """ Flush all pending computations and updates to the database. """ self._recompute_all() - for model_name in list(self.all.towrite): + for model_name in OrderedSet(field.model_name for field in self.cache.get_dirty_fields()): self[model_name].flush_model() def is_protected(self, field, record): @@ -810,6 +810,8 @@ class Environment(Mapping): return self.company.id elif key == 'uid': return (self.uid, self.su) + elif key == 'lang': + return get_context('lang') or None elif key == 'active_test': return get_context('active_test', field.context.get('active_test', True)) else: @@ -844,8 +846,6 @@ class Transaction: self.protected = StackMap() # pending computations {field: ids} self.tocompute = defaultdict(OrderedSet) - # pending updates {model: {id: {field: value}}} - self.towrite = defaultdict(lambda: defaultdict(dict)) def flush(self): """ Flush pending computations and updates in the transaction. """ @@ -860,9 +860,8 @@ class Transaction: def clear(self): """ Clear the caches and pending computations and updates in the translations. """ - self.cache.invalidate() + self.cache.clear() self.tocompute.clear() - self.towrite.clear() def reset(self): """ Reset the transaction. This clears the transaction, and reassigns @@ -882,11 +881,55 @@ EMPTY_DICT = frozendict() class Cache(object): - """ Implementation of the cache of records. """ + """ Implementation of the cache of records. + + For most fields, the cache is simply a mapping from a record and a field to + a value. In the case of context-dependent fields, the mapping also depends + on the environment of the given record. For the sake of performance, the + cache is first partitioned by field, then by record. This makes some + common ORM operations pretty fast, like determining which records have a + value for a given field, or invalidating a given field on all possible + records. + + The cache can also mark some entries as "dirty". Dirty entries essentially + marks values that are different from the database. They represent database + updates that haven't been done yet. Note that dirty entries only make + sense for stored fields. Note also that if a field is dirty on a given + record, and the field is context-dependent, then all the values of the + record for that field are considered dirty. For the sake of consistency, + the values that should be in the database must be in a context where all + the field's context keys are ``None``. + """ + def __init__(self): # {field: {record_id: value}, field: {context_key: {record_id: value}}} self._data = defaultdict(dict) + # {field: set[id]} stores the fields and ids that are changed in the + # cache, but not yet written in the database; their changed values are + # in `_data` + self._dirty = defaultdict(OrderedSet) + + def __repr__(self): + # for debugging: show the cache content and dirty flags as stars + data = {} + for field, field_cache in sorted(self._data.items(), key=lambda item: str(item[0])): + dirty_ids = self._dirty.get(field, ()) + if field_cache and isinstance(next(iter(field_cache)), tuple): + data[field] = { + key: { + Starred(id_) if id_ in dirty_ids else id_: val + for id_, val in key_cache.items() + } + for key, key_cache in field_cache.items() + } + else: + data[field] = { + Starred(id_) if id_ in dirty_ids else id_: val + for id_, val in field_cache.items() + } + return repr(data) + def _get_field_cache(self, model, field): """ Return the field cache of the given field, but not for modifying it. """ field_cache = self._data.get(field, EMPTY_DICT) @@ -915,15 +958,63 @@ class Cache(object): raise CacheMiss(record, field) return default - def set(self, record, field, value): - """ Set the value of ``field`` for ``record``. """ + def set(self, record, field, value, dirty=False, check_dirty=True): + """ Set the value of ``field`` for ``record``. + One can normally make a clean field dirty but not the other way around. + Updating a dirty field without ``dirty=True`` is a programming error and + raises an exception. + + :param dirty: whether ``field`` must be made dirty on ``record`` after + the update + :param check_dirty: whether updating a dirty field without making it + dirty must raise an exception + """ field_cache = self._set_field_cache(record, field) field_cache[record._ids[0]] = value + if not check_dirty: + return + if dirty: + assert field.column_type and field.store and record.id + self._dirty[field].add(record.id) + if record.pool.field_depends_context[field]: + # put the values under conventional context key values {'context_key': None}, + # in order to ease the retrieval of those values to flush them + context_none = dict.fromkeys(record.pool.field_depends_context[field]) + record = record.with_env(record.env(context=context_none)) + field_cache = self._set_field_cache(record, field) + field_cache[record._ids[0]] = value + elif record.id in self._dirty.get(field, ()): + _logger.error("cache.set() removing flag dirty on %s.%s", record, field.name, stack_info=True) - def update(self, records, field, values): - """ Set the values of ``field`` for several ``records``. """ + def update(self, records, field, values, dirty=False, check_dirty=True): + """ Set the values of ``field`` for several ``records``. + One can normally make a clean field dirty but not the other way around. + Updating a dirty field without ``dirty=True`` is a programming error and + raises an exception. + + :param dirty: whether ``field`` must be made dirty on ``record`` after + the update + :param check_dirty: whether updating a dirty field without making it + dirty must raise an exception + """ field_cache = self._set_field_cache(records, field) field_cache.update(zip(records._ids, values)) + if not check_dirty: + return + if dirty: + assert field.column_type and field.store and all(records._ids) + self._dirty[field].update(records._ids) + if records.pool.field_depends_context[field]: + # put the values under conventional context key values {'context_key': None}, + # in order to ease the retrieval of those values to flush them + context_none = dict.fromkeys(records.pool.field_depends_context[field]) + records = records.with_env(records.env(context=context_none)) + field_cache = self._set_field_cache(records, field) + field_cache.update(zip(records._ids, values)) + else: + dirty_ids = self._dirty.get(field) + if dirty_ids and not dirty_ids.isdisjoint(records._ids): + _logger.error("cache.update() removing flag dirty on %s.%s", records, field.name, stack_info=True) def insert_missing(self, records, field, values): """ Set the values of ``field`` for the records in ``records`` that @@ -936,6 +1027,7 @@ class Cache(object): def remove(self, record, field): """ Remove the value of ``field`` for ``record``. """ + assert record.id not in self._dirty.get(field, ()) try: field_cache = self._set_field_cache(record, field) del field_cache[record._ids[0]] @@ -994,8 +1086,48 @@ class Cache(object): if record_id not in field_cache: yield record_id + def get_dirty_fields(self): + """ Return the fields that have dirty records in cache. """ + return self._dirty.keys() + + def has_dirty_fields(self, records, fields=None): + """ Return whether any of the given records has dirty fields. + + :param fields: a collection of fields or ``None``; the value ``None`` is + interpreted as any field on ``records`` + """ + if fields is None: + return any( + not ids.isdisjoint(records._ids) + for field, ids in self._dirty.items() + if field.model_name == records._name + ) + else: + return any( + field in self._dirty and not self._dirty[field].isdisjoint(records._ids) + for field in fields + ) + + def clear_dirty_field(self, field): + """ Make the given field clean on all records, and return the ids of the + formerly dirty records for the field. + """ + return self._dirty.pop(field, ()) + def invalidate(self, spec=None): - """ Invalidate the cache, partially or totally depending on ``spec``. """ + """ Invalidate the cache, partially or totally depending on ``spec``. + + If a field is context-dependent, invalidating it for a given record + actually invalidates all the values of that field on the record. In + other words, the field is invalidated for the record in all + environments. + + This operation is unsafe by default, and must be used with care. + Indeed, invalidating a dirty field on a record may lead to an error, + because doing so drops the value to be written in database. + + spec = [(field, ids), (field, None), ...] + """ if spec is None: self._data.clear() elif spec: @@ -1011,6 +1143,11 @@ class Cache(object): for id_ in ids: field_cache.pop(id_, None) + def clear(self): + """ Invalidate the cache and its dirty flags. """ + self._data.clear() + self._dirty.clear() + def check(self, env): """ Check the consistency of the cache for the given environment. """ depends_context = env.registry.field_depends_context @@ -1018,8 +1155,8 @@ class Cache(object): def process(model, field, field_cache): # ignore new records and records to flush - towrite = env.all.towrite.get(model._name) or {} - ids = [id_ for id_ in field_cache if id_ and field.name not in towrite.get(id_, ())] + dirty_ids = self._dirty.get(field, ()) + ids = [id_ for id_ in field_cache if id_ and id_ not in dirty_ids] if not ids: return @@ -1063,6 +1200,17 @@ class Cache(object): _logger.warning("Invalid cache: %s", pformat(invalids)) +class Starred: + """ Simple helper class to ``repr`` a value with a star suffix. """ + __slots__ = ['value'] + + def __init__(self, value): + self.value = value + + def __repr__(self): + return f"{self.value!r}*" + + # keep those imports here in order to handle cyclic dependencies correctly from odoo import SUPERUSER_ID from odoo.modules.registry import Registry diff --git a/odoo/fields.py b/odoo/fields.py index 0ebb328d140..8a1fe92e5cf 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -1087,16 +1087,8 @@ class Field(MetaField('DummyField', (object,), {})): return records # update the cache - cache.update(records, self, itertools.repeat(cache_value)) - - # update towrite - if self.store: - towrite = records.env.all.towrite[self.model_name] - record = records[:1] - write_value = self.convert_to_write(cache_value, record) - column_value = self.convert_to_column(write_value, record) - for record in records.filtered('id'): - towrite[record.id][self.name] = column_value + dirty = self.store and any(records._ids) + cache.update(records, self, itertools.repeat(cache_value), dirty=dirty) return records @@ -1681,29 +1673,29 @@ class _String(Field): if not records: return records - lang = records.env.lang or None # used in _update_translations below + lang = records.env.lang installed = records.env['res.lang'].get_installed() single_lang = installed[0][0] if len(installed) <= 1 else None # modify the column (source) if single_lang or lang in (None, 'en_US') or callable(self.translate) or not cache_value: - towrite = records.env.all.towrite[self.model_name] - for rid in records._ids: - # cache_value is already in database format - towrite[rid][self.name] = cache_value if self.translate is True and cache_value: tname = f"{self.model_name},{self.name}" records.env['ir.translation']._set_source(tname, records._ids, value) # invalidate the field in all languages because the fallback value # for translations is modified cache.invalidate([(self, records.ids)]) - if single_lang or lang == 'en_US': - # modifying with lang=None also updates the installed language, - # and modifying with a lang also updates for lang=None - others = records.with_context(lang=None if lang else single_lang) - cache.update(others, self, itertools.repeat(cache_value)) - - cache.update(records, self, itertools.repeat(cache_value)) + cache.update(records, self, itertools.repeat(cache_value), dirty=True) + if single_lang and not lang: + # modifying with lang=None also updates the installed language + others = records.with_context(lang=single_lang) + cache.update(others, self, itertools.repeat(cache_value), dirty=True) + else: + # Ignore the dirty flag when updating the value in cache. This is + # necessary for translated fields, when you have to update the + # field's value in a language without making it dirty while the + # field's column value is already dirty. + cache.update(records, self, itertools.repeat(cache_value), check_dirty=False) if callable(self.translate): # the source value of self has been updated, synchronize translated @@ -2222,7 +2214,8 @@ class Binary(Field): except (TypeError): pass cache_value = self.convert_to_cache(value, record) - cache.set(record, self, cache_value) + dirty = self.column_type and self.store and any(records._ids) + cache.set(record, self, cache_value, dirty=dirty) except CacheMiss: pass else: @@ -2363,7 +2356,8 @@ class Image(Binary): super(Image, self).write(records, new_value) cache_value = self.convert_to_cache(value if self.related else new_value, records) - records.env.cache.update(records, self, itertools.repeat(cache_value)) + dirty = self.column_type and self.store and any(records._ids) + records.env.cache.update(records, self, itertools.repeat(cache_value), dirty=dirty) def _image_process(self, value): if self.readonly and not self.max_width and not self.max_height: @@ -2928,14 +2922,8 @@ class Many2one(_Relational): self._remove_inverses(records, cache_value) # update the cache of self - cache.update(records, self, itertools.repeat(cache_value)) - - # update towrite - if self.store: - towrite = records.env.all.towrite[self.model_name] - for record in records.filtered('id'): - # cache_value is already in database format - towrite[record.id][self.name] = cache_value + dirty = self.store and any(records._ids) + cache.update(records, self, itertools.repeat(cache_value), dirty=dirty) # update the cache of one2many fields of new corecord self._update_inverses(records, cache_value) diff --git a/odoo/models.py b/odoo/models.py index c2e5fbaf816..3c7b1d36cb0 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -5534,7 +5534,8 @@ class BaseModel(metaclass=MetaModel): # convert monetary fields after other columns for correct value rounding for field, value in sorted(field_values, key=lambda item: item[0].write_sequence): - cache.set(self, field, field.convert_to_cache(value, self, validate)) + value = field.convert_to_cache(value, self, validate) + cache.set(self, field, value, check_dirty=False) # set inverse fields on new records in the comodel if field.relational: @@ -5832,17 +5833,8 @@ class BaseModel(metaclass=MetaModel): :param fnames: optional iterable of field names to flush """ self._recompute_recordset(fnames) - towrite = self.env.all.towrite.get(self._name) - if fnames is None: - must_flush = towrite and any(id_ in towrite for id_ in self._ids) - else: - must_flush = towrite and any( - fname in vals - for id_ in self._ids - for vals in towrite.get(id_, ()) - for fname in fnames - ) - if must_flush: + fields_ = None if fnames is None else (self._fields[fname] for fname in fnames) + if self.env.cache.has_dirty_fields(self, fields_): self._flush(fnames) def _flush(self, fnames=None): @@ -5875,13 +5867,32 @@ class BaseModel(metaclass=MetaModel): model_fields[field.related_field.model_name].append(field.related_field) for model_name, fields_ in model_fields.items(): - if any( - field.name in vals - for vals in self.env.all.towrite.get(model_name, {}).values() - for field in fields_ - ): - id_vals = self.env.all.towrite.pop(model_name) - process(self.env[model_name], id_vals) + dirty_fields = self.env.cache.get_dirty_fields() + if any(field in dirty_fields for field in fields_): + # if any field is context-dependent, the values to flush should + # be found with a context where the context keys are all None + context_none = dict.fromkeys( + key + for field in fields_ + for key in self.pool.field_depends_context[field] + ) + model = self.env(context=context_none)[model_name] + id_vals = defaultdict(dict) + for field in model._fields.values(): + ids = self.env.cache.clear_dirty_field(field) + if not ids: + continue + records = model.browse(ids) + values = list(self.env.cache.get_values(records, field)) + assert len(values) == len(records), \ + f"Could not find all values of {field} to flush them\n" \ + f" Context: {self.env.context}\n" \ + f" Cache: {self.env.cache!r}" + for record, value in zip(records, values): + value = field.convert_to_write(value, record) + value = field.convert_to_column(value, record) + id_vals[record.id][field.name] = value + process(model, id_vals) # flush the inverse of one2many fields, too for field in fields: @@ -6209,7 +6220,9 @@ class BaseModel(metaclass=MetaModel): spec = [] for field in fields: spec.append((field, ids)) + # TODO VSC: used to remove the inverse of many_to_one from the cache, though we might not need it anymore for invf in self.pool.field_inverses[field]: + self.env[invf.model_name].flush_model([invf.name]) spec.append((invf, None)) self.env.cache.invalidate(spec)