-
Notifications
You must be signed in to change notification settings - Fork 278
fix(assistant): improve middleware dispatch and inject kwargs in middleware #1456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
69f8f93
5a9f0f1
38c40a1
807e53b
ebc67bb
4c9c247
27e6723
5ec3366
cf20db4
a024552
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from slack_bolt.context.assistant.thread_context_store.store import AssistantThreadContextStore | ||
| from slack_bolt.listener_matcher.builtins import build_listener_matcher | ||
|
|
||
| from slack_bolt.middleware.attaching_agent_kwargs import AttachingAgentKwargs | ||
| from slack_bolt.request.request import BoltRequest | ||
| from slack_bolt.response.response import BoltResponse | ||
| from slack_bolt.listener_matcher import CustomListenerMatcher | ||
|
|
@@ -236,6 +237,15 @@ def process( # type: ignore[return] | |
| if listeners is not None: | ||
| for listener in listeners: | ||
| if listener.matches(req=req, resp=resp): | ||
| middleware_resp, next_was_not_called = listener.run_middleware(req=req, resp=resp) | ||
| if next_was_not_called: | ||
| if middleware_resp is not None: | ||
| return middleware_resp | ||
| # The listener middleware didn't call next(). | ||
| # Skip this listener and try the next one. | ||
| continue | ||
| if middleware_resp is not None: | ||
| resp = middleware_resp | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👁️🗨️ question: Was middleware not handled for the
And it's not clear to me if the global middleware is sufficient or if this is gives us confidence that listeners for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good catch 💯 from my understanding the middleware set on assistant listers were ignored 😅 this does seem to be needed and
I think the unit tests should give us confidence that the arguments and middleware are present in the listeners |
||
| return listener_runner.run( | ||
| request=req, | ||
| response=resp, | ||
|
|
@@ -262,6 +272,7 @@ def build_listener( | |
| return listener_or_functions | ||
| elif isinstance(listener_or_functions, list): | ||
| middleware = middleware if middleware else [] | ||
| middleware.insert(0, AttachingAgentKwargs(self.thread_context_store)) | ||
| functions = listener_or_functions | ||
| ack_function = functions.pop(0) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| from .attaching_agent_kwargs import AttachingAgentKwargs | ||
|
|
||
| __all__ = [ | ||
| "AttachingAgentKwargs", | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| from typing import Optional, Callable, Awaitable | ||
|
|
||
| from slack_bolt.context.assistant.async_assistant_utilities import AsyncAssistantUtilities | ||
| from slack_bolt.context.assistant.thread_context_store.async_store import AsyncAssistantThreadContextStore | ||
| from slack_bolt.middleware.async_middleware import AsyncMiddleware | ||
| from slack_bolt.request.async_request import AsyncBoltRequest | ||
| from slack_bolt.request.payload_utils import is_assistant_event, to_event | ||
| from slack_bolt.response import BoltResponse | ||
|
|
||
|
|
||
| class AsyncAttachingAgentKwargs(AsyncMiddleware): | ||
|
|
||
| thread_context_store: Optional[AsyncAssistantThreadContextStore] | ||
|
|
||
| def __init__(self, thread_context_store: Optional[AsyncAssistantThreadContextStore] = None): | ||
| self.thread_context_store = thread_context_store | ||
|
|
||
| async def async_process( | ||
| self, | ||
| *, | ||
| req: AsyncBoltRequest, | ||
| resp: BoltResponse, | ||
| next: Callable[[], Awaitable[BoltResponse]], | ||
| ) -> Optional[BoltResponse]: | ||
| event = to_event(req.body) | ||
| if event is not None: | ||
| if is_assistant_event(req.body): | ||
| assistant = AsyncAssistantUtilities( | ||
| payload=event, | ||
| context=req.context, | ||
| thread_context_store=self.thread_context_store, | ||
| ) | ||
| req.context["say"] = assistant.say | ||
| req.context["set_status"] = assistant.set_status | ||
| req.context["set_title"] = assistant.set_title | ||
| req.context["set_suggested_prompts"] = assistant.set_suggested_prompts | ||
| req.context["get_thread_context"] = assistant.get_thread_context | ||
| req.context["save_thread_context"] = assistant.save_thread_context | ||
| return await next() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| from typing import Optional, Callable | ||
|
|
||
| from slack_bolt.context.assistant.assistant_utilities import AssistantUtilities | ||
| from slack_bolt.context.assistant.thread_context_store.store import AssistantThreadContextStore | ||
| from slack_bolt.middleware import Middleware | ||
| from slack_bolt.request.payload_utils import is_assistant_event, to_event | ||
| from slack_bolt.request.request import BoltRequest | ||
| from slack_bolt.response.response import BoltResponse | ||
|
|
||
|
|
||
| class AttachingAgentKwargs(Middleware): | ||
|
|
||
| thread_context_store: Optional[AssistantThreadContextStore] | ||
|
|
||
| def __init__(self, thread_context_store: Optional[AssistantThreadContextStore] = None): | ||
| self.thread_context_store = thread_context_store | ||
|
|
||
| def process(self, *, req: BoltRequest, resp: BoltResponse, next: Callable[[], BoltResponse]) -> Optional[BoltResponse]: | ||
| event = to_event(req.body) | ||
| if event is not None: | ||
| if is_assistant_event(req.body): | ||
| assistant = AssistantUtilities( | ||
| payload=event, | ||
| context=req.context, | ||
| thread_context_store=self.thread_context_store, | ||
| ) | ||
| req.context["say"] = assistant.say | ||
| req.context["set_status"] = assistant.set_status | ||
| req.context["set_title"] = assistant.set_title | ||
| req.context["set_suggested_prompts"] = assistant.set_suggested_prompts | ||
| req.context["get_thread_context"] = assistant.get_thread_context | ||
| req.context["save_thread_context"] = assistant.save_thread_context | ||
| return next() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌟 praise: In agreement with @srtaalej this is a good change - the
thread_tsattempted before was not set at this point in execution. Now we want to avoid changing this behavior while still improving the availablecontextvalues. Solid!