From 39be91b48ec2a0aa037ca17f776689ce9f59b1cc Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 25 Oct 2023 09:24:16 +0200 Subject: [PATCH] [IMP] core: clean up the ACL tracebacks some Currently if a record is not in the cache it raises a `CacheMiss` the handling of which leads to loading the record from the database (or something). If an issue occurs during that loading, because the loading is an `except` scope the cache miss gets linked with the new one via > During handling of the above exception, another exception occurred: This is both noise (the cache miss is not actually relevant) and misleading, because the wording makes it look like an unrelated error occurred during handling. - move the loading of the record out of the `except` to limit the scope of the `KeyError` and avoid "inheriting" it - try to `from None` a few specific errors to remove implicit linkage, as the new error should have all relevant information, the key error / cache miss is just an implementation detail closes odoo/odoo#139680 Signed-off-by: Xavier Morel (xmo) --- odoo/addons/base/models/ir_model.py | 2 +- odoo/api.py | 2 +- odoo/fields.py | 175 ++++++++++++++-------------- 3 files changed, 90 insertions(+), 89 deletions(-) diff --git a/odoo/addons/base/models/ir_model.py b/odoo/addons/base/models/ir_model.py index 55595fe8405..64ff764146d 100644 --- a/odoo/addons/base/models/ir_model.py +++ b/odoo/addons/base/models/ir_model.py @@ -1935,7 +1935,7 @@ class IrModelAccess(models.Model): group_info=group_info, resolution_info=resolution_info) - raise AccessError(msg) + raise AccessError(msg) from None return has_access diff --git a/odoo/api.py b/odoo/api.py index 35b10823638..6298596ec4d 100644 --- a/odoo/api.py +++ b/odoo/api.py @@ -969,7 +969,7 @@ class Cache(object): return cache_value except KeyError: if default is NOTHING: - raise CacheMiss(record, field) + raise CacheMiss(record, field) from None return default def set(self, record, field, value, dirty=False, check_dirty=True): diff --git a/odoo/fields.py b/odoo/fields.py index 140a1f7da07..213099f0610 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -1159,100 +1159,101 @@ class Field(MetaField('DummyField', (object,), {})): try: value = env.cache.get(record, self) - + return self.convert_to_record(value, record) except KeyError: - # behavior in case of cache miss: - # - # on a real record: - # stored -> fetch from database (computation done above) - # not stored and computed -> compute - # not stored and not computed -> default - # - # on a new record w/ origin: - # stored and not (computed and readonly) -> fetch from origin - # stored and computed and readonly -> compute - # not stored and computed -> compute - # not stored and not computed -> default - # - # on a new record w/o origin: - # stored and computed -> compute - # stored and not computed -> new delegate or default - # not stored and computed -> compute - # not stored and not computed -> default - # - if self.store and record.id: - # real record: fetch from database - recs = record._in_cache_without(self) - try: - recs._fetch_field(self) - except AccessError: - if len(recs) == 1: - raise - record._fetch_field(self) - if not env.cache.contains(record, self): - raise MissingError("\n".join([ - _("Record does not exist or has been deleted."), - _("(Record: %s, User: %s)") % (record, env.uid), - ])) - value = env.cache.get(record, self) + pass + # behavior in case of cache miss: + # + # on a real record: + # stored -> fetch from database (computation done above) + # not stored and computed -> compute + # not stored and not computed -> default + # + # on a new record w/ origin: + # stored and not (computed and readonly) -> fetch from origin + # stored and computed and readonly -> compute + # not stored and computed -> compute + # not stored and not computed -> default + # + # on a new record w/o origin: + # stored and computed -> compute + # stored and not computed -> new delegate or default + # not stored and computed -> compute + # not stored and not computed -> default + # + if self.store and record.id: + # real record: fetch from database + recs = record._in_cache_without(self) + try: + recs._fetch_field(self) + except AccessError: + if len(recs) == 1: + raise + record._fetch_field(self) + if not env.cache.contains(record, self): + raise MissingError("\n".join([ + _("Record does not exist or has been deleted."), + _("(Record: %s, User: %s)", record, env.uid), + ])) from None + value = env.cache.get(record, self) - elif self.store and record._origin and not (self.compute and self.readonly): - # new record with origin: fetch from origin - value = self.convert_to_cache(record._origin[self.name], record) - env.cache.set(record, self, value) + elif self.store and record._origin and not (self.compute and self.readonly): + # new record with origin: fetch from origin + value = self.convert_to_cache(record._origin[self.name], record) + env.cache.set(record, self, value) - elif self.compute: - # non-stored field or new record without origin: compute - if env.is_protected(self, record): - value = self.convert_to_cache(False, record, validate=False) - env.cache.set(record, self, value) - else: - recs = record if self.recursive else record._in_cache_without(self) - try: - self.compute_value(recs) - except (AccessError, MissingError): - self.compute_value(record) - try: - value = env.cache.get(record, self) - except CacheMiss: - if self.readonly and not self.store: - raise ValueError("Compute method failed to assign %s.%s" % (record, self.name)) - # fallback to null value if compute gives nothing - value = self.convert_to_cache(False, record, validate=False) - env.cache.set(record, self, value) - - elif self.type == 'many2one' and self.delegate and not record.id: - # parent record of a new record: new record, with the same - # values as record for the corresponding inherited fields - def is_inherited_field(name): - field = record._fields[name] - return field.inherited and field.related.split('.')[0] == self.name - - parent = record.env[self.comodel_name].new({ - name: value - for name, value in record._cache.items() - if is_inherited_field(name) - }) - # in case the delegate field has inverse one2many fields, this - # updates the inverse fields as well - record._update_cache({self.name: parent}, validate=False) - value = env.cache.get(record, self) - - else: - # non-stored field or stored field on new record: default value + elif self.compute: #pylint: disable=using-constant-test + # non-stored field or new record without origin: compute + if env.is_protected(self, record): value = self.convert_to_cache(False, record, validate=False) env.cache.set(record, self, value) - defaults = record.default_get([self.name]) - if self.name in defaults: - # The null value above is necessary to convert x2many field - # values. For instance, converting [(Command.LINK, id)] - # accesses the field's current value, then adds the given - # id. Without an initial value, the conversion ends up here - # to determine the field's value, and generates an infinite - # recursion. - value = self.convert_to_cache(defaults[self.name], record) + else: + recs = record if self.recursive else record._in_cache_without(self) + try: + self.compute_value(recs) + except (AccessError, MissingError): + self.compute_value(record) + try: + value = env.cache.get(record, self) + except CacheMiss: + if self.readonly and not self.store: + raise ValueError(f"Compute method failed to assign {record}.{self.name}") from None + # fallback to null value if compute gives nothing + value = self.convert_to_cache(False, record, validate=False) env.cache.set(record, self, value) + elif self.type == 'many2one' and self.delegate and not record.id: + # parent record of a new record: new record, with the same + # values as record for the corresponding inherited fields + def is_inherited_field(name): + field = record._fields[name] + return field.inherited and field.related.split('.')[0] == self.name + + parent = record.env[self.comodel_name].new({ + name: value + for name, value in record._cache.items() + if is_inherited_field(name) + }) + # in case the delegate field has inverse one2many fields, this + # updates the inverse fields as well + record._update_cache({self.name: parent}, validate=False) + value = env.cache.get(record, self) + + else: + # non-stored field or stored field on new record: default value + value = self.convert_to_cache(False, record, validate=False) + env.cache.set(record, self, value) + defaults = record.default_get([self.name]) + if self.name in defaults: + # The null value above is necessary to convert x2many field + # values. For instance, converting [(Command.LINK, id)] + # accesses the field's current value, then adds the given + # id. Without an initial value, the conversion ends up here + # to determine the field's value, and generates an infinite + # recursion. + value = self.convert_to_cache(defaults[self.name], record) + env.cache.set(record, self, value) + return self.convert_to_record(value, record) def mapped(self, records):