Repository: sanic-org/sanic
Pull request: Deprecate Conditions and Triggers Saved in `handler` Callable; Save Condition in `signal.extra` Instead
closes #2581

The change in last version of this PR is authored by @ahopkins. Thank you!

Problem statement:
A signal handler cannot be a method on an object
**Describe the bug**
<!-- A clear and concise description of what the bug is, make sure to paste any exceptions and tracebacks. -->
If I try and register a method on an object as a signal handler, an exception is thrown because it is not possible to set an attribute on a bound method
```
    self.add_signal(self.after_routing, 'http.routing.after')
  File "sanic/lib/python3.7/site-packages/sanic/mixins/signals.py", line 77, in add_signal
    handler
  File "sanic/lib/python3.7/site-packages/sanic/mixins/signals.py", line 57, in decorator
    self._apply_signal(future_signal)
  File "sanic/lib/python3.7/site-packages/sanic/app.py", line 420, in _apply_signal
    return self.signal_router.add(*signal)
  File "sanic/lib/python3.7/site-packages/sanic/signals.py", line 222, in add
    handler.__requirements__ = condition  # type: ignore
AttributeError: 'method' object has no attribute '__requirements__'
```


**Code snippet**
<!-- Relevant source code, make sure to remove what is not necessary. -->
```python
import sanic

class MyApp(sanic.Sanic):
  def __init__(self, *args, **kwargs):
    super().__init__(*args, **kwargs)
    self.add_signal(self.after_routing, 'http.routing.after')

  def after_routing(self, request, route, kwargs, handler):
    pass

app = MyApp(name='method_signal_handler')
```

**Expected behavior**
<!-- A clear and concise description of what you expected to happen. -->
I would expect the bound method to be registered, and then called when the signal is dispatched.

**Environment (please complete the following information):**
<!-- Please provide the information below. Instead, you can copy and paste the message that Sanic shows on startup. If you do, please remember to format it with ``` -->
 - OS: MacOS
 - Sanic Version: 22.9


**Additional context**
<!-- Add any other context about the problem here. -->

Hints from issue discussion:
Added code tags for readability.

Commit message: Save conditions and triggers in dicts

Patch to review:
diff --git a/sanic/signals.py b/sanic/signals.py
index 3b662684c0..b4a62c2147 100644
--- a/sanic/signals.py
+++ b/sanic/signals.py
@@ -86,6 +86,8 @@ def __init__(self) -> None:
         )
         self.allow_fail_builtin = True
         self.ctx.loop = None
+        self.triggers: Dict[SignalHandler, str] = dict()
+        self.conditions: Dict[SignalHandler, Optional[dict]] = dict()
 
     def get(  # type: ignore
         self,
@@ -154,9 +156,7 @@ async def _dispatch(
         try:
             for signal in signals:
                 params.pop("__trigger__", None)
-                requirements = getattr(
-                    signal.handler, "__requirements__", None
-                )
+                requirements = self.conditions.get(signal.handler, None)
                 if (
                     (condition is None and signal.ctx.exclusive is False)
                     or (condition is None and not requirements)
@@ -219,8 +219,8 @@ def add(  # type: ignore
         if not trigger:
             event = ".".join([*parts[:2], "<__trigger__>"])
 
-        handler.__requirements__ = condition  # type: ignore
-        handler.__trigger__ = trigger  # type: ignore
+        self.conditions[handler] = condition
+        self.triggers[handler] = trigger
 
         signal = super().add(
             event,


Code review comments:
diff_hunk: @@ -86,6 +86,8 @@ def __init__(self) -> None:
         )
         self.allow_fail_builtin = True
         self.ctx.loop = None
+        self.triggers: Dict[SignalHandler, str] = dict()
+        self.conditions: Dict[SignalHandler, Optional[dict]] = dict()
path: sanic/signals.py
text: @user1:
This is potentially problematic because we would be using a `SignalHandler` as the key in a dictionary. `SignalHandler` is supposed to be `Callable`, so that means it can be something other than just an instance method. There are callable things that are not hashable, so cannot be keys in a dictionary. For example, any user-defined class that implements an `__eq__` method but not a `__hash__` method is not hashable. So this class:
```
class NotHashable:
  def __init__(self, value):
    self.value = value
  def __eq__(self, other):
    return self.vlaue == other.value
  def __call__(self):
    return self.value
```
Instances of `NotHashable` are in theory valid signal handlers, but they are not hashable (have an `__eq__` but do not have a `__hash__`) so would fail here.

```
a = NotHashable(1)
a() # returns 1
{a: 1} # raises TypeError: Unhashable type 'A'
```

I'm not sure of the full range of callables that are not hashable, whether that's a worthwhile tradeoff, but this implementation would not allow the use of such patterns. I can't seem to find a reference for what is the `hash` of "normal" callables.

@user2:
I'm also not sure how well that would work with pickling too. While the idea of storing on the router seems okay, it probably needs to be a more stable value using `__name__`. But, that too is not guaranteed to work (`partial` for example).

@author:
Any suggestions for an alternative solution? I think a work around is to wrap  the passed in `Callable` with another function so that would make it hashable or even variable injectable (for the original implementation).

@user1:
Could the stored routes be 3-tuples of (handler, conditions, trigger), rather than just the handler? Then when iterating over routes (signals), the conditions and trigger would be simultaneously available.

@author:
How about hashing the handler object's [id](https://docs.python.org/3/library/functions.html#id)? This seems simpler than storing and iterating the list of tuples.

But the problem is "two objects with non-overlapping lifetimes may have the same [id()](https://docs.python.org/3/library/functions.html#id) value."

I think adding `__name__` to a tuple with id, `(handler.__name__, id(handler))` then hash it can possibly get rid of the duplicated id issue.

@user2:
I think we are complicating this further than it has to. I am pretty sure that methodology preexisting the existence of `Route` (in this case a `Signal`) having both a `ctx` (which should really not be used except by the end user) and `extra`.

`trigger` has already moved over to this context. We can probably do the same. How about just do this:


```diff
diff --git a/sanic/signals.py b/sanic/signals.py
index 3b66268..a95de83 100644
--- a/sanic/signals.py
+++ b/sanic/signals.py
@@ -154,9 +154,7 @@ class SignalRouter(BaseRouter):
         try:
             for signal in signals:
                 params.pop("__trigger__", None)
-                requirements = getattr(
-                    signal.handler, "__requirements__", None
-                )
+                requirements = signal.extra.requirements
                 if (
                     (condition is None and signal.ctx.exclusive is False)
                     or (condition is None and not requirements)
@@ -219,8 +217,11 @@ class SignalRouter(BaseRouter):
         if not trigger:
             event = ".".join([*parts[:2], "<__trigger__>"])
 
-        handler.__requirements__ = condition  # type: ignore
-        handler.__trigger__ = trigger  # type: ignore
+        try:
+            handler.__requirements__ = condition  # type: ignore
+            handler.__trigger__ = trigger  # type: ignore
+        except AttributeError:
+            ...
 
         signal = super().add(
             event,
@@ -232,6 +233,7 @@ class SignalRouter(BaseRouter):
         signal.ctx.exclusive = exclusive
         signal.ctx.trigger = trigger
         signal.ctx.definition = event_definition
+        signal.extra.requirements = condition
 
         return cast(Signal, signal)
```

On a completely different note, I must say I find this very frustrating:

```python
>>> class Foo:
...  def bar(self):
...   ...
... 
>>> Foo.bar.z = 1
>>> foo = Foo()
>>> foo.bar.z
1
>>> foo.bar.z = 3
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'method' object has no attribute 'z'
```

@author:
That sounds a good idea. But if `trigger` and `condition` are going to be in `ctx` and/or `extra`, why we still need this?
```python
try:
    handler.__requirements__ = condition  # type: ignore
    handler.__trigger__ = trigger  # type: ignore
except AttributeError:
    ...
```

@user2:
remove in v23

likely no one is touching it, but potentially the trigger is used by a third party app. I'd want to give it some time.

@author:
@user2 I think your method is a good solution. May I apply your code to this PR? I am also thinking about making `handler.__requirements__` and `handler.__trigger__` be [Python properties](https://docs.python.org/3/library/functions.html#property) so we can inject deprecation warning in the getter of the attributes. What do you think?

@user2:
:+1: 

@author:
@user2 
I just did a small experiment and found `property` doesn't work as I expect if it is injected in to `function` object. 

```
>>> def handler():
...     return
...
>>> attr = 1
>>> def fget():
...     return attr
...
>>> def fset(new_val):
...     attr = new_val
...
>>> handler.attr = property(fget=fget, fset=fset)
>>> handler.attr
<property object at 0x000001CB8E290720>
```

I thought `handler.attr` would invoke `fget` but it doesn't... I can't think of another wat to add deprecation warning. Can we just remove it (in whether next version or few versions after it) and state this change in the release note since this was always an internal API? Or any idea to add the warning?

@user2:
Yeah. We'll document the deprecation/removal. Leave it for now though.