Merge branch 'main' into add-storage-clearer-use-1127

ops/framework.py

@@ -782,18 +782,11 @@ class SomeObject:
             RuntimeError: if bound_event or observer are the wrong type.
         """
         if not isinstance(bound_event, BoundEvent):
-            raise RuntimeError(
+            raise TypeError(
                 f'Framework.observe requires a BoundEvent as second parameter, got {bound_event}')
         if not isinstance(observer, types.MethodType):
-            # help users of older versions of the framework
-            if isinstance(observer, charm.CharmBase):
-                raise TypeError(
-                    'observer methods must now be explicitly provided;'
-                    ' please replace observe(self.on.{0}, self)'
-                    ' with e.g. observe(self.on.{0}, self._on_{0})'.format(
-                        bound_event.event_kind))
-            raise RuntimeError(
-                f'Framework.observe requires a method as third parameter, got {observer}')
+            raise TypeError(
+                f"Framework.observe requires a method as the 'observer' parameter, got {observer}")
 
         event_type = bound_event.event_type
         event_kind = bound_event.event_kind
@@ -804,27 +797,23 @@ class SomeObject:
         if hasattr(emitter, "handle"):
             emitter_path = emitter.handle.path
         else:
-            raise RuntimeError(
+            raise TypeError(
                 f'event emitter {type(emitter).__name__} must have a "handle" attribute')
 
-        # Validate that the method has an acceptable call signature.
-        sig = inspect.signature(observer)
-        # Self isn't included in the params list, so the first arg will be the event.
-        extra_params = list(sig.parameters.values())[1:]
-
         method_name = observer.__name__
 
         assert isinstance(observer.__self__, Object), "can't register observers " \
                                                       "that aren't `Object`s"
         observer_obj = observer.__self__
-        if not sig.parameters:
-            raise TypeError(
-                f'{type(observer_obj).__name__}.{method_name} must accept event parameter')
-        elif any(param.default is inspect.Parameter.empty for param in extra_params):
-            # Allow for additional optional params, since there's no reason to exclude them, but
-            # required params will break.
+
+        # Validate that the method has an acceptable call signature.
+        sig = inspect.signature(observer, follow_wrapped=False)
+        try:
+            sig.bind(EventBase(None))  # type: ignore
+        except TypeError as e:
             raise TypeError(
-                f'{type(observer_obj).__name__}.{method_name} has extra required parameter')
+                f'{type(observer_obj).__name__}.{method_name} must be callable with '
+                "only 'self' and the 'event'") from e
 
         # TODO Prevent the exact same parameters from being registered more than once.
 

test/test_charm.py

@@ -89,7 +89,7 @@ def _on_start(self, event: ops.EventBase):
 
         self.assertEqual(charm.started, True)
 
-        with self.assertRaisesRegex(TypeError, "observer methods must now be explicitly provided"):
+        with self.assertRaises(TypeError):
             framework.observe(charm.on.start, charm)  # type: ignore
 
     def test_observe_decorated_method(self):

test/test_framework.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import datetime
+import functools
 import gc
 import inspect
 import io
@@ -166,7 +167,7 @@ def on_foo(self, event: ops.EventBase):
         framework.observe(pub.foo, obs.on_any)
         framework.observe(pub.bar, obs.on_any)
 
-        with self.assertRaisesRegex(RuntimeError, "^Framework.observe requires a method"):
+        with self.assertRaisesRegex(TypeError, "^Framework.observe requires a method"):
             framework.observe(pub.baz, obs)  # type: ignore
 
         pub.foo.emit()
@@ -178,6 +179,61 @@ def on_foo(self, event: ops.EventBase):
             "<MyEvent via MyNotifier[1]/bar[2]>",
         ])
 
+    def test_event_observer_more_args(self):
+        framework = self.create_framework()
+
+        class MyEvent(ops.EventBase):
+            pass
+
+        class MyNotifier(ops.Object):
+            foo = ops.EventSource(MyEvent)
+            bar = ops.EventSource(MyEvent)
+            baz = ops.EventSource(MyEvent)
+            qux = ops.EventSource(MyEvent)
+
+        class MyObserver(ops.Object):
+            def __init__(self, parent: ops.Object, key: str):
+                super().__init__(parent, key)
+                self.seen: typing.List[str] = []
+                self.reprs: typing.List[str] = []
+
+            def on_foo(self, event: ops.EventBase):
+                self.seen.append(f"on_foo:{event.handle.kind}")
+                self.reprs.append(repr(event))
+
+            def on_bar(self, event: ops.EventBase, _: int = 1):
+                self.seen.append(f"on_bar:{event.handle.kind}")
+                self.reprs.append(repr(event))
+
+            def on_baz(self, event: ops.EventBase, *, _: int = 1):
+                self.seen.append(f"on_baz:{event.handle.kind}")
+                self.reprs.append(repr(event))
+
+            def on_qux(self, event: ops.EventBase, *args, **kwargs):  # type: ignore
+                self.seen.append(f"on_qux:{event.handle.kind}")
+                self.reprs.append(repr(event))
+
+        pub = MyNotifier(framework, "1")
+        obs = MyObserver(framework, "1")
+
+        framework.observe(pub.foo, obs.on_foo)
+        framework.observe(pub.bar, obs.on_bar)
+        framework.observe(pub.baz, obs.on_baz)
+        framework.observe(pub.qux, obs.on_qux)  # type: ignore
+
+        pub.foo.emit()
+        pub.bar.emit()
+        pub.baz.emit()
+        pub.qux.emit()
+
+        self.assertEqual(obs.seen, ['on_foo:foo', 'on_bar:bar', 'on_baz:baz', 'on_qux:qux'])
+        self.assertEqual(obs.reprs, [
+            "<MyEvent via MyNotifier[1]/foo[1]>",
+            "<MyEvent via MyNotifier[1]/bar[2]>",
+            "<MyEvent via MyNotifier[1]/baz[3]>",
+            "<MyEvent via MyNotifier[1]/qux[4]>",
+        ])
+
     def test_bad_sig_observer(self):
 
         class MyEvent(ops.EventBase):
@@ -210,11 +266,11 @@ def _on_qux(self, event: ops.EventBase, extra: typing.Optional[typing.Any] = Non
         pub = MyNotifier(framework, "pub")
         obs = MyObserver(framework, "obs")
 
-        with self.assertRaisesRegex(TypeError, "must accept event parameter"):
+        with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"):
             framework.observe(pub.foo, obs._on_foo)  # type: ignore
-        with self.assertRaisesRegex(TypeError, "has extra required parameter"):
+        with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"):
             framework.observe(pub.bar, obs._on_bar)  # type: ignore
-        with self.assertRaisesRegex(TypeError, "has extra required parameter"):
+        with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"):
             framework.observe(pub.baz, obs._on_baz)  # type: ignore
         framework.observe(pub.qux, obs._on_qux)
 
@@ -886,6 +942,46 @@ def _on_event(self, event: ops.EventBase):
                 'ObjectWithStorage[obj]/StoredStateData[_stored]',
                 'ObjectWithStorage[obj]/on/event[1]']))
 
+    def test_wrapped_handler(self):
+        # It's fine to wrap the event handler, as long as the framework can
+        # still call it with just the `event` argument.
+        def add_arg(func: typing.Callable[..., None]) -> typing.Callable[..., None]:
+            @functools.wraps(func)
+            def wrapper(charm: ops.CharmBase, event: ops.EventBase):
+                return func(charm, event, "extra-arg")
+
+            return wrapper
+
+        class MyCharm(ops.CharmBase):
+            @add_arg
+            def _on_event(self, _, another_arg: str):
+                assert another_arg == "extra-arg"
+
+        framework = self.create_framework()
+        charm = MyCharm(framework)
+        framework.observe(charm.on.start, charm._on_event)
+        charm.on.start.emit()
+
+        # If the event handler is wrapped, and then needs an additional argument
+        # that's the same problem as if we didn't wrap it and required an extra
+        # argument, so check that also correctly fails.
+        def bad_arg(func: typing.Callable[..., None]) -> typing.Callable[..., None]:
+            @functools.wraps(func)
+            def wrapper(charm: ops.CharmBase, event: ops.EventBase, _: str):
+                return func(charm, event)
+
+            return wrapper
+
+        class BadCharm(ops.CharmBase):
+            @bad_arg
+            def _on_event(self, _: ops.EventBase):
+                assert False, 'should not get to here'
+
+        framework = self.create_framework()
+        charm = BadCharm(framework)
+        with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"):
+            framework.observe(charm.on.start, charm._on_event)
+
 
 MutableTypesTestCase = typing.Tuple[
     typing.Callable[[], typing.Any],  # Called to get operand A.