Skip to content

Fix #378 by adding middleware error handlers #401

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.

Sign up for GitHub

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

Merged
seratch merged 3 commits into from
Jul 14, 2021
Merged

Fix #378 by adding middleware error handlers #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cca6fd8
Fix #378 by adding middleware error handlers
seratch Jul 10, 2021
0870ec2
Delete the internals that are no longer used
seratch Jul 10, 2021
26f1a30
Merge branch 'main' into issue-378-middleware-errors
seratch Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 90 additions & 64 deletions slack_bolt/app/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Comment on lines +543 to 550

@seratch seratch Jul 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines of code are the essential changes in this pull request.

return self._handle_unmatched_requests(req, resp)

def _handle_unmatched_requests(
self, req: BoltRequest, resp: BoltResponse
Expand Down Expand Up @@ -664,6 +686,10 @@ def custom_error_handler(error, body, logger):
logger=self._framework_logger,
func=func,
)
self._middleware_error_handler = CustomMiddlewareErrorHandler(

@seratch seratch Jul 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backward-compatibility, I didn't rename and modify the existing CustomListenerErrorHandler to be reusable for this. In the future major version, we may want to have a unified interface for these two error handlers. They are mostly the same.

logger=self._framework_logger,
func=func,
)
return func

# -------------------------
Expand Down
163 changes: 93 additions & 70 deletions slack_bolt/app/async_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

# -------------------------
Expand Down
Loading