Allow custom decorators to overwrite Bolt arguments injection

[dimitarOnGithub]

Hi,

Opening this issue as a continuation of the conversation in #688, as I believe there's some design decisions that need to be made prior to reviewing and implementing the suggested changes.

Suggested Changes

My suggestion is changing the get_arg_names_of_callable to:

def get_arg_names_of_callable(func: Callable) -> List[str]:
    # Check if the func is decorated and if the decorator requires specific data injection
    # in the case of a decorator that handles *args and **kwargs, the length of inspect.getfullargspec(func).args is 0,
    # so we can defer to the unwrapped func
    if hasattr(func, '__wrapped__') and inspect.getfullargspec(func).args:
        return inspect.getfullargspec(func).args
    return inspect.getfullargspec(inspect.unwrap(func)).args

Checking if the listener function contains a __wrapped__ attribute (populated by functools), allows the function to to instead inspect the wrapper itself.

In the cases where the wrapper has not defined specific args injection (ie, wrapper signature is def wrapper(*args, **kwargs)), the length of inspect.getfullargspec(func).args is 0, in which case the function defaults to unwrapping and inspecting the listener itself.

What does this achieve?

This change would allow users to have decorators that overwrite the arguments injection as defined by the listener functions, to illustrate;

With the current behavior, having a listener defined as:

@app.event("message")
def message_listener(event, say):
    say(f"hi! You said {event.get('text')}")

tells Bolt to inject the event and say arguments when calling the listener function:

>>> from test_file import message_listener
>>> from slack_bolt.util.utils import get_arg_names_of_callable
>>> get_arg_names_of_callable(message_listener)
['event', 'say']

Implementing the changes, would instead result in:

def custom_decorator(function):
    @wraps(function)
    def wrapper(event, context, say):
        # Do something here
        return function(event)
    return wrapper

@app.event("message")
@custom_decorator
def message_listener(event):
    print(f"Got {event}")

>>> from test_file import message_listener
>>> from slack_bolt.util.utils import get_arg_names_of_callable
>>> get_arg_names_of_callable(message_listener)
['event', 'context', 'say']

Or a custom decorator with additional arguments

def custom_decorator_with_value(custom_variable):
    def decorator(function):
        @wraps(function)
        def wrapper(event, context, say):
            # Do something here
            # Only pass the custom variable to the listener
            return function(custom_variable)
        return wrapper
    return decorator

@app.event("message")
@custom_decorator_with_value("test value overriding the args injection")
def message_listener(user_var):
    print(f"Got {user_var}")

While still retaining the fix of #689:

def general_decorator(function):
    @wraps(function)
    def wrapper(*args, **kwargs):
        # Wrapper hasn't defined specific args injection,
        # Bolt will inject whatever the listener func has requested.
        # It's up to the user to manipulate the kwargs and pass them onwards
        return function(*args, **kwargs)
    return wrapper

@app.event("message")
@general_decorator
def message_listener(event, client, say):
    print(f"Event: {event}")
    print(f"Client: {client}")
    print(f"Say: {say}")

>>> from test_file import message_listener
>>> from slack_bolt.util.utils import get_arg_names_of_callable
>>> get_arg_names_of_callable(message_listener)
['event', 'client', 'say']

Assumptions and Downsides

Unfortunately, this change operates based on a few assumptions, each of which comes with downsides, and this is the reason why I believe it's best to talk through it first before submitting a PR straight off the bat; perhaps the shortcomings will outweigh the benefits and implementing this would not be a good fit for the Bolt's design principles and long-term maintenance.

1. The request relies entirely on the assumption that any and all decorators would have been declared via the functools @wraps functionality as that wrapper is the one that populates the __wrapped__ argument;
2. The request, if implemented, will only work in the case of one decorator override - unfortunately, functionality becomes very obscure in cases where users have two or more decorators applied to a listener function and it opens the door to human error in cases where one decorator has been defined to require event and client, while the next one expects resp and ack. The solution to this, of course, would be on the end user - they must be consistent in their decorators' definition and ensure that either all of them require the exact same args, or let the listener function be responsible for args injection while the decorators themselves only access the kwargs and modify them as required (in other words, the last code block above);
3. This does not account for frameworks such as the one mentioned in #709 , I have no experience with it and I can't provide feedback as to how it'd work, if at all, in that case.

---

Category (place an x in each of the [ ])

• [x] slack_bolt.App and/or its core components
• [x] slack_bolt.async_app.AsyncApp and/or its core components
• [ ] Adapters in slack_bolt.adapter
• [ ] Others

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

[seratch]

Hi @dimitarOnGithub, thank you so much for taking the time to write this.

First off, apologies for any confusion, but when I read your comment on the closed issue, I was imagining slightly different use cases (that being said, I was fully unsure, so I asked you to share more details though).

The flexibility described here is something the bolt-python project would like to avoid, because, as you mentioned, customizations of listeners/middleware utilizing decorators could cause unexpected behavior and be difficult for humans to debug. Additionally, future enhancements on the bolt-python side may create conflicts.

If I understand your intention correctly, customization before running a listener is achievable with custom global (@app.use/middleware) or listener-specific (a parameter to @app.event) middleware. We would like to provide a single approach, fully controlled by the framework, to handle this.

I apologize again if this was a waste of your time, but I hope my response here clarifies things.

[dimitarOnGithub]

Hi @seratch ,

Please do not apologize! In fact, I should be the one doing that considering I didn't make my intention in the original comment clear enough, it'd have surely saved you the time you had to put aside to review this instead.

I completely understand, hence why I wanted to first provide all the details before jumping the gun and submitting a PR, I appreciate you taking the time to review my suggestion and provide feedback.

In my case, I'm already using some middleware to filter out and manipulate incoming requests, but as I use Bolt as a part of a bigger application, I wanted to be able to further abstract chunks of the requests and only provide listener functions with the bare minimum they need to perform a given request. Either way, it's not the end of the world for me and given the flexibility of the library, I'm sure I'll be able to find workarounds.

Again, thank you so much for your time and I can't wait to see what the future holds for Bolt!