Repository: canonical/operator
Pull request #53 (2019-11-25 15:48:14)

Implement dirty flag for StoredStateData
The Framework needs to trigger save_snapshot for StoredStateData only
when the state has actually changed.

The change also extends testing of nested data types to make sure
save_snapshot is called after changes to nested data types.


Patch:
diff --git a/op/framework.py b/op/framework.py
index 08d9527..c39cf45 100644
--- a/op/framework.py
+++ b/op/framework.py
@@ -563,12 +563,14 @@ class StoredStateData(Object):
     def __init__(self, parent, attr_name):
         super().__init__(parent, attr_name)
         self._cache = {}
+        self.dirty = False
 
     def __getitem__(self, key):
         return self._cache.get(key)
 
     def __setitem__(self, key, value):
         self._cache[key] = value
+        self.dirty = True
 
     def __contains__(self, key):
         return key in self._cache
@@ -578,9 +580,12 @@ class StoredStateData(Object):
 
     def restore(self, snapshot):
         self._cache = snapshot
+        self.dirty = False
 
     def on_commit(self, event):
-        self.framework.save_snapshot(self)
+        if self.dirty:
+            self.framework.save_snapshot(self)
+            self.dirty = False
 
 class BoundStoredState:
 
@@ -683,9 +688,11 @@ class StoredDict(collections.abc.MutableMapping):
 
     def __setitem__(self, key, value):
         self._under[key] = _unwrap_stored(self._stored_data, value)
+        self._stored_data.dirty = True
 
     def __delitem__(self, key):
         del self._under[key]
+        self._stored_data.dirty = True
 
     def __iter__(self):
         return self._under.__iter__()
@@ -713,18 +720,22 @@ class StoredList(collections.abc.MutableSequence):
 
     def __setitem__(self, index, value):
         self._under[index] = _unwrap_stored(self._stored_data, value)
+        self._stored_data.dirty = True
 
     def __delitem__(self, index):
         del self._under[index]
+        self._stored_data.dirty = True
 
     def __len__(self):
         return len(self._under)
 
     def insert(self, index, value):
         self._under.insert(index, value)
+        self._stored_data.dirty = True
 
     def append(self, value):
         self._under.append(value)
+        self._stored_data.dirty = True
 
     def __eq__(self, other):
         if isinstance(other, StoredList):
@@ -775,9 +786,11 @@ class StoredSet(collections.abc.MutableSet):
 
     def add(self, key):
         self._under.add(key)
+        self._stored_data.dirty = True
 
     def discard(self, key):
         self._under.discard(key)
+        self._stored_data.dirty = True
 
     def __contains__(self, key):
         return key in self._under


Test patch:
diff --git a/test/test_framework.py b/test/test_framework.py
index e72d514..5fd59ec 100755
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -6,8 +6,10 @@ import shutil
 
 from pathlib import Path
 
-from op.framework import Framework, Handle, Event, EventsBase, EventBase, Object, PreCommitEvent, CommitEvent
-from op.framework import NoSnapshotError, StoredState, StoredList
+from op.framework import (
+    Framework, Handle, Event, EventsBase, EventBase, Object, PreCommitEvent, CommitEvent,
+    NoSnapshotError, StoredState, StoredList, BoundStoredState, StoredStateData
+)
 
 
 class TestFramework(unittest.TestCase):
@@ -547,7 +549,7 @@ class TestStoredState(unittest.TestCase):
         self.assertEqual(obj_copy.state.foo, 42)
         self.assertEqual(obj_copy.state.bar, "s")
 
-    def test_mutable_types(self):
+    def test_mutable_types_invalid(self):
         framework = self.create_framework()
 
         class SomeObject(Object):
@@ -565,52 +567,65 @@ class TestStoredState(unittest.TestCase):
 
         framework.commit()
 
+    def test_mutable_types(self):
         # Test and validation functions in a list of 2-tuples.
         # Assignment and keywords like del are not supported in lambdas so functions are used instead.
         test_operations = [(
-            {},                                                            # Operand A.
-            None,                                                          # Operand B.
-            {},                                                            # Expected result.
-            lambda a, b: None,                                             # Operation to perform.
+            lambda: {},         # Operand A.
+            None,               # Operand B.
+            {},                 # Expected result.
+            lambda a, b: None,  # Operation to perform.
             lambda res, expected_res: self.assertEqual(res, expected_res)  # Validation to perform.
         ), (
-            {},
+            lambda: {},
             {'a': {}},
             {'a': {}},
             lambda a, b: a.update(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            {'a': {}},
+            lambda: {'a': {}},
             {'b': 'c'},
             {'a': {'b': 'c'}},
             lambda a, b: a['a'].update(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            {'a': {'b': 'c'}},
+            lambda: {'a': {'b': 'c'}},
             {'d': 'e'},
             {'a': {'b': 'c', 'd': 'e'}},
             lambda a, b: a['a'].update(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            {'a': {'b': 'c', 'd': 'e'}},
+            lambda: {'a': {'b': 'c', 'd': 'e'}},
             'd',
             {'a': {'b': 'c'}},
             lambda a, b: a['a'].pop(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            [],
+            lambda: {'s': set()},
+            'a',
+            {'s': {'a'}},
+            lambda a, b: a['s'].add(b),
+            lambda res, expected_res: self.assertEqual(res, expected_res)
+        ), (
+            lambda: {'s': {'a'}},
+            'a',
+            {'s': set()},
+            lambda a, b: a['s'].discard(b),
+            lambda res, expected_res: self.assertEqual(res, expected_res)
+        ), (
+            lambda: [],
             None,
             [],
             lambda a, b: None,
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            [],
+            lambda: [],
             'a',
             ['a'],
             lambda a, b: a.append(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            ['a'],
+            lambda: ['a'],
             ['c'],
             ['a', ['c']],
             lambda a, b: a.append(b),
@@ -619,13 +634,13 @@ class TestStoredState(unittest.TestCase):
                 self.assertIsInstance(res[1], StoredList),
             )
         ), (
-            ['a', ['c']],
+            lambda: ['a', ['c']],
             'b',
             ['b', 'a', ['c']],
             lambda a, b: a.insert(0, b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            ['b', 'a', ['c']],
+            lambda: ['b', 'a', ['c']],
             ['d'],
             ['b', ['d'], 'a', ['c']],
             lambda a, b: a.insert(1, b),
@@ -634,37 +649,71 @@ class TestStoredState(unittest.TestCase):
                 self.assertIsInstance(res[1], StoredList)
             ),
         ), (
-            ['b', ['d'], 'a', ['c']],
+            lambda: ['b', 'a', ['c']],
+            ['d'],
+            ['b', ['d'], ['c']],
+            # a[1] = b
+            lambda a, b: a.__setitem__(1, b),
+            lambda res, expected_res: (
+                self.assertEqual(res, expected_res),
+                self.assertIsInstance(res[1], StoredList)
+            ),
+        ), (
+            lambda: ['b', ['d'], 'a', ['c']],
             0,
             [['d'], 'a', ['c']],
             lambda a, b: a.pop(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            [['d'], 'a', ['c']],
+            lambda: [['d'], 'a', ['c']],
             ['d'],
             ['a', ['c']],
             lambda a, b: a.remove(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            set(),
+            lambda: ['a', ['c']],
+            'd',
+            ['a', ['c', 'd']],
+            lambda a, b: a[1].append(b),
+            lambda res, expected_res: self.assertEqual(res, expected_res)
+        ), (
+            lambda: ['a', ['c', 'd']],
+            1,
+            ['a', ['c']],
+            lambda a, b: a[1].pop(b),
+            lambda res, expected_res: self.assertEqual(res, expected_res)
+        ), (
+            lambda: ['a', ['c']],
+            'd',
+            ['a', ['c', 'd']],
+            lambda a, b: a[1].insert(1, b),
+            lambda res, expected_res: self.assertEqual(res, expected_res)
+        ), (
+            lambda: ['a', ['c', 'd']],
+            'd',
+            ['a', ['c']],
+            lambda a, b: a[1].remove(b),
+            lambda res, expected_res: self.assertEqual(res, expected_res)
+        ), (
+            lambda: set(),
             None,
             set(),
             lambda a, b: None,
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            set(),
+            lambda: set(),
             'a',
             set(['a']),
             lambda a, b: a.add(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            set(['a']),
+            lambda: set(['a']),
             'a',
             set(),
             lambda a, b: a.discard(b),
             lambda res, expected_res: self.assertEqual(res, expected_res)
         ), (
-            set(),
+            lambda: set(),
             {'a'},
             set(),
             # Nested sets are not allowed as sets themselves are not hashable.
@@ -675,24 +724,48 @@ class TestStoredState(unittest.TestCase):
         class SomeObject(Object):
             state = StoredState()
 
-        for a, b, expected_res, op, validate_op in test_operations:
+        # Validate correctness of modification operations.
+        for get_a, b, expected_res, op, validate_op in test_operations:
             framework = self.create_framework()
             obj = SomeObject(framework, '1')
-            obj.state.a = a
+
+            obj.state.a = get_a()
+            self.assertTrue(isinstance(obj.state, BoundStoredState))
 
             op(obj.state.a, b)
             validate_op(obj.state.a, expected_res)
 
+            obj.state.a = get_a()
+            framework.commit()
+
+            obj_copy1 = SomeObject(framework, '1')
+            self.assertEqual(obj_copy1.state.a, get_a())
+
+            op(obj_copy1.state.a, b)
+            validate_op(obj_copy1.state.a, expected_res)
+
             framework.commit()
 
             framework_copy = self.create_framework()
 
-            obj = SomeObject(framework_copy, '1')
+            obj_copy2 = SomeObject(framework_copy, '1')
 
-            validate_op(obj.state.a, expected_res)
+            validate_op(obj_copy2.state.a, expected_res)
 
+            # Validate the dirty state functionality.
+            # obj_copy2 state is not dirty because it was not modified in any supported way since the last commit so
+            # it still contains the old value at this point. State is overridden here via save_snapshot to validate that
+            # the modification will not be saved when StoredStateData is not dirty. This check assumes that the artificially
+            # created StoredStateData does not observe the on_commit event.
+            framework_copy.save_snapshot(StoredStateData(obj_copy2, 'state'))
             framework_copy.commit()
 
+            obj_copy3 = SomeObject(framework_copy, '1')
+
+            # Now make sure that the modification was not saved as the state holding it was not dirty.
+            with self.assertRaises(AttributeError):
+                obj_copy3.state.a
+
     def test_comparison_operations(self):
         test_operations = [(
             {"1"},               # Operand A.