From 6f30f0af9cd8af3dbde1af5b6fce4efb0e41e67c Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 18 Jan 2018 12:33:21 +0100 Subject: [PATCH] [FIX] models: behavior with multiple inverse/compute mixed on same fields --- .../models/base_address_extended.py | 8 +-- odoo/addons/test_new_api/ir.model.access.csv | 1 + odoo/addons/test_new_api/models.py | 34 +++++++++-- .../test_new_api/tests/test_new_fields.py | 61 +++++++++++++++---- odoo/models.py | 28 +++++---- odoo/tools/misc.py | 7 +++ 6 files changed, 105 insertions(+), 34 deletions(-) diff --git a/addons/base_address_extended/models/base_address_extended.py b/addons/base_address_extended/models/base_address_extended.py index a2c31249951..1ca0178ba20 100644 --- a/addons/base_address_extended/models/base_address_extended.py +++ b/addons/base_address_extended/models/base_address_extended.py @@ -74,12 +74,7 @@ class Partner(models.Model): # add trailing chars in street_format street_value += street_format[previous_pos:] - - # /!\ Note that we must use a sql query to bypass the orm as it would call _split_street() - # that would try to set the fields we just modified. - self._cr.execute('UPDATE res_partner SET street = %s WHERE ID = %s', (street_value, partner.id)) - #invalidate the cache for the field we manually set - self.invalidate_cache(['street'], [partner.id]) + partner.street = street_value @api.multi @api.depends('street') @@ -133,7 +128,6 @@ class Partner(models.Model): else: vals[field_name] = street_raw # assign the values to the fields - # /!\ Note that a write(vals) would cause a recursion since it would bypass the cache for k, v in vals.items(): partner[k] = v diff --git a/odoo/addons/test_new_api/ir.model.access.csv b/odoo/addons/test_new_api/ir.model.access.csv index fd10a216720..46a658c818d 100644 --- a/odoo/addons/test_new_api/ir.model.access.csv +++ b/odoo/addons/test_new_api/ir.model.access.csv @@ -18,5 +18,6 @@ access_test_new_api_company,access_test_new_api_company,model_test_new_api_compa access_test_new_api_company_attr,access_test_new_api_company_attr,model_test_new_api_company_attr,,1,1,1,1 access_test_new_api_compute_inverse,access_test_new_api_compute_inverse,model_test_new_api_compute_inverse,,1,1,1,1 access_test_new_api_compute_protected,access_test_new_api_compute_protected,model_test_new_api_compute_protected,,1,1,1,1 +access_test_new_api_multi_compute_inverse,access_test_new_api_multi_compute_inverse,model_test_new_api_multi_compute_inverse,,1,1,1,1 access_test_new_api_recursive,access_test_new_api_recursive,model_test_new_api_recursive,,1,1,1,1 access_test_new_api_cascade,access_test_new_api_cascade,model_test_new_api_cascade,,1,1,1,1 diff --git a/odoo/addons/test_new_api/models.py b/odoo/addons/test_new_api/models.py index 274d8257d04..70e1d1b2d2c 100644 --- a/odoo/addons/test_new_api/models.py +++ b/odoo/addons/test_new_api/models.py @@ -342,28 +342,54 @@ class ComputeProtected(models.Model): class ComputeInverse(models.Model): _name = 'test_new_api.compute.inverse' - counts = {'compute': 0, 'inverse': 0} - foo = fields.Char() bar = fields.Char(compute='_compute_bar', inverse='_inverse_bar', store=True) @api.depends('foo') def _compute_bar(self): - self.counts['compute'] += 1 + self._context.get('log', []).append('compute') for record in self: record.bar = record.foo def _inverse_bar(self): - self.counts['inverse'] += 1 + self._context.get('log', []).append('inverse') for record in self: record.foo = record.bar +class MultiComputeInverse(models.Model): + """ Model with the same inverse method for several fields. """ + _name = 'test_new_api.multi_compute_inverse' + + foo = fields.Char(default='', required=True) + bar1 = fields.Char(compute='_compute_bars', inverse='_inverse_bar1', store=True) + bar2 = fields.Char(compute='_compute_bars', inverse='_inverse_bar23', store=True) + bar3 = fields.Char(compute='_compute_bars', inverse='_inverse_bar23', store=True) + + @api.depends('foo') + def _compute_bars(self): + self._context.get('log', []).append('compute') + for record in self: + substrs = record.foo.split('/') + ['', '', ''] + record.bar1, record.bar2, record.bar3 = substrs[:3] + + def _inverse_bar1(self): + self._context.get('log', []).append('inverse1') + for record in self: + record.write({'foo': '/'.join([record.bar1, record.bar2, record.bar3])}) + + def _inverse_bar23(self): + self._context.get('log', []).append('inverse23') + for record in self: + record.write({'foo': '/'.join([record.bar1, record.bar2, record.bar3])}) + + class CompanyDependent(models.Model): _name = 'test_new_api.company' foo = fields.Char(company_dependent=True) + class CompanyDependentAttribute(models.Model): _name = 'test_new_api.company.attr' diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index c0fa1bc7b59..09605f37aa2 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -278,33 +278,70 @@ class TestFields(common.TransactionCase): self.assertEqual(ewan.parent, cath) self.assertEqual(ewan.name, "Erwan") - record = self.env['test_new_api.compute.inverse'] - # create/write on 'foo' should only invoke the compute method - record.counts.update(compute=0, inverse=0) - record = record.create({'foo': 'Hi'}) + log = [] + model = self.env['test_new_api.compute.inverse'].with_context(log=log) + record = model.create({'foo': 'Hi'}) self.assertEqual(record.foo, 'Hi') self.assertEqual(record.bar, 'Hi') - self.assertEqual(record.counts, {'compute': 1, 'inverse': 0}) + self.assertCountEqual(log, ['compute']) - record.counts.update(compute=0, inverse=0) + log.clear() record.write({'foo': 'Ho'}) self.assertEqual(record.foo, 'Ho') self.assertEqual(record.bar, 'Ho') - self.assertEqual(record.counts, {'compute': 1, 'inverse': 0}) + self.assertCountEqual(log, ['compute']) # create/write on 'bar' should only invoke the inverse method - record.counts.update(compute=0, inverse=0) - record = record.create({'bar': 'Hi'}) + log.clear() + record = model.create({'bar': 'Hi'}) self.assertEqual(record.foo, 'Hi') self.assertEqual(record.bar, 'Hi') - self.assertEqual(record.counts, {'compute': 0, 'inverse': 1}) + self.assertCountEqual(log, ['inverse']) - record.counts.update(compute=0, inverse=0) + log.clear() record.write({'bar': 'Ho'}) self.assertEqual(record.foo, 'Ho') self.assertEqual(record.bar, 'Ho') - self.assertEqual(record.counts, {'compute': 0, 'inverse': 1}) + self.assertCountEqual(log, ['inverse']) + + # Test compatibility multiple compute/inverse fields + log = [] + model = self.env['test_new_api.multi_compute_inverse'].with_context(log=log) + record = model.create({ + 'bar1': '1', + 'bar2': '2', + 'bar3': '3', + }) + self.assertEqual(record.foo, '1/2/3') + self.assertEqual(record.bar1, '1') + self.assertEqual(record.bar2, '2') + self.assertEqual(record.bar3, '3') + self.assertCountEqual(log, ['inverse1', 'inverse23']) + + log.clear() + record.write({'bar2': '4', 'bar3': '5'}) + self.assertEqual(record.foo, '1/4/5') + self.assertEqual(record.bar1, '1') + self.assertEqual(record.bar2, '4') + self.assertEqual(record.bar3, '5') + self.assertCountEqual(log, ['inverse23']) + + log.clear() + record.write({'bar1': '6', 'bar2': '7'}) + self.assertEqual(record.foo, '6/7/5') + self.assertEqual(record.bar1, '6') + self.assertEqual(record.bar2, '7') + self.assertEqual(record.bar3, '5') + self.assertCountEqual(log, ['inverse1', 'inverse23']) + + log.clear() + record.write({'foo': 'A/B/C'}) + self.assertEqual(record.foo, 'A/B/C') + self.assertEqual(record.bar1, 'A') + self.assertEqual(record.bar2, 'B') + self.assertEqual(record.bar3, 'C') + self.assertCountEqual(log, ['compute']) def test_14_search(self): """ test search on computed fields """ diff --git a/odoo/models.py b/odoo/models.py index 0af021810cc..3dcfeb74315 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -50,7 +50,7 @@ from . import tools from .exceptions import AccessError, MissingError, ValidationError, UserError from .osv.query import Query from .tools import frozendict, lazy_classproperty, lazy_property, ormcache, \ - Collector, LastOrderedSet, OrderedSet, pycompat + Collector, LastOrderedSet, OrderedSet, pycompat, groupby from .tools.config import config from .tools.func import frame_codeinfo from .tools.misc import CountingStream, DEFAULT_SERVER_DATETIME_FORMAT, DEFAULT_SERVER_DATE_FORMAT @@ -2978,6 +2978,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): inverse_vals = {} inherited_vals = defaultdict(dict) # {modelname: {fieldname: value}} unknown_names = [] + inverse_fields = [] protected_fields = [] for key, val in vals.items(): if key in bad_names: @@ -2992,7 +2993,8 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): inherited_vals[field.related_field.model_name][key] = val elif field.inverse: inverse_vals[key] = val - protected_fields.append(field) + inverse_fields.append(field) + protected_fields.extend(self._field_computed.get(field, [field])) if unknown_names: _logger.warning("%s.write() with unknown fields: %s", @@ -3021,8 +3023,11 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): self.modified(set(inverse_vals) - set(store_vals)) for record in self: record._cache.update(record._convert_to_cache(inverse_vals, update=True)) - for key in inverse_vals: - self._fields[key].determine_inverse(self) + + # in case several fields use the same inverse method, call it once + for fields in groupby(attrgetter('inverse'), inverse_fields): + fields[0].determine_inverse(self) + self.modified(set(inverse_vals) - set(store_vals)) # check Python constraints for inversed fields @@ -3174,6 +3179,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): inverse_vals = {} inherited_vals = defaultdict(dict) # {modelname: {fieldname: value}} unknown_names = [] + inverse_fields = [] protected_fields = [] for key, val in vals.items(): if key in bad_names: @@ -3188,7 +3194,8 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): inherited_vals[field.related_field.model_name][key] = val elif field.inverse: inverse_vals[key] = val - protected_fields.append(field) + inverse_fields.append(field) + protected_fields.extend(self._field_computed.get(field, [field])) if unknown_names: _logger.warning("%s.create() with unknown fields: %s", @@ -3209,8 +3216,11 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): with self.env.protecting(protected_fields, record): # put the values of inverse fields in cache, and inverse them record._cache.update(record._convert_to_cache(inverse_vals)) - for key in inverse_vals: - self._fields[key].determine_inverse(record) + + # in case several fields use the same inverse method, call it once + for fields in groupby(attrgetter('inverse'), inverse_fields): + fields[0].determine_inverse(record) + record.modified(set(inverse_vals) - set(store_vals)) # check Python constraints for inversed fields @@ -3303,10 +3313,6 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): # check Python constraints self._validate_fields(vals) - # recompute fields - if self.env.recompute and self._context.get('recompute', True): - self.recompute() - self.check_access_rule('create') # add translations diff --git a/odoo/tools/misc.py b/odoo/tools/misc.py index a9b66117f03..5fc1f18e1b2 100644 --- a/odoo/tools/misc.py +++ b/odoo/tools/misc.py @@ -1062,6 +1062,13 @@ class LastOrderedSet(OrderedSet): OrderedSet.discard(self, elem) OrderedSet.add(self, elem) +def groupby(key, elems): + """ Return a partition of ``elems`` based on ``key(elem)``. """ + groups = defaultdict(list) + for elem in elems: + groups[key(elem)].append(elem) + return groups.values() + def unique(it): """ "Uniquifier" for the provided iterable: will output each element of the iterable once.