From 8974e928fa9f9b54ef3fbaf685aedbcccd2c755d Mon Sep 17 00:00:00 2001 From: Olivier Dony Date: Wed, 23 Jul 2014 12:30:24 +0200 Subject: [PATCH] [FIX] fields: do not revalidate field values unless they are being modified In the previous implementation of the new API fields, both fields.Selection and fields.Reference were performing early validation of their `value` as soon as it entered the cache, either by being read, written, or computed. This is a source of trouble and performance problems, and is unnecessary, as we should consider that the database always contains valid values. If that is not the case it means it was modified externally and is an exception that should be handled externally as well. Revalidating selection/reference values can be expensive when the domain of values is dynamic and requires extra database queries, with extra access rights control, etc. This patch adds a `validate` parameter to `convert_to_cache`, allowing to turn off the re-validation on demand. The ORM will turn off validation whenever the value being converted is supposed to be already validated, such as when reading it from the database. The parameter is currently ignored by all other fields, and defaults to True so validation is performed in all other caes. --- openerp/fields.py | 40 +++++++++++++++++++++++++--------------- openerp/models.py | 13 ++++++------- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/openerp/fields.py b/openerp/fields.py index ff50f73d1b8..586e76db39e 100644 --- a/openerp/fields.py +++ b/openerp/fields.py @@ -604,16 +604,23 @@ class Field(object): """ return the null value for this field in the given environment """ return False - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): """ convert `value` to the cache level in `env`; `value` may come from an assignment, or have the format of methods :meth:`BaseModel.read` or :meth:`BaseModel.write` + + :param bool validate: when True, field-specific validation of + `value` will be performed """ return value def convert_to_read(self, value, use_name_get=True): """ convert `value` from the cache to a value as returned by method :meth:`BaseModel.read` + + :param bool use_name_get: when True, value's diplay name will + be computed using :meth:`BaseModel.name_get`, if relevant + for the field """ return value @@ -753,7 +760,7 @@ class Field(object): try: values = target._convert_to_cache({ f.name: source[f.name] for f in self.computed_fields - }) + }, validate=False) except MissingError as e: values = FailedValue(e) target._cache.update(values) @@ -855,7 +862,7 @@ class Boolean(Field): """ Boolean field. """ type = 'boolean' - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): return bool(value) def convert_to_export(self, value, env): @@ -868,7 +875,7 @@ class Integer(Field): """ Integer field. """ type = 'integer' - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): return int(value or 0) def convert_to_read(self, value, use_name_get=True): @@ -908,7 +915,7 @@ class Float(Field): _column_digits = property(lambda self: not callable(self._digits) and self._digits) _column_digits_compute = property(lambda self: callable(self._digits) and self._digits) - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): # apply rounding here, otherwise value in cache may be wrong! if self.digits: return float_round(float(value or 0.0), precision_digits=self.digits[1]) @@ -942,7 +949,7 @@ class Char(_String): _related_size = property(attrgetter('size')) _description_size = property(attrgetter('size')) - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): return bool(value) and ustr(value)[:self.size] @@ -956,7 +963,7 @@ class Text(_String): """ type = 'text' - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): return bool(value) and ustr(value) @@ -964,7 +971,7 @@ class Html(_String): """ Html field. """ type = 'html' - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): return bool(value) and html_sanitize(value) @@ -1013,7 +1020,7 @@ class Date(Field): """ Convert a :class:`date` value into the format expected by the ORM. """ return value.strftime(DATE_FORMAT) - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): if not value: return False if isinstance(value, basestring): @@ -1078,7 +1085,7 @@ class Datetime(Field): """ Convert a :class:`datetime` value into the format expected by the ORM. """ return value.strftime(DATETIME_FORMAT) - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): if not value: return False if isinstance(value, basestring): @@ -1158,7 +1165,9 @@ class Selection(Field): selection = selection(env[self.model_name]) return [value for value, _ in selection] - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): + if not validate: + return value or False if value in self.get_values(env): return value elif not value: @@ -1196,9 +1205,10 @@ class Reference(Selection): _column_size = property(attrgetter('size')) - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): if isinstance(value, BaseModel): - if value._name in self.get_values(env) and len(value) <= 1: + if ((not validate or value._name in self.get_values(env)) + and len(value) <= 1): return value.with_env(env) or False elif isinstance(value, basestring): res_model, res_id = value.split(',') @@ -1293,7 +1303,7 @@ class Many2one(_Relational): """ Update the cached value of `self` for `records` with `value`. """ records._cache[self] = value - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): if isinstance(value, (NoneType, int)): return env[self.comodel_name].browse(value) if isinstance(value, BaseModel): @@ -1346,7 +1356,7 @@ class _RelationalMulti(_Relational): for record in records: record._cache[self] = record[self.name] | value - def convert_to_cache(self, value, env): + def convert_to_cache(self, value, env, validate=True): if isinstance(value, BaseModel): if value._name == self.comodel_name: return value.with_env(env) diff --git a/openerp/models.py b/openerp/models.py index ecc8c7263fa..fffdd93a53c 100644 --- a/openerp/models.py +++ b/openerp/models.py @@ -3136,7 +3136,7 @@ class BaseModel(object): if field not in self._cache: for values in result: record = self.browse(values.pop('id')) - record._cache.update(record._convert_to_cache(values)) + record._cache.update(record._convert_to_cache(values, validate=False)) if field not in self._cache: e = AccessError("No value found for %s.%s" % (self, field.name)) self._cache[field] = FailedValue(e) @@ -3215,7 +3215,7 @@ class BaseModel(object): # store result in cache for POST fields for vals in result: record = self.browse(vals['id']) - record._cache.update(record._convert_to_cache(vals)) + record._cache.update(record._convert_to_cache(vals, validate=False)) # determine the fields that must be processed now fields_post = [f for f in field_names if not self._columns[f]._classic_write] @@ -3256,7 +3256,7 @@ class BaseModel(object): # store result in cache for vals in result: record = self.browse(vals.pop('id')) - record._cache.update(record._convert_to_cache(vals)) + record._cache.update(record._convert_to_cache(vals, validate=False)) # store failed values in cache for the records that could not be read fetched = self.browse(ids) @@ -3596,7 +3596,6 @@ class BaseModel(object): if not self: return True - cr, uid, context = self.env.args self._check_concurrency(self._ids) self.check_access_rights('write') @@ -5098,11 +5097,11 @@ class BaseModel(object): context = dict(args[0] if args else self._context, **kwargs) return self.with_env(self.env(context=context)) - def _convert_to_cache(self, values): + def _convert_to_cache(self, values, validate=True): """ Convert the `values` dictionary into cached values. """ fields = self._fields return { - name: fields[name].convert_to_cache(value, self.env) + name: fields[name].convert_to_cache(value, self.env, validate=validate) for name, value in values.iteritems() if name in fields } @@ -5555,7 +5554,7 @@ class BaseModel(object): return if 'value' in method_res: method_res['value'].pop('id', None) - self.update(self._convert_to_cache(method_res['value'])) + self.update(self._convert_to_cache(method_res['value'], validate=False)) if 'domain' in method_res: result.setdefault('domain', {}).update(method_res['domain']) if 'warning' in method_res: