From cca6fd8164d1fff7eb846350c837a0df2b0ea7f4 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sun, 11 Jul 2021 06:53:07 +0900 Subject: [PATCH 1/2] Fix #378 by adding middleware error handlers --- slack_bolt/app/app.py | 154 ++++++++++------- slack_bolt/app/async_app.py | 163 ++++++++++-------- slack_bolt/kwargs_injection/async_utils.py | 8 + slack_bolt/kwargs_injection/utils.py | 10 +- .../async_listener_completion_handler.py | 17 +- .../listener/async_listener_error_handler.py | 17 +- .../listener/listener_completion_handler.py | 14 +- slack_bolt/listener/listener_error_handler.py | 15 +- .../async_middleware_error_handler.py | 71 ++++++++ .../middleware/middleware_error_handler.py | 69 ++++++++ tests/scenario_tests/test_error_handler.py | 26 +++ .../scenario_tests_async/test_app_bot_only.py | 5 +- .../test_error_handler.py | 27 +++ 13 files changed, 412 insertions(+), 184 deletions(-) create mode 100644 slack_bolt/middleware/async_middleware_error_handler.py create mode 100644 slack_bolt/middleware/middleware_error_handler.py diff --git a/slack_bolt/app/app.py b/slack_bolt/app/app.py index e4c6a920d..ed2af34c9 100644 --- a/slack_bolt/app/app.py +++ b/slack_bolt/app/app.py @@ -62,6 +62,10 @@ CustomMiddleware, ) from slack_bolt.middleware.message_listener_matches import MessageListenerMatches +from slack_bolt.middleware.middleware_error_handler import ( + DefaultMiddlewareErrorHandler, + CustomMiddlewareErrorHandler, +) from slack_bolt.middleware.url_verification import UrlVerification from slack_bolt.oauth import OAuthFlow from slack_bolt.oauth.internals import select_consistent_installation_store @@ -309,6 +313,9 @@ def message_hello(message, say): executor=listener_executor, ), ) + self._middleware_error_handler = DefaultMiddlewareErrorHandler( + logger=self._framework_logger, + ) self._init_middleware_list_done = False self._init_middleware_list( @@ -448,84 +455,99 @@ def dispatch(self, req: BoltRequest) -> BoltResponse: def middleware_next(): middleware_state["next_called"] = True - for middleware in self._middleware_list: - middleware_state["next_called"] = False - if self._framework_logger.level <= logging.DEBUG: - self._framework_logger.debug(debug_applying_middleware(middleware.name)) - resp = middleware.process(req=req, resp=resp, next=middleware_next) - if not middleware_state["next_called"]: - if resp is None: - # next() method was not called without providing the response to return to Slack - # This should not be an intentional handling in usual use cases. - resp = BoltResponse( - status=404, body={"error": "no next() calls in middleware"} + try: + for middleware in self._middleware_list: + middleware_state["next_called"] = False + if self._framework_logger.level <= logging.DEBUG: + self._framework_logger.debug( + debug_applying_middleware(middleware.name) ) - if self._raise_error_for_unhandled_request is True: - self._listener_runner.listener_error_handler.handle( - error=BoltUnhandledRequestError( + resp = middleware.process(req=req, resp=resp, next=middleware_next) + if not middleware_state["next_called"]: + if resp is None: + # next() method was not called without providing the response to return to Slack + # This should not be an intentional handling in usual use cases. + resp = BoltResponse( + status=404, body={"error": "no next() calls in middleware"} + ) + if self._raise_error_for_unhandled_request is True: + self._listener_runner.listener_error_handler.handle( + error=BoltUnhandledRequestError( + request=req, + current_response=resp, + last_global_middleware_name=middleware.name, + ), request=req, - current_response=resp, - last_global_middleware_name=middleware.name, - ), - request=req, - response=resp, + response=resp, + ) + return resp + self._framework_logger.warning( + warning_unhandled_by_global_middleware(middleware.name, req) ) return resp - self._framework_logger.warning( - warning_unhandled_by_global_middleware(middleware.name, req) - ) return resp - return resp - - for listener in self._listeners: - listener_name = get_name_for_callable(listener.ack_function) - self._framework_logger.debug(debug_checking_listener(listener_name)) - if listener.matches(req=req, resp=resp): - # run all the middleware attached to this listener first - middleware_resp, next_was_not_called = listener.run_middleware( - req=req, resp=resp - ) - if next_was_not_called: - if middleware_resp is not None: - if self._framework_logger.level <= logging.DEBUG: - debug_message = debug_return_listener_middleware_response( - listener_name, - middleware_resp.status, - middleware_resp.body, - starting_time, - ) - self._framework_logger.debug(debug_message) - return middleware_resp - # The last listener middleware didn't call next() method. - # This means the listener is not for this incoming request. - continue - if middleware_resp is not None: - resp = middleware_resp + for listener in self._listeners: + listener_name = get_name_for_callable(listener.ack_function) + self._framework_logger.debug(debug_checking_listener(listener_name)) + if listener.matches(req=req, resp=resp): + # run all the middleware attached to this listener first + middleware_resp, next_was_not_called = listener.run_middleware( + req=req, resp=resp + ) + if next_was_not_called: + if middleware_resp is not None: + if self._framework_logger.level <= logging.DEBUG: + debug_message = ( + debug_return_listener_middleware_response( + listener_name, + middleware_resp.status, + middleware_resp.body, + starting_time, + ) + ) + self._framework_logger.debug(debug_message) + return middleware_resp + # The last listener middleware didn't call next() method. + # This means the listener is not for this incoming request. + continue - self._framework_logger.debug(debug_running_listener(listener_name)) - listener_response: Optional[BoltResponse] = self._listener_runner.run( + if middleware_resp is not None: + resp = middleware_resp + + self._framework_logger.debug(debug_running_listener(listener_name)) + listener_response: Optional[ + BoltResponse + ] = self._listener_runner.run( + request=req, + response=resp, + listener_name=listener_name, + listener=listener, + ) + if listener_response is not None: + return listener_response + + if resp is None: + resp = BoltResponse(status=404, body={"error": "unhandled request"}) + if self._raise_error_for_unhandled_request is True: + self._listener_runner.listener_error_handler.handle( + error=BoltUnhandledRequestError( + request=req, + current_response=resp, + ), request=req, response=resp, - listener_name=listener_name, - listener=listener, ) - if listener_response is not None: - return listener_response - - if resp is None: - resp = BoltResponse(status=404, body={"error": "unhandled request"}) - if self._raise_error_for_unhandled_request is True: - self._listener_runner.listener_error_handler.handle( - error=BoltUnhandledRequestError( - request=req, - current_response=resp, - ), + return resp + return self._handle_unmatched_requests(req, resp) + except Exception as error: + resp = BoltResponse(status=500, body="") + self._middleware_error_handler.handle( + error=error, request=req, response=resp, ) return resp - return self._handle_unmatched_requests(req, resp) def _handle_unmatched_requests( self, req: BoltRequest, resp: BoltResponse @@ -664,6 +686,10 @@ def custom_error_handler(error, body, logger): logger=self._framework_logger, func=func, ) + self._middleware_error_handler = CustomMiddlewareErrorHandler( + logger=self._framework_logger, + func=func, + ) return func # ------------------------- diff --git a/slack_bolt/app/async_app.py b/slack_bolt/app/async_app.py index 8deac5f76..510f89e6b 100644 --- a/slack_bolt/app/async_app.py +++ b/slack_bolt/app/async_app.py @@ -12,6 +12,10 @@ AsyncDefaultListenerCompletionHandler, ) from slack_bolt.listener.asyncio_runner import AsyncioListenerRunner +from slack_bolt.middleware.async_middleware_error_handler import ( + AsyncCustomMiddlewareErrorHandler, + AsyncDefaultMiddlewareErrorHandler, +) from slack_bolt.middleware.message_listener_matches.async_message_listener_matches import ( AsyncMessageListenerMatches, ) @@ -334,6 +338,9 @@ async def message_hello(message, say): # async function logger=self._framework_logger, ), ) + self._async_middleware_error_handler = AsyncDefaultMiddlewareErrorHandler( + logger=self._framework_logger, + ) self._init_middleware_list_done = False self._init_async_middleware_list( @@ -499,89 +506,101 @@ async def async_dispatch(self, req: AsyncBoltRequest) -> BoltResponse: async def async_middleware_next(): middleware_state["next_called"] = True - for middleware in self._async_middleware_list: - middleware_state["next_called"] = False - if self._framework_logger.level <= logging.DEBUG: - self._framework_logger.debug(f"Applying {middleware.name}") - resp = await middleware.async_process( - req=req, resp=resp, next=async_middleware_next - ) - if not middleware_state["next_called"]: - if resp is None: - # next() method was not called without providing the response to return to Slack - # This should not be an intentional handling in usual use cases. - resp = BoltResponse( - status=404, body={"error": "no next() calls in middleware"} - ) - if self._raise_error_for_unhandled_request is True: - await self._async_listener_runner.listener_error_handler.handle( - error=BoltUnhandledRequestError( + try: + for middleware in self._async_middleware_list: + middleware_state["next_called"] = False + if self._framework_logger.level <= logging.DEBUG: + self._framework_logger.debug(f"Applying {middleware.name}") + resp = await middleware.async_process( + req=req, resp=resp, next=async_middleware_next + ) + if not middleware_state["next_called"]: + if resp is None: + # next() method was not called without providing the response to return to Slack + # This should not be an intentional handling in usual use cases. + resp = BoltResponse( + status=404, body={"error": "no next() calls in middleware"} + ) + if self._raise_error_for_unhandled_request is True: + await self._async_listener_runner.listener_error_handler.handle( + error=BoltUnhandledRequestError( + request=req, + current_response=resp, + last_global_middleware_name=middleware.name, + ), request=req, - current_response=resp, - last_global_middleware_name=middleware.name, - ), - request=req, - response=resp, + response=resp, + ) + return resp + self._framework_logger.warning( + warning_unhandled_by_global_middleware(middleware.name, req) ) return resp - self._framework_logger.warning( - warning_unhandled_by_global_middleware(middleware.name, req) - ) return resp - return resp - for listener in self._async_listeners: - listener_name = get_name_for_callable(listener.ack_function) - self._framework_logger.debug(debug_checking_listener(listener_name)) - if await listener.async_matches(req=req, resp=resp): - # run all the middleware attached to this listener first - ( - middleware_resp, - next_was_not_called, - ) = await listener.run_async_middleware(req=req, resp=resp) - if next_was_not_called: + for listener in self._async_listeners: + listener_name = get_name_for_callable(listener.ack_function) + self._framework_logger.debug(debug_checking_listener(listener_name)) + if await listener.async_matches(req=req, resp=resp): + # run all the middleware attached to this listener first + ( + middleware_resp, + next_was_not_called, + ) = await listener.run_async_middleware(req=req, resp=resp) + if next_was_not_called: + if middleware_resp is not None: + if self._framework_logger.level <= logging.DEBUG: + debug_message = ( + debug_return_listener_middleware_response( + listener_name, + middleware_resp.status, + middleware_resp.body, + starting_time, + ) + ) + self._framework_logger.debug(debug_message) + return middleware_resp + # The last listener middleware didn't call next() method. + # This means the listener is not for this incoming request. + continue + if middleware_resp is not None: - if self._framework_logger.level <= logging.DEBUG: - debug_message = debug_return_listener_middleware_response( - listener_name, - middleware_resp.status, - middleware_resp.body, - starting_time, - ) - self._framework_logger.debug(debug_message) - return middleware_resp - # The last listener middleware didn't call next() method. - # This means the listener is not for this incoming request. - continue - - if middleware_resp is not None: - resp = middleware_resp - - self._framework_logger.debug(debug_running_listener(listener_name)) - listener_response: Optional[ - BoltResponse - ] = await self._async_listener_runner.run( + resp = middleware_resp + + self._framework_logger.debug(debug_running_listener(listener_name)) + listener_response: Optional[ + BoltResponse + ] = await self._async_listener_runner.run( + request=req, + response=resp, + listener_name=listener_name, + listener=listener, + ) + if listener_response is not None: + return listener_response + + if resp is None: + resp = BoltResponse(status=404, body={"error": "unhandled request"}) + if self._raise_error_for_unhandled_request is True: + await self._async_listener_runner.listener_error_handler.handle( + error=BoltUnhandledRequestError( + request=req, + current_response=resp, + ), request=req, response=resp, - listener_name=listener_name, - listener=listener, ) - if listener_response is not None: - return listener_response - - if resp is None: - resp = BoltResponse(status=404, body={"error": "unhandled request"}) - if self._raise_error_for_unhandled_request is True: - await self._async_listener_runner.listener_error_handler.handle( - error=BoltUnhandledRequestError( - request=req, - current_response=resp, - ), + return resp + return self._handle_unmatched_requests(req, resp) + + except Exception as error: + resp = BoltResponse(status=500, body="") + await self._async_middleware_error_handler.handle( + error=error, request=req, response=resp, ) return resp - return self._handle_unmatched_requests(req, resp) def _handle_unmatched_requests( self, req: AsyncBoltRequest, resp: BoltResponse @@ -729,6 +748,10 @@ async def custom_error_handler(error, body, logger): func=func, ) ) + self._async_middleware_error_handler = AsyncCustomMiddlewareErrorHandler( + logger=self._framework_logger, + func=func, + ) return func # ------------------------- diff --git a/slack_bolt/kwargs_injection/async_utils.py b/slack_bolt/kwargs_injection/async_utils.py index a6ee86f96..dbfff4691 100644 --- a/slack_bolt/kwargs_injection/async_utils.py +++ b/slack_bolt/kwargs_injection/async_utils.py @@ -27,6 +27,8 @@ def build_async_required_kwargs( response: Optional[BoltResponse], next_func: Callable[[], None] = None, this_func: Optional[Callable] = None, + error: Optional[Exception] = None, # for error handlers + next_keys_required: bool = True, # False for listeners / middleware / error handlers ) -> Dict[str, Any]: all_available_args = { "logger": logger, @@ -53,7 +55,13 @@ def build_async_required_kwargs( # middleware "next": next_func, "next_": next_func, # for the middleware using Python's built-in `next()` function + # error handler + "error": error, # Exception } + if not next_keys_required: + all_available_args.pop("next") + all_available_args.pop("next_") + all_available_args["payload"] = ( all_available_args["options"] or all_available_args["shortcut"] diff --git a/slack_bolt/kwargs_injection/utils.py b/slack_bolt/kwargs_injection/utils.py index 94cbc3c64..f0ad186a4 100644 --- a/slack_bolt/kwargs_injection/utils.py +++ b/slack_bolt/kwargs_injection/utils.py @@ -1,7 +1,7 @@ # pytype: skip-file import inspect import logging -from typing import Callable, Dict, Optional, Any, Sequence +from typing import Callable, Dict, Optional, Any, Sequence, List from slack_bolt.request import BoltRequest from slack_bolt.response import BoltResponse @@ -27,6 +27,8 @@ def build_required_kwargs( response: Optional[BoltResponse], next_func: Callable[[], None] = None, this_func: Optional[Callable] = None, + error: Optional[Exception] = None, # for error handlers + next_keys_required: bool = True, # False for listeners / middleware / error handlers ) -> Dict[str, Any]: all_available_args = { "logger": logger, @@ -53,7 +55,13 @@ def build_required_kwargs( # middleware "next": next_func, "next_": next_func, # for the middleware using Python's built-in `next()` function + # error handler + "error": error, # Exception } + if not next_keys_required: + all_available_args.pop("next") + all_available_args.pop("next_") + all_available_args["payload"] = ( all_available_args["options"] or all_available_args["shortcut"] diff --git a/slack_bolt/listener/async_listener_completion_handler.py b/slack_bolt/listener/async_listener_completion_handler.py index e14a58a5d..14a4d8e91 100644 --- a/slack_bolt/listener/async_listener_completion_handler.py +++ b/slack_bolt/listener/async_listener_completion_handler.py @@ -3,13 +3,7 @@ from logging import Logger from typing import Callable, Dict, Any, Awaitable, Optional -from slack_bolt.listener.async_internals import ( - _build_all_available_args, -) -from slack_bolt.listener.internals import ( - _convert_all_available_args_to_kwargs, -) - +from slack_bolt.kwargs_injection.async_utils import build_async_required_kwargs from slack_bolt.request.async_request import AsyncBoltRequest from slack_bolt.response import BoltResponse @@ -42,15 +36,12 @@ async def handle( request: AsyncBoltRequest, response: Optional[BoltResponse], ) -> None: - all_available_args = _build_all_available_args( + kwargs: Dict[str, Any] = build_async_required_kwargs( + required_arg_names=self.arg_names, logger=self.logger, request=request, response=response, - ) - kwargs: Dict[str, Any] = _convert_all_available_args_to_kwargs( - all_available_args=all_available_args, - arg_names=self.arg_names, - logger=self.logger, + next_keys_required=False, ) await self.func(**kwargs) diff --git a/slack_bolt/listener/async_listener_error_handler.py b/slack_bolt/listener/async_listener_error_handler.py index 17d643d09..7f038380a 100644 --- a/slack_bolt/listener/async_listener_error_handler.py +++ b/slack_bolt/listener/async_listener_error_handler.py @@ -3,13 +3,7 @@ from logging import Logger from typing import Callable, Dict, Any, Awaitable, Optional -from slack_bolt.listener.async_internals import ( - _build_all_available_args, -) -from slack_bolt.listener.internals import ( - _convert_all_available_args_to_kwargs, -) - +from slack_bolt.kwargs_injection.async_utils import build_async_required_kwargs from slack_bolt.request.async_request import AsyncBoltRequest from slack_bolt.response import BoltResponse @@ -46,16 +40,13 @@ async def handle( request: AsyncBoltRequest, response: Optional[BoltResponse], ) -> None: - all_available_args = _build_all_available_args( + kwargs: Dict[str, Any] = build_async_required_kwargs( + required_arg_names=self.arg_names, logger=self.logger, error=error, request=request, response=response, - ) - kwargs: Dict[str, Any] = _convert_all_available_args_to_kwargs( - all_available_args=all_available_args, - arg_names=self.arg_names, - logger=self.logger, + next_keys_required=False, ) returned_response = await self.func(**kwargs) if returned_response is not None and isinstance( diff --git a/slack_bolt/listener/listener_completion_handler.py b/slack_bolt/listener/listener_completion_handler.py index 2419bc56e..18a062d32 100644 --- a/slack_bolt/listener/listener_completion_handler.py +++ b/slack_bolt/listener/listener_completion_handler.py @@ -3,10 +3,7 @@ from logging import Logger from typing import Callable, Dict, Any, Optional -from slack_bolt.listener.internals import ( - _build_all_available_args, - _convert_all_available_args_to_kwargs, -) +from slack_bolt.kwargs_injection import build_required_kwargs from slack_bolt.request.request import BoltRequest from slack_bolt.response.response import BoltResponse @@ -38,15 +35,12 @@ def handle( request: BoltRequest, response: Optional[BoltResponse], ): - all_available_args = _build_all_available_args( + kwargs: Dict[str, Any] = build_required_kwargs( + required_arg_names=self.arg_names, logger=self.logger, request=request, response=response, - ) - kwargs: Dict[str, Any] = _convert_all_available_args_to_kwargs( - all_available_args=all_available_args, - arg_names=self.arg_names, - logger=self.logger, + next_keys_required=False, ) self.func(**kwargs) diff --git a/slack_bolt/listener/listener_error_handler.py b/slack_bolt/listener/listener_error_handler.py index 0ee9dde8e..3bf4e1da9 100644 --- a/slack_bolt/listener/listener_error_handler.py +++ b/slack_bolt/listener/listener_error_handler.py @@ -3,11 +3,7 @@ from logging import Logger from typing import Callable, Dict, Any, Optional -from slack_bolt.listener.internals import ( - _build_all_available_args, - _convert_all_available_args_to_kwargs, -) - +from slack_bolt.kwargs_injection import build_required_kwargs from slack_bolt.request.request import BoltRequest from slack_bolt.response.response import BoltResponse @@ -42,16 +38,13 @@ def handle( request: BoltRequest, response: Optional[BoltResponse], ): - all_available_args = _build_all_available_args( + kwargs: Dict[str, Any] = build_required_kwargs( + required_arg_names=self.arg_names, logger=self.logger, error=error, request=request, response=response, - ) - kwargs: Dict[str, Any] = _convert_all_available_args_to_kwargs( - all_available_args=all_available_args, - arg_names=self.arg_names, - logger=self.logger, + next_keys_required=False, ) returned_response = self.func(**kwargs) if returned_response is not None and isinstance( diff --git a/slack_bolt/middleware/async_middleware_error_handler.py b/slack_bolt/middleware/async_middleware_error_handler.py new file mode 100644 index 000000000..426a721c2 --- /dev/null +++ b/slack_bolt/middleware/async_middleware_error_handler.py @@ -0,0 +1,71 @@ +import inspect +from abc import ABCMeta, abstractmethod +from logging import Logger +from typing import Callable, Dict, Any, Awaitable, Optional + +from slack_bolt.kwargs_injection.async_utils import build_async_required_kwargs +from slack_bolt.request.async_request import AsyncBoltRequest +from slack_bolt.response import BoltResponse + + +class AsyncMiddlewareErrorHandler(metaclass=ABCMeta): + @abstractmethod + async def handle( + self, + error: Exception, + request: AsyncBoltRequest, + response: Optional[BoltResponse], + ) -> None: + """Handles an unhandled exception. + + Args: + error: The raised exception. + request: The request. + response: The response. + """ + raise NotImplementedError() + + +class AsyncCustomMiddlewareErrorHandler(AsyncMiddlewareErrorHandler): + def __init__( + self, logger: Logger, func: Callable[..., Awaitable[Optional[BoltResponse]]] + ): + self.func = func + self.logger = logger + self.arg_names = inspect.getfullargspec(func).args + + async def handle( + self, + error: Exception, + request: AsyncBoltRequest, + response: Optional[BoltResponse], + ) -> None: + kwargs: Dict[str, Any] = build_async_required_kwargs( + required_arg_names=self.arg_names, + logger=self.logger, + error=error, + request=request, + response=response, + next_keys_required=False, + ) + returned_response = await self.func(**kwargs) + if returned_response is not None and isinstance( + returned_response, BoltResponse + ): + response.status = returned_response.status + response.headers = returned_response.headers + response.body = returned_response.body + + +class AsyncDefaultMiddlewareErrorHandler(AsyncMiddlewareErrorHandler): + def __init__(self, logger: Logger): + self.logger = logger + + async def handle( + self, + error: Exception, + request: AsyncBoltRequest, + response: Optional[BoltResponse], + ): + message = f"Failed to run a middleware function (error: {error})" + self.logger.exception(message) diff --git a/slack_bolt/middleware/middleware_error_handler.py b/slack_bolt/middleware/middleware_error_handler.py new file mode 100644 index 000000000..0315f03da --- /dev/null +++ b/slack_bolt/middleware/middleware_error_handler.py @@ -0,0 +1,69 @@ +import inspect +from abc import ABCMeta, abstractmethod +from logging import Logger +from typing import Callable, Optional, Any, Dict + +from slack_bolt.kwargs_injection.utils import build_required_kwargs +from slack_bolt.request.request import BoltRequest +from slack_bolt.response.response import BoltResponse + + +class MiddlewareErrorHandler(metaclass=ABCMeta): + @abstractmethod + def handle( + self, + error: Exception, + request: BoltRequest, + response: Optional[BoltResponse], + ) -> None: + """Handles an unhandled exception. + + Args: + error: The raised exception. + request: The request. + response: The response. + """ + raise NotImplementedError() + + +class CustomMiddlewareErrorHandler(MiddlewareErrorHandler): + def __init__(self, logger: Logger, func: Callable[..., Optional[BoltResponse]]): + self.func = func + self.logger = logger + self.arg_names = inspect.getfullargspec(func).args + + def handle( + self, + error: Exception, + request: BoltRequest, + response: Optional[BoltResponse], + ): + kwargs: Dict[str, Any] = build_required_kwargs( + required_arg_names=self.arg_names, + logger=self.logger, + error=error, + request=request, + response=response, + next_keys_required=False, + ) + returned_response = self.func(**kwargs) + if returned_response is not None and isinstance( + returned_response, BoltResponse + ): + response.status = returned_response.status + response.headers = returned_response.headers + response.body = returned_response.body + + +class DefaultMiddlewareErrorHandler(MiddlewareErrorHandler): + def __init__(self, logger: Logger): + self.logger = logger + + def handle( + self, + error: Exception, + request: BoltRequest, + response: Optional[BoltResponse], + ): + message = f"Failed to run a middleware middleware (error: {error})" + self.logger.exception(message) diff --git a/tests/scenario_tests/test_error_handler.py b/tests/scenario_tests/test_error_handler.py index be744efe0..eb82c1d84 100644 --- a/tests/scenario_tests/test_error_handler.py +++ b/tests/scenario_tests/test_error_handler.py @@ -242,3 +242,29 @@ def handle_errors(error): response = app.dispatch(self.build_valid_request()) assert response.status == 404 assert response.body == "TODO" + + def test_global_middleware_errors(self): + app = App( + client=self.web_client, + signing_secret=self.signing_secret, + ) + + @app.middleware + def broken_middleware(next_): + assert next_ is not None + raise RuntimeError("Something wrong!") + + response = app.dispatch(self.build_valid_request()) + assert response.status == 500 + assert response.body == "" + + @app.error + def handle_errors(body, next_, error): + assert next_ is None + assert body is not None + assert isinstance(error, RuntimeError) + return BoltResponse(status=503, body="as expected") + + response = app.dispatch(self.build_valid_request()) + assert response.status == 503 + assert response.body == "as expected" diff --git a/tests/scenario_tests_async/test_app_bot_only.py b/tests/scenario_tests_async/test_app_bot_only.py index a94fd497d..e557a6588 100644 --- a/tests/scenario_tests_async/test_app_bot_only.py +++ b/tests/scenario_tests_async/test_app_bot_only.py @@ -74,8 +74,9 @@ async def test_bot_only_default_off(self): app.event("app_mention")(whats_up) request = self.build_valid_app_mention_request() - with pytest.raises(ValueError): - await app.async_dispatch(request) + response = await app.async_dispatch(request) + assert response.status == 500 + assert response.body == "" @pytest.mark.asyncio async def test_bot_only(self): diff --git a/tests/scenario_tests_async/test_error_handler.py b/tests/scenario_tests_async/test_error_handler.py index d534a764e..c190cb018 100644 --- a/tests/scenario_tests_async/test_error_handler.py +++ b/tests/scenario_tests_async/test_error_handler.py @@ -257,3 +257,30 @@ async def handle_errors(error): response = await app.async_dispatch(self.build_valid_request()) assert response.status == 404 assert response.body == "TODO" + + @pytest.mark.asyncio + async def test_middleware_errors(self): + app = AsyncApp( + client=self.web_client, + signing_secret=self.signing_secret, + ) + + @app.middleware + async def broken_middleware(next_): + assert next_ is not None + raise RuntimeError("Something wrong!") + + response = await app.async_dispatch(self.build_valid_request()) + assert response.status == 500 + assert response.body == "" + + @app.error + async def handle_errors(body, next_, error): + assert next_ is None + assert body is not None + assert isinstance(error, RuntimeError) + return BoltResponse(status=503, body="as expected") + + response = await app.async_dispatch(self.build_valid_request()) + assert response.status == 503 + assert response.body == "as expected" From 0870ec2b1cf7737e08b5123fcecb0abef2a98685 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sun, 11 Jul 2021 06:59:00 +0900 Subject: [PATCH 2/2] Delete the internals that are no longer used --- slack_bolt/listener/async_internals.py | 58 -------------------- slack_bolt/listener/internals.py | 75 -------------------------- 2 files changed, 133 deletions(-) delete mode 100644 slack_bolt/listener/async_internals.py delete mode 100644 slack_bolt/listener/internals.py diff --git a/slack_bolt/listener/async_internals.py b/slack_bolt/listener/async_internals.py deleted file mode 100644 index 60347bfb6..000000000 --- a/slack_bolt/listener/async_internals.py +++ /dev/null @@ -1,58 +0,0 @@ -from logging import Logger -from typing import Dict, Any, Optional - -from slack_bolt.request.async_request import AsyncBoltRequest -from slack_bolt.request.payload_utils import ( - to_options, - to_shortcut, - to_action, - to_view, - to_command, - to_event, - to_message, - to_step, -) -from slack_bolt.response import BoltResponse - - -def _build_all_available_args( - logger: Logger, - request: AsyncBoltRequest, - response: Optional[BoltResponse], - error: Optional[Exception] = None, -) -> Dict[str, Any]: - all_available_args = { - "logger": logger, - "error": error, - "client": request.context.client, - "req": request, - "request": request, - "resp": response, - "response": response, - "context": request.context, - # payload - "body": request.body, - "options": to_options(request.body), - "shortcut": to_shortcut(request.body), - "action": to_action(request.body), - "view": to_view(request.body), - "command": to_command(request.body), - "event": to_event(request.body), - "message": to_message(request.body), - "step": to_step(request.body), - # utilities - "say": request.context.say, - "respond": request.context.respond, - } - all_available_args["payload"] = ( - all_available_args["options"] - or all_available_args["shortcut"] - or all_available_args["action"] - or all_available_args["view"] - or all_available_args["command"] - or all_available_args["event"] - or all_available_args["message"] - or all_available_args["step"] - or request.body - ) - return all_available_args diff --git a/slack_bolt/listener/internals.py b/slack_bolt/listener/internals.py deleted file mode 100644 index 9b0682c1d..000000000 --- a/slack_bolt/listener/internals.py +++ /dev/null @@ -1,75 +0,0 @@ -from logging import Logger -from typing import Optional, Dict, Any, List - -from slack_bolt.request.request import BoltRequest -from slack_bolt.response.response import BoltResponse - -from slack_bolt.request.payload_utils import ( - to_options, - to_shortcut, - to_action, - to_view, - to_command, - to_event, - to_message, - to_step, -) - - -def _build_all_available_args( - logger: Logger, - request: BoltRequest, - response: Optional[BoltResponse], - error: Optional[Exception] = None, -) -> Dict[str, Any]: - all_available_args = { - "logger": logger, - "error": error, - "client": request.context.client, - "req": request, - "request": request, - "resp": response, - "response": response, - "context": request.context, - # payload - "body": request.body, - "options": to_options(request.body), - "shortcut": to_shortcut(request.body), - "action": to_action(request.body), - "view": to_view(request.body), - "command": to_command(request.body), - "event": to_event(request.body), - "message": to_message(request.body), - "step": to_step(request.body), - # utilities - "say": request.context.say, - "respond": request.context.respond, - } - all_available_args["payload"] = ( - all_available_args["options"] - or all_available_args["shortcut"] - or all_available_args["action"] - or all_available_args["view"] - or all_available_args["command"] - or all_available_args["event"] - or all_available_args["message"] - or all_available_args["step"] - or request.body - ) - return all_available_args - - -def _convert_all_available_args_to_kwargs( - all_available_args: Dict[str, Any], - arg_names: List[str], - logger: Logger, -) -> Dict[str, Any]: - kwargs: Dict[str, Any] = { # type: ignore - k: v for k, v in all_available_args.items() if k in arg_names # type: ignore - } - found_arg_names = kwargs.keys() - for name in arg_names: - if name not in found_arg_names: - logger.warning(f"{name} is not a valid argument") - kwargs[name] = None - return kwargs