Repository: canonical/operator
Pull request #92 (2019-12-16 13:46:11)

Object singularity
Ensure that `Object.__init__` doesn't ever create 2 objects that have the same handle path.
This will raise a RuntimeError if 2 objects are created that have the same path. It also won't let you create an Object that matches one that has been created by load_snapshot(), and load_snapshot will also fail if the same object gets loaded 2 times.
The test suite has been updated to use _forget where appropriate. And the Event _reemit also uses _forget. But that seems to be the only cases where we need to use it.


Patch:
diff --git a/ops/charm.py b/ops/charm.py
index 2ec6a23..1c172da 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -1,4 +1,4 @@
-from ops.framework import Object, Event, EventBase, EventsBase
+from ops.framework import Object, EventSource, EventBase, EventsBase
 
 
 class HookEvent(EventBase):
@@ -113,16 +113,16 @@ class StorageDetachingEvent(StorageEvent):
 
 class CharmEvents(EventsBase):
 
-    install = Event(InstallEvent)
-    start = Event(StartEvent)
-    stop = Event(StopEvent)
-    update_status = Event(UpdateStatusEvent)
-    config_changed = Event(ConfigChangedEvent)
-    upgrade_charm = Event(UpgradeCharmEvent)
-    pre_series_upgrade = Event(PreSeriesUpgradeEvent)
-    post_series_upgrade = Event(PostSeriesUpgradeEvent)
-    leader_elected = Event(LeaderElectedEvent)
-    leader_settings_changed = Event(LeaderSettingsChangedEvent)
+    install = EventSource(InstallEvent)
+    start = EventSource(StartEvent)
+    stop = EventSource(StopEvent)
+    update_status = EventSource(UpdateStatusEvent)
+    config_changed = EventSource(ConfigChangedEvent)
+    upgrade_charm = EventSource(UpgradeCharmEvent)
+    pre_series_upgrade = EventSource(PreSeriesUpgradeEvent)
+    post_series_upgrade = EventSource(PostSeriesUpgradeEvent)
+    leader_elected = EventSource(LeaderElectedEvent)
+    leader_settings_changed = EventSource(LeaderSettingsChangedEvent)
 
 
 class CharmBase(Object):
@@ -229,7 +229,7 @@ class ResourceMeta:
     def __init__(self, name, raw):
         self.resource_name = name
         self.type = raw['type']
-        self.filename = raw['filename']
+        self.filename = raw.get('filename', None)
         self.description = raw.get('description', '')
 
 
diff --git a/ops/framework.py b/ops/framework.py
index eef6961..9c27bf1 100755
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -112,17 +112,16 @@ class EventBase:
         self.deferred = False
 
 
-class Event:
-    """Event creates class descriptors to operate with events.
+class EventSource:
+    """EventSource wraps an event type with a descriptor to facilitate observing and emitting.
 
     It is generally used as:
 
         class SomethingHappened(EventBase):
             pass
 
-        class SomeObject:
-            something_happened = Event(SomethingHappened)
-
+        class SomeObject(Object):
+            something_happened = EventSource(SomethingHappened)
 
     With that, instances of that type will offer the someobj.something_happened
     attribute which is a BoundEvent and may be used to emit and observe the event.
@@ -138,7 +137,7 @@ class Event:
     def __set_name__(self, emitter_type, event_kind):
         if self.event_kind is not None:
             raise RuntimeError(
-                f'Event({self.event_type.__name__}) reused as '
+                f'EventSource({self.event_type.__name__}) reused as '
                 f'{self.emitter_type.__name__}.{self.event_kind} and '
                 f'{emitter_type.__name__}.{event_kind}')
         self.event_kind = event_kind
@@ -199,12 +198,13 @@ class Object:
         else:
             self.framework = parent.framework
             self.handle = Handle(parent, kind, key)
+        self.framework._track(self)
 
         # TODO This can probably be dropped, because the event type is only
         # really relevant if someone is either emitting the event or observing
         # it.
         for attr_name, attr_value in inspect.getmembers(type(self)):
-            if isinstance(attr_value, Event):
+            if isinstance(attr_value, EventSource):
                 event_type = attr_value.event_type
                 event_kind = attr_name
                 emitter = self
@@ -247,7 +247,7 @@ class EventsBase(Object):
         except AttributeError:
             pass
 
-        event_descriptor = Event(event_type)
+        event_descriptor = EventSource(event_type)
         event_descriptor.__set_name__(cls, event_kind)
         setattr(cls, event_kind, event_descriptor)
 
@@ -258,7 +258,7 @@ class EventsBase(Object):
         # We have to iterate over the class rather than instance to allow for properties which
         # might call this method (e.g., event views), leading to infinite recursion.
         for attr_name, attr_value in inspect.getmembers(type(self)):
-            if isinstance(attr_value, Event):
+            if isinstance(attr_value, EventSource):
                 # We actually care about the bound_event, however, since it
                 # provides the most info for users of this method.
                 event_kind = attr_name
@@ -289,8 +289,8 @@ class CommitEvent(EventBase):
 
 
 class FrameworkEvents(EventsBase):
-    pre_commit = Event(PreCommitEvent)
-    commit = Event(CommitEvent)
+    pre_commit = EventSource(PreCommitEvent)
+    commit = EventSource(CommitEvent)
 
 
 class NoSnapshotError(Exception):
@@ -386,6 +386,7 @@ class Framework(Object):
         self.model = model
         self._observers = []      # [(observer_path, method_name, parent_path, event_key)]
         self._observer = weakref.WeakValueDictionary()       # {observer_path: observer}
+        self._objects = weakref.WeakValueDictionary()
         self._type_registry = {}  # {(parent_path, kind): cls}
         self._type_known = set()  # {cls}
 
@@ -393,15 +394,29 @@ class Framework(Object):
 
         # We can't use the higher-level StoredState because it relies on events.
         self.register_type(StoredStateData, None, StoredStateData.handle_kind)
-        self._stored = StoredStateData(self, '_stored')
+        stored_handle = Handle(None, StoredStateData.handle_kind, '_stored')
         try:
-            self._stored = self.load_snapshot(self._stored.handle)
+            self._stored = self.load_snapshot(stored_handle)
         except NoSnapshotError:
+            self._stored = StoredStateData(self, '_stored')
             self._stored['event_count'] = 0
 
     def close(self):
         self._storage.close()
 
+    def _track(self, obj):
+        """Track object and ensure it is the only object created using its handle path."""
+        if obj is self:
+            # Framework objects don't track themselves
+            return
+        if obj.handle.path in self.framework._objects:
+            raise RuntimeError(f"two objects claiming to be {obj.handle.path} have been created")
+        self._objects[obj.handle.path] = obj
+
+    def _forget(self, obj):
+        """Stop tracking the given object. See also _track."""
+        self._objects.pop(obj.handle.path, None)
+
     def commit(self):
         # Give a chance for objects to persist data they want to before a commit is made.
         self.on.pre_commit.emit()
@@ -457,6 +472,7 @@ class Framework(Object):
         obj.framework = self
         obj.handle = handle
         obj.restore(data)
+        self._track(obj)
         return obj
 
     def drop_snapshot(self, handle):
@@ -584,6 +600,8 @@ class Framework(Object):
                 deferred = True
             else:
                 self._storage.drop_notice(event_path, observer_path, method_name)
+            # We intentionally consider this event to be dead and reload it from scratch in the next path.
+            self.framework._forget(event)
 
         if not deferred:
             self._storage.drop_snapshot(last_event_path)
@@ -594,7 +612,7 @@ class StoredStateChanged(EventBase):
 
 
 class StoredStateEvents(EventsBase):
-    changed = Event(StoredStateChanged)
+    changed = EventSource(StoredStateChanged)
 
 
 class StoredStateData(Object):


Test patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index f558259..ec6506c 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -9,7 +9,7 @@ from pathlib import Path
 
 from ops.charm import CharmBase, CharmMeta
 from ops.charm import CharmEvents
-from ops.framework import Framework, Event, EventBase
+from ops.framework import Framework, EventSource, EventBase
 from ops.model import Model, ModelBackend
 
 
@@ -29,7 +29,7 @@ class TestCharm(unittest.TestCase):
             pass
 
         class TestCharmEvents(CharmEvents):
-            custom = Event(CustomEvent)
+            custom = EventSource(CustomEvent)
 
         # Relations events are defined dynamically and modify the class attributes.
         # We use a subclass temporarily to prevent these side effects from leaking.
diff --git a/test/test_framework.py b/test/test_framework.py
index 6d8f582..4fa38aa 100755
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -8,7 +8,7 @@ import gc
 from pathlib import Path
 
 from ops.framework import (
-    Framework, Handle, Event, EventsBase, EventBase, Object, PreCommitEvent, CommitEvent,
+    Framework, Handle, EventSource, EventsBase, EventBase, Object, PreCommitEvent, CommitEvent,
     NoSnapshotError, StoredState, StoredList, BoundStoredState, StoredStateData
 )
 
@@ -90,6 +90,8 @@ class TestFramework(unittest.TestCase):
         self.assertEqual(event2.my_n, 2)
 
         framework2.save_snapshot(event2)
+        del event2
+        gc.collect()
         event3 = framework2.load_snapshot(handle)
         self.assertEqual(event3.my_n, 3)
 
@@ -110,9 +112,9 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyNotifier(Object):
-            foo = Event(MyEvent)
-            bar = Event(MyEvent)
-            baz = Event(MyEvent)
+            foo = EventSource(MyEvent)
+            bar = EventSource(MyEvent)
+            baz = EventSource(MyEvent)
 
         class MyObserver(Object):
             def __init__(self, parent, key):
@@ -150,10 +152,10 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyNotifier(Object):
-            foo = Event(MyEvent)
-            bar = Event(MyEvent)
-            baz = Event(MyEvent)
-            qux = Event(MyEvent)
+            foo = EventSource(MyEvent)
+            bar = EventSource(MyEvent)
+            baz = EventSource(MyEvent)
+            qux = EventSource(MyEvent)
 
         class MyObserver(Object):
             def on_foo(self):
@@ -231,11 +233,11 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyNotifier1(Object):
-            a = Event(MyEvent)
-            b = Event(MyEvent)
+            a = EventSource(MyEvent)
+            b = EventSource(MyEvent)
 
         class MyNotifier2(Object):
-            c = Event(MyEvent)
+            c = EventSource(MyEvent)
 
         class MyObserver(Object):
             def __init__(self, parent, key):
@@ -264,9 +266,14 @@ class TestFramework(unittest.TestCase):
         pub2.c.emit()
 
         # Events remain stored because they were deferred.
-        ev_a = framework.load_snapshot(Handle(pub1, "a", "1"))
-        ev_b = framework.load_snapshot(Handle(pub1, "b", "2"))
-        ev_c = framework.load_snapshot(Handle(pub2, "c", "3"))
+        ev_a_handle = Handle(pub1, "a", "1")
+        framework.load_snapshot(ev_a_handle)
+        ev_b_handle = Handle(pub1, "b", "2")
+        framework.load_snapshot(ev_b_handle)
+        ev_c_handle = Handle(pub2, "c", "3")
+        framework.load_snapshot(ev_c_handle)
+        # make sure the objects are gone before we reemit them
+        gc.collect()
 
         framework.reemit()
         obs1.done["a"] = True
@@ -285,9 +292,9 @@ class TestFramework(unittest.TestCase):
         self.assertEqual(" ".join(obs2.seen), "a b c a b c a b c a c a c c")
 
         # Now the event objects must all be gone from storage.
-        self.assertRaises(NoSnapshotError, framework.load_snapshot, ev_a.handle)
-        self.assertRaises(NoSnapshotError, framework.load_snapshot, ev_b.handle)
-        self.assertRaises(NoSnapshotError, framework.load_snapshot, ev_c.handle)
+        self.assertRaises(NoSnapshotError, framework.load_snapshot, ev_a_handle)
+        self.assertRaises(NoSnapshotError, framework.load_snapshot, ev_b_handle)
+        self.assertRaises(NoSnapshotError, framework.load_snapshot, ev_c_handle)
 
     def test_custom_event_data(self):
         framework = self.create_framework()
@@ -305,7 +312,7 @@ class TestFramework(unittest.TestCase):
                 self.my_n = snapshot["My N!"] + 1
 
         class MyNotifier(Object):
-            foo = Event(MyEvent)
+            foo = EventSource(MyEvent)
 
         class MyObserver(Object):
             def __init__(self, parent, key):
@@ -346,7 +353,7 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyEvents(EventsBase):
-            foo = Event(MyEvent)
+            foo = EventSource(MyEvent)
 
         class MyNotifier(Object):
             on = MyEvents()
@@ -368,6 +375,71 @@ class TestFramework(unittest.TestCase):
         pub.on.foo.emit()
         self.assertEqual(observed_events, ["foo"])
 
+    def test_forget_and_multiple_objects(self):
+        framework = self.create_framework()
+
+        class MyObject(Object):
+            pass
+
+        o1 = MyObject(framework, "path")
+        # Creating a second object at the same path should fail with RuntimeError
+        with self.assertRaises(RuntimeError):
+            o2 = MyObject(framework, "path")
+        # Unless we _forget the object first
+        framework._forget(o1)
+        o2 = MyObject(framework, "path")
+        self.assertEqual(o1.handle.path, o2.handle.path)
+        # Deleting the tracked object should also work
+        del o2
+        gc.collect()
+        o3 = MyObject(framework, "path")
+        self.assertEqual(o1.handle.path, o3.handle.path)
+        # Or using a second framework
+        framework_copy = self.create_framework()
+        o_copy = MyObject(framework_copy, "path")
+        self.assertEqual(o1.handle.path, o_copy.handle.path)
+
+    def test_forget_and_multiple_objects_with_load_snapshot(self):
+        framework = self.create_framework()
+
+        class MyObject(Object):
+            def __init__(self, parent, name):
+                super().__init__(parent, name)
+                self.value = name
+
+            def snapshot(self):
+                return self.value
+
+            def restore(self, value):
+                self.value = value
+
+        framework.register_type(MyObject, None, MyObject.handle_kind)
+        o1 = MyObject(framework, "path")
+        framework.save_snapshot(o1)
+        framework.commit()
+        o_handle = o1.handle
+        del o1
+        gc.collect()
+        o2 = framework.load_snapshot(o_handle)
+        # Trying to load_snapshot a second object at the same path should fail with RuntimeError
+        with self.assertRaises(RuntimeError):
+            framework.load_snapshot(o_handle)
+        # Unless we _forget the object first
+        framework._forget(o2)
+        o3 = framework.load_snapshot(o_handle)
+        self.assertEqual(o2.value, o3.value)
+        # A loaded object also prevents direct creation of an object
+        with self.assertRaises(RuntimeError):
+            MyObject(framework, "path")
+        # But we can create an object, or load a snapshot in a copy of the framework
+        framework_copy1 = self.create_framework()
+        o_copy1 = MyObject(framework_copy1, "path")
+        self.assertEqual(o_copy1.value, "path")
+        framework_copy2 = self.create_framework()
+        framework_copy2.register_type(MyObject, None, MyObject.handle_kind)
+        o_copy2 = framework_copy2.load_snapshot(o_handle)
+        self.assertEqual(o_copy2.value, "path")
+
     def test_events_base(self):
         framework = self.create_framework()
 
@@ -375,7 +447,7 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyEvents(EventsBase):
-            foo = Event(MyEvent)
+            foo = EventSource(MyEvent)
 
         class MyNotifier(Object):
             on = MyEvents()
@@ -402,7 +474,7 @@ class TestFramework(unittest.TestCase):
         class MyEvent(EventBase):
             pass
 
-        event = Event(MyEvent)
+        event = EventSource(MyEvent)
 
         class MyEvents(EventsBase):
             foo = event
@@ -412,7 +484,7 @@ class TestFramework(unittest.TestCase):
                 foo = event
         self.assertEqual(
             str(cm.exception.__cause__),
-            "Event(MyEvent) reused as MyEvents.foo and OtherEvents.foo")
+            "EventSource(MyEvent) reused as MyEvents.foo and OtherEvents.foo")
 
         with self.assertRaises(RuntimeError) as cm:
             class MyNotifier(Object):
@@ -420,7 +492,7 @@ class TestFramework(unittest.TestCase):
                 bar = event
         self.assertEqual(
             str(cm.exception.__cause__),
-            "Event(MyEvent) reused as MyEvents.foo and MyNotifier.bar")
+            "EventSource(MyEvent) reused as MyEvents.foo and MyNotifier.bar")
 
     def test_reemit_ignores_unknown_event_type(self):
         # The event type may have been gone for good, and nobody cares,
@@ -432,7 +504,7 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyNotifier(Object):
-            foo = Event(MyEvent)
+            foo = EventSource(MyEvent)
 
         class MyObserver(Object):
             def __init__(self, parent, key):
@@ -474,11 +546,11 @@ class TestFramework(unittest.TestCase):
             pass
 
         class MyEvents(EventsBase):
-            foo = Event(MyFoo)
+            foo = EventSource(MyFoo)
 
         class MyNotifier(Object):
             on = MyEvents()
-            bar = Event(MyBar)
+            bar = EventSource(MyBar)
 
         class MyObserver(Object):
             def __init__(self, parent, key):
@@ -588,7 +660,7 @@ class TestFramework(unittest.TestCase):
                 self.value = value
 
         class MyNotifier(Object):
-            foo = Event(MyEvent)
+            foo = EventSource(MyEvent)
 
         class MyObserver(Object):
             has_deferred = False
@@ -635,8 +707,8 @@ class TestStoredState(unittest.TestCase):
         self.tmpdir = Path(tempfile.mkdtemp())
         self.addCleanup(shutil.rmtree, self.tmpdir)
 
-    def create_framework(self):
-        framework = Framework(self.tmpdir / "framework.data", self.tmpdir, None, None)
+    def create_framework(self, cls=Framework):
+        framework = cls(self.tmpdir / "framework.data", self.tmpdir, None, None)
         self.addCleanup(framework.close)
         return framework
 
@@ -856,9 +928,19 @@ class TestStoredState(unittest.TestCase):
         class SomeObject(Object):
             state = StoredState()
 
+        class WrappedFramework(Framework):
+            def __init__(self, data_path, charm_dir, meta, model):
+                super().__init__(data_path, charm_dir, meta, model)
+                self.snapshots = []
+
+            def save_snapshot(self, value):
+                if value.handle.path == 'SomeObject[1]/StoredStateData[state]':
+                    self.snapshots.append((type(value), value.snapshot()))
+                return super().save_snapshot(value)
+
         # Validate correctness of modification operations.
         for get_a, b, expected_res, op, validate_op in test_operations:
-            framework = self.create_framework()
+            framework = self.create_framework(cls=WrappedFramework)
             obj = SomeObject(framework, '1')
 
             obj.state.a = get_a()
@@ -869,34 +951,29 @@ class TestStoredState(unittest.TestCase):
 
             obj.state.a = get_a()
             framework.commit()
-
+            # We should see an update for initializing a
+            self.assertEqual(framework.snapshots, [
+                (StoredStateData, {'a': get_a()}),
+            ])
+            del obj
+            gc.collect()
             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()
+            framework_copy = self.create_framework(cls=WrappedFramework)
 
             obj_copy2 = SomeObject(framework_copy, '1')
 
             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'))
+            # Commit saves the pre-commit and commit events, and the framework event counter, but shouldn't update the stored state of my object
+            framework.snapshots.clear()
             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
+            self.assertEqual(framework_copy.snapshots, [])
 
     def test_comparison_operations(self):
         test_operations = [(
@@ -985,8 +1062,8 @@ class TestStoredState(unittest.TestCase):
 
         framework = self.create_framework()
 
-        for a, b, op, op_ab, op_ba in test_operations:
-            obj = SomeObject(framework, "1")
+        for i, (a, b, op, op_ab, op_ba) in enumerate(test_operations):
+            obj = SomeObject(framework, str(i))
             obj.state.a = a
             self.assertEqual(op(obj.state.a, b), op_ab)
             self.assertEqual(op(b, obj.state.a), op_ba)
@@ -1026,8 +1103,8 @@ class TestStoredState(unittest.TestCase):
 
         # Validate that operations between StoredSet and built-in sets only result in built-in sets being returned.
         # Make sure that commutativity is preserved and that the original sets are not changed or used as a result.
-        for variable_operand, operation, ab_res, ba_res in test_operations:
-            obj = SomeObject(framework, "2")
+        for i, (variable_operand, operation, ab_res, ba_res) in enumerate(test_operations):
+            obj = SomeObject(framework, str(i))
             obj.state.set = {"a", "b"}
 
             for a, b, expected in [(obj.state.set, variable_operand, ab_res), (variable_operand, obj.state.set, ba_res)]: