Skip to content
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

Fix #378 by adding middleware error handlers #401

Merged
merged 3 commits into from Jul 14, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Jul 10, 2021

This pull request fixes #378 by applying the following changes:

  • Add middleware_error_handler in both App and AsyncApp
  • Update @app.error registration to enable the custom error handler for both middleware and listeners
  • Refactor the kwargs injection for error handlers

Category (place an x in each of the [ ])

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run ./scripts/install_all_and_run_tests.sh after making the changes.

Comment on lines +543 to 550
except Exception as error:
resp = BoltResponse(status=500, body="")
self._middleware_error_handler.handle(
error=error,
request=req,
response=resp,
)
return resp
Copy link
Member 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.

@@ -664,6 +686,10 @@ def custom_error_handler(error, body, logger):
logger=self._framework_logger,
func=func,
)
self._middleware_error_handler = CustomMiddlewareErrorHandler(
Copy link
Member 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.

@@ -1,58 +0,0 @@
from logging import Logger
Copy link
Member Author

Choose a reason for hiding this comment

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

As this internal code is no longer used, we can safely delete the whole. We've switched to slack_bolt/kwargs_inject/async_utils.py

@codecov
Copy link

codecov bot commented Jul 10, 2021

Codecov Report

Merging #401 (0870ec2) into main (007d278) will increase coverage by 2.07%.
The diff coverage is n/a.

❗ Current head 0870ec2 differs from pull request most recent head 26f1a30. Consider uploading reports for the commit 26f1a30 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   89.65%   91.72%   +2.07%     
==========================================
  Files         128      167      +39     
  Lines        3585     5436    +1851     
==========================================
+ Hits         3214     4986    +1772     
- Misses        371      450      +79     
Impacted Files Coverage Δ
slack_bolt/util/utils.py 84.61% <0.00%> (-5.87%) ⬇️
slack_bolt/request/async_request.py 94.28% <0.00%> (-5.72%) ⬇️
slack_bolt/request/async_internals.py 96.15% <0.00%> (-3.85%) ⬇️
slack_bolt/request/request.py 97.05% <0.00%> (-2.95%) ⬇️
slack_bolt/adapter/aws_lambda/chalice_handler.py 84.28% <0.00%> (-2.39%) ⬇️
slack_bolt/oauth/internals.py 97.77% <0.00%> (-2.23%) ⬇️
slack_bolt/version.py 100.00% <0.00%> (ø)
slack_bolt/__init__.py 100.00% <0.00%> (ø)
slack_bolt/async_app.py 100.00% <0.00%> (ø)
slack_bolt/app/__init__.py 100.00% <0.00%> (ø)
... and 128 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 149c354...26f1a30. Read the comment docs.

@seratch seratch merged commit fa4988f into slackapi:main Jul 14, 2021
@seratch seratch deleted the issue-378-middleware-errors branch July 14, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle exceptions raised in middleware
1 participant