slackapi / bolt-python

A framework to build Slack apps using Python
https://tools.slack.dev/bolt-python/
MIT License
1.07k stars 248 forks source link

Error triggered when next() is not returned in a middleware using a shortcut #1213

Open mobidyc opened 1 day ago

mobidyc commented 1 day ago

Reproducible in:

Only with a "shortcut" command, when a middleware returns a Boltresponse instead of next(), an error is triggered in the client (app and web), even if the treatment is OK.

Either it should be documented that a middleware should always return next() or it is a bug.

For information, I tried to use a BolResponse to ease my unittests.

The slack_bolt version

slack-bolt==1.21.2

Python runtime version

Python 3.10.15

OS info

Linux 6.11.8-200.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Nov 14 20:38:18 UTC 2024

Steps to reproduce:

This call triggers the error in the slack client (app and web), however, the treatment is OK and works with no error.

slack_app = App(token=SLACK_BOT_TOKEN, signing_secret=SLACK_SIGNING_SECRET)

# Middleware to check if the event is from allowed channels
@slack_app.use  # Middleware that runs before events
def filter_channels(client, context, logger, payload, next):
    channel_id = context.get("channel_id") or context.get("channel")

    http_ok = BoltResponse(status=200, body="OK")

    if not CHANNELS_ARE_RESTRICTED:
        next()
        return http_ok

@slack_app.shortcut("shortcut_test")
def handle_shortcut_test(ack, body, respond):
    ack()
    print("Shortcut test successful!")

Expected result:

No error

Actual result:

Capture d’écran du 2024-12-02 19-34-18

Workaround

Return next() instead of just calling it

slack_app = App(token=SLACK_BOT_TOKEN, signing_secret=SLACK_SIGNING_SECRET)

# Middleware to check if the event is from allowed channels
@slack_app.use  # Middleware that runs before events
def filter_channels(client, context, logger, payload, next):
    channel_id = context.get("channel_id") or context.get("channel")

    if not CHANNELS_ARE_RESTRICTED:
        return next()

@slack_app.shortcut("shortcut_test")
def handle_shortcut_test(ack, body, respond):
    ack()
    print("Shortcut test successful!")
hello-ashleyintech commented 1 day ago

Hi, @mobidyc! Thanks for submitting this issue! 🙌

It looks like you're implementing global middleware, and it is correct that you must call next() in your global middleware, regardless of listener type, to ensure that the request is processed correctly. It passes control to the next middleware function in the chain or to the final shortcut handler.

I'm seeing some documentation on this in the docs linked above:

Both global and listener middleware must call next() to pass control of the execution chain to the next middleware.

Was this something you encountered in the docs? If so, is there a way this could be rephrased to make next()'s requirements more clear?

You also mention that this happens only with a shortcut command - are you using this middleware with other listener types, and are you seeing different results?

mobidyc commented 1 day ago

Thank you for your reply.

The issue is not with calling next() but with returning next(), which is a bit different.

I also use that global middleware to check if my bot is invited in a channel not allowed, in this case, the bot just leaves the channel, and no errors are seen on the client side.

hello-ashleyintech commented 14 hours ago

@mobidyc hi! Thanks for adding that additional clarification.

I wonder if this is due to the next(); being inside a conditional, and having the return of next() properly terminate the middleware process. Looking at the example here, I see that next() is called at the very end of the function completely broken out of any logic, similar to how a default return; statement might - have you tried this to see if this helps with your case? You could modify the logic to something like this:

if CHANNELS_ARE_RESTRICTED:
        // put some sort of error log here
next();

Let me know if this helps!

mobidyc commented 2 hours ago

I workaround my issue by just returning next() now.

Still don't know if this is a bug or not, but I think it worth putting the information in the documentation.

hello-ashleyintech commented 1 minute ago

@mobidyc gotcha! @seratch do you know if this is expected behavior re: returning next()?