Repository navigation
fix(assistant): accept plain functions as listener middleware - #1591
Open
sahiljagtap08 wants to merge 1 commit into
Open
sahiljagtap08 wants to merge 1 commit into
sahiljagtap08 wants to merge 1 commit into
Conversation
Assistant listener decorators advertise middleware as a list of
functions or Middleware objects, like App does. build_listener passed
the list straight to CustomListener, so a plain function raised
AttributeError ('function' object has no attribute 'process') on every
matching event. It also inserted AttachingConversationKwargs into the
caller's list, so a list shared between decorators grew on each use.
- Wrap plain functions in CustomMiddleware / AsyncCustomMiddleware
- Build a new middleware list instead of editing the caller's
- Add sync and async tests using a function middleware
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AssistantandAsyncAssistantlistener decorators (thread_started,user_message, and so on) are typed to accept middleware as a list of plain functions orMiddlewareobjects, the same asApp.event(). Butbuild_listenerpassed the list straight toCustomListenerwithout wrapping functions, so a plain function middleware raised:on every matching event, which became a 500.
A second problem in the same code:
build_listenerinsertedAttachingConversationKwargsinto the caller's list in place. A list shared between two decorators ended up with that middleware added twice.Changes in
slack_bolt/middleware/assistant/:CustomMiddleware(sync) orAsyncCustomMiddleware(async), matching whatApp._register_listenerdoes. Anything else raises the sameBoltErrorthatAppraises.build_listenertype hint now matches the decorator type hints.Testing
Added
test_assistant_with_function_listener_middlewareto bothtests/scenario_tests/test_events_assistant.pyandtests/scenario_tests_async/test_events_assistant.py. Each registers a function middleware on two listeners, checks the middleware and listener both run, and checks the caller's list is unchanged. Both fail onmainand pass with this change. Ran./scripts/format.sh,./scripts/lint.sh,./scripts/run_mypy.sh, and the assistant test files.Category
Requirements
./scripts/install_all_and_run_tests.shafter making the changes.Fixes #1597