From a426d9f398d427f7e0b023a3a770e1bfa6c6c40f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20=C3=89vrard?= Date: Mon, 12 Apr 2021 19:39:25 +0200 Subject: [PATCH] Use immutable datastructures for Dict and MultiSelection fields In case a Dict field is sent during an on_change, modification of its value won't be detected because a shallow copy will keep its reference in the pseudo-record issue10105 review347541002 --- CHANGELOG | 1 + trytond/model/fields/dict.py | 26 +++++++++++++++++++++- trytond/model/fields/multiselection.py | 7 +++++- trytond/tests/modelview.py | 13 ++++++++++- trytond/tests/test_field_dict.py | 13 +++++++++++ trytond/tests/test_field_multiselection.py | 14 ++++++------ trytond/tests/test_modelstorage.py | 2 +- trytond/tests/test_modelview.py | 18 ++++++++++++++- 8 files changed, 82 insertions(+), 12 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index f3c286791c..9a755a5cef 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,4 @@ +* Use immutable datastructures for Dict and MultiSelection fields * Skip warnings for non-interactive operations * Check rule only if _check_access is set * Add statistics to Cache diff --git a/trytond/model/fields/dict.py b/trytond/model/fields/dict.py index 849cc9f2ff..dc7d73c6c7 100644 --- a/trytond/model/fields/dict.py +++ b/trytond/model/fields/dict.py @@ -17,6 +17,25 @@ dumps = partial( json.dumps, cls=JSONEncoder, separators=(',', ':'), sort_keys=True) +class ImmutableDict(dict): + + __slots__ = () + + def _not_allowed(cls, *args, **kwargs): + raise TypeError("Operation not allowed on ImmutableDict") + + __setitem__ = _not_allowed + __delitem__ = _not_allowed + __ior__ = _not_allowed + clear = _not_allowed + pop = _not_allowed + popitem = _not_allowed + setdefault = _not_allowed + update = _not_allowed + + del _not_allowed + + class Dict(Field): 'Define dict field.' _type = 'dict' @@ -41,7 +60,7 @@ class Dict(Field): # If stored as JSON conversion is done on backend if isinstance(data, str): data = json.loads(data, object_hook=JSONDecoder()) - dicts[value['id']] = data + dicts[value['id']] = ImmutableDict(data) return dicts def sql_format(self, value): @@ -57,6 +76,11 @@ class Dict(Field): value = dumps(d) return value + def __set__(self, inst, value): + if value: + value = ImmutableDict(value) + super().__set__(inst, value) + def translated(self, name=None, type_='values'): "Return a descriptor for the translated value of the field" if name is None: diff --git a/trytond/model/fields/multiselection.py b/trytond/model/fields/multiselection.py index 060b6e57a4..5c2d18e49f 100644 --- a/trytond/model/fields/multiselection.py +++ b/trytond/model/fields/multiselection.py @@ -61,7 +61,7 @@ class MultiSelection(SelectionMixin, Field): # If stored as JSON conversion is done on backend if isinstance(data, str): data = json.loads(data) - lists[value['id']] = data + lists[value['id']] = tuple(data) return lists def sql_format(self, value): @@ -70,6 +70,11 @@ class MultiSelection(SelectionMixin, Field): value = dumps(sorted(set(value))) return value + def __set__(self, inst, value): + if value: + value = tuple(value) + super().__set__(inst, value) + def _domain_column(self, operator, column): database = Transaction().database return database.json_get(super()._domain_column(operator, column)) diff --git a/trytond/tests/modelview.py b/trytond/tests/modelview.py index b9c1ce13bf..b583bf3c4e 100644 --- a/trytond/tests/modelview.py +++ b/trytond/tests/modelview.py @@ -1,7 +1,7 @@ # This file is part of Tryton. The COPYRIGHT file at the top level of # this repository contains the full copyright notices and license terms. -from trytond.model import ModelView, ModelSQL, fields +from trytond.model import ModelView, ModelSQL, DictSchemaMixin, fields from trytond.pool import Pool from trytond.pyson import If, Eval @@ -20,6 +20,16 @@ class ModelViewChangedValues(ModelView): 'Targets') m2m_targets = fields.Many2Many('test.modelview.changed_values.target', None, None, 'Targets') + multiselection = fields.MultiSelection([ + ('a', 'A'), ('b', 'B'), + ], "MultiSelection") + dictionary = fields.Dict( + 'test.modelview.changed_values.dictionary', "Dictionary") + + +class ModelViewChangedValuesDictSchema(DictSchemaMixin, ModelSQL): + 'ModelView Changed Values Dict Schema' + __name__ = 'test.modelview.changed_values.dictionary' class ModelViewChangedValuesTarget(ModelView): @@ -253,6 +263,7 @@ class ModelViewViewAttributesDepends(ModelView): def register(module): Pool.register( ModelViewChangedValues, + ModelViewChangedValuesDictSchema, ModelViewChangedValuesTarget, ModelViewChangedValuesStoredTarget, ModelViewStoredChangedValues, diff --git a/trytond/tests/test_field_dict.py b/trytond/tests/test_field_dict.py index 5a98b65fd8..280c50ccf5 100644 --- a/trytond/tests/test_field_dict.py +++ b/trytond/tests/test_field_dict.py @@ -886,6 +886,19 @@ class FieldDictTestCase(unittest.TestCase): self.assertDictEqual( dict_.dico_string_keys, {'a': 'A', 'type': "Type"}) + @with_transaction + def test_set_key(self): + "Test setting a key of dict" + Dict = Pool().get('test.dict') + self.create_schema() + + dict_, = Dict.create([{ + 'dico': {'a': 1, 'type': 'arabic'}, + }]) + + with self.assertRaises(TypeError): + dict.dico['a'] = 5 + @unittest.skipUnless(backend.name == 'postgresql', "unaccent works only on postgresql") diff --git a/trytond/tests/test_field_multiselection.py b/trytond/tests/test_field_multiselection.py index dbd7cad224..9db539a832 100644 --- a/trytond/tests/test_field_multiselection.py +++ b/trytond/tests/test_field_multiselection.py @@ -28,7 +28,7 @@ class FieldMultiSelectionTestCase(unittest.TestCase): 'selects': None, }]) - self.assertEqual(selection.selects, ['bar', 'foo']) + self.assertEqual(selection.selects, ('bar', 'foo')) self.assertEqual(selection_none.selects, None) @with_transaction() @@ -38,7 +38,7 @@ class FieldMultiSelectionTestCase(unittest.TestCase): with self.assertRaises(SelectionValidationError): Selection.create([{ - 'selects': ['invalid'], + 'selects': ('invalid'), }]) @with_transaction() @@ -54,8 +54,8 @@ class FieldMultiSelectionTestCase(unittest.TestCase): 'dyn_selects': ['baz'], }]) - self.assertEqual(selection_foo.dyn_selects, ['foo']) - self.assertEqual(selection_bar.dyn_selects, ['baz']) + self.assertEqual(selection_foo.dyn_selects, ('foo',)) + self.assertEqual(selection_bar.dyn_selects, ('baz',)) @with_transaction() def test_create_dynamic_none(self): @@ -89,7 +89,7 @@ class FieldMultiSelectionTestCase(unittest.TestCase): 'static_selects': ['foo', 'bar'], }]) - self.assertEqual(selection.static_selects, ['bar', 'foo']) + self.assertEqual(selection.static_selects, ('bar', 'foo')) @with_transaction() def test_create_static_none(self): @@ -121,7 +121,7 @@ class FieldMultiSelectionTestCase(unittest.TestCase): 'selects': ['foo', 'bar'], }]) - self.assertEqual(selection.selects, ['bar', 'foo']) + self.assertEqual(selection.selects, ('bar', 'foo')) @with_transaction() def test_create_required_without_value(self): @@ -163,7 +163,7 @@ class FieldMultiSelectionTestCase(unittest.TestCase): 'selects': ['foo', 'bar'], }) - self.assertEqual(selection.selects, ['bar', 'foo']) + self.assertEqual(selection.selects, ('bar', 'foo')) @with_transaction() def test_string(self): diff --git a/trytond/tests/test_modelstorage.py b/trytond/tests/test_modelstorage.py index 6d04e0969d..c22b043eed 100644 --- a/trytond/tests/test_modelstorage.py +++ b/trytond/tests/test_modelstorage.py @@ -638,7 +638,7 @@ class EvalEnvironmentTestCase(unittest.TestCase): record = Model(multiselection=['value1', 'value2']) env = EvalEnvironment(record, Model) - self.assertEqual(env.get('multiselection'), ['value1', 'value2']) + self.assertEqual(env.get('multiselection'), ('value1', 'value2')) @with_transaction() def test_parent_field(self): diff --git a/trytond/tests/test_modelview.py b/trytond/tests/test_modelview.py index a327f14a5c..53483555b8 100644 --- a/trytond/tests/test_modelview.py +++ b/trytond/tests/test_modelview.py @@ -34,6 +34,8 @@ class ModelView(unittest.TestCase): record.target = Target(1) record.ref_target = Target(2) record.targets = [Target(name='bar')] + record.multiselection = ['a'] + record.dictionary = {'key': 'value'} self.assertEqual(record._changed_values, { 'name': 'foo', 'target': 1, @@ -43,12 +45,18 @@ class ModelView(unittest.TestCase): (0, {'id': None, 'name': 'bar'}), ], }, + 'multiselection': ('a',), + 'dictionary': {'key': 'value'}, }) record = Model(name='test', target=1, targets=[ {'id': 1, 'name': 'foo'}, {'id': 2}, - ], m2m_targets=[5, 6, 7]) + ], + m2m_targets=[5, 6, 7], + multiselection=['a'], + dictionary={'key': 'value'}, + ) self.assertEqual(record._changed_values, {}) @@ -56,6 +64,12 @@ class ModelView(unittest.TestCase): target.name = 'bar' record.targets = [target] record.m2m_targets = [Target(9), Target(10)] + ms = list(record.multiselection) + ms.append('b') + record.multiselection = ms + dico = record.dictionary.copy() + dico['key'] = 'another value' + record.dictionary = dico self.assertEqual(record._changed_values, { 'targets': { 'update': [{'id': 1, 'name': 'bar'}], @@ -65,6 +79,8 @@ class ModelView(unittest.TestCase): 'remove': [5, 6, 7], 'add': [(0, {'id': 9}), (1, {'id': 10})], }, + 'multiselection': ('a', 'b'), + 'dictionary': {'key': 'another value'}, }) # change only one2many record