Repository: canonical/operator
Pull request #1147 (2024-03-12 06:09:00)

fix: inspect the correct signature when validating observe arguments
When validating the `observe` arguments, inspect the signature of the method that we'll call, rather than digging down into any wrapped version, so that wrappers can adjust the signature as required, as long as the framework can still call them with the expected arguments.

This also fixes a bug where the check would fail to properly identify `*` and `**` as not required, so a signature like `def _on_x(self, event, *args, **kwargs)` would fail, even though the framework can use it to emit events.

Also remove the code that checks for `framework.observe(self.on.x, self)` style calls, which were possible in some old version of ops, and raises a specific error just for that (the general error is clear enough, and we don't believe anyone is still doing this).

When checking the type of the event handler, raise `TypeError` for errors, rather than a mixture of `RuntimeError` and `TypeError`.

Fixes #1129

tonyandrewmeyer: > Hmm, for some weird reason we're getting test failures on the observability charms (and not the issue we found yesterday). I think it's actually caused by this change, but I'm not sure why. Is it because the signature uses `_` for the event parameters? I didn't think `_` was special in Python though.
> 
> From https://github.com/canonical/operator/actions/runs/8270750559/job/22628988637?pr=1147
> 
> ```
> Traceback (most recent call last):
>   File "/usr/lib/python3.10/unittest/case.py", line 59, in testPartExecutor
>     yield
>   File "/usr/lib/python3.10/unittest/case.py", line 587, in run
>     self._callSetUp()
>   File "/usr/lib/python3.10/unittest/case.py", line 546, in _callSetUp
>     self.setUp()
>   File "/usr/lib/python3.10/unittest/mock.py", line 1379, in patched
>     return func(*newargs, **newkeywargs)
>   File "/home/runner/work/operator/operator/tests/unit/test_charm.py", line 166, in setUp
>     self.harness.begin()
>   File "/home/runner/work/operator/operator/.tox/unit/lib/python3.10/site-packages/ops/testing.py", line 366, in begin
>     self._charm = TestCharm(self._framework)  # type: ignore
>   File "/home/runner/work/operator/operator/lib/charms/tempo_k8s/v1/charm_tracing.py", line 287, in wrap_init
>     original_init(self, framework, *args, **kwargs)
>   File "/home/runner/work/operator/operator/src/charm.py", line 101, in __init__
>     self.server_cert = CertHandler(
>   File "/home/runner/work/operator/operator/lib/charms/observability_libs/v1/cert_handler.py", line 133, in __init__
>     self.framework.observe(
>   File "/home/runner/work/operator/operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 819, in observe
>     raise TypeError(
> TypeError: CertHandler._on_config_changed has extra required parameter
> ```

Ah, nice catch - thanks. I was (badly) assuming that the failure was related to the issue found yesterday and so hadn't looked into it. I'll investigate.
tonyandrewmeyer: > Ah, nice catch - thanks. I was (badly) assuming that the failure was related to the issue found yesterday and so hadn't looked into it. I'll investigate.

The reason for this is that with the change, we are looking at the correct signature, but we weren't correctly evaluating the signature all along.

With ops 2.11, this charm:

```python
class MyCharm(ops.CharmBase):
    def __init__(self, framework):
        super().__init__(framework)
        framework.observe(self.on.install, self._on_install)

    def _on_install(self, *args, **kwargs):
        # args will be (event, ), kwargs will be {} - that's fine!
        pass
```

fails with:

```
unit-badsigcheck-0: 17:43:20 ERROR unit.badsigcheck/0.juju-log Uncaught exception while in charm code:
Traceback (most recent call last):
  File "/var/lib/juju/agents/unit-badsigcheck-0/charm/./src/charm.py", line 15, in <module>
    ops.main(MyCharm)
  File "/var/lib/juju/agents/unit-badsigcheck-0/charm/venv/ops/main.py", line 471, in __call__
    return main(charm_class, use_juju_for_storage=use_juju_for_storage)
  File "/var/lib/juju/agents/unit-badsigcheck-0/charm/venv/ops/main.py", line 444, in main
    charm = charm_class(framework)
  File "/var/lib/juju/agents/unit-badsigcheck-0/charm/./src/charm.py", line 8, in __init__
    framework.observe(self.on.install, self._on_install)
  File "/var/lib/juju/agents/unit-badsigcheck-0/charm/venv/ops/framework.py", line 826, in observe
    raise TypeError(
TypeError: MyCharm._on_install has extra required parameter
```

What the existing code did was look at all the arguments (other than event, which is already examined) and check if they have a default value - if they did, then they weren't required, so were ok. However, `*args` and `*kwargs` don't have default values, but aren't required either, because they're of variable length, and that length can be zero. So we should skip those as well.

I've run this (updated) branch against 144 charms and there are no new failures compared to ops main, so I'm confident we should be good now.

@benhoyt suggested that `inspect` might have something to do the work for us, and there is the `bind()` method on the signature. I think that will be more readable (and leaves the work of getting it all correct to the core Python devs!), so am switching to that and am adding some additional tests for different types of argument.
benhoyt: Thanks for checking with super-tox too. I'm going to go ahead and merge this.

Patch:
diff --git a/ops/framework.py b/ops/framework.py
index 645abdc..07294ed 100644
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -782,18 +782,11 @@ class Framework(Object):
             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 Framework(Object):
         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 patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index 7ebbce2..1a9ac52 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -89,7 +89,7 @@ class TestCharm(unittest.TestCase):
 
         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):
diff --git a/test/test_framework.py b/test/test_framework.py
index 7a2a778..bea8745 100644
--- a/test/test_framework.py
+++ b/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 @@ class TestFramework(BaseTestCase):
         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 @@ class TestFramework(BaseTestCase):
             "<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 @@ class TestFramework(BaseTestCase):
         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 @@ class TestFramework(BaseTestCase):
                 '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.