-
Notifications
You must be signed in to change notification settings - Fork 248
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
Add missing listener suggestion to the default unhandled error message #323
Conversation
Codecov Report
@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 91.46% 91.48% +0.01%
==========================================
Files 167 167
Lines 5298 5369 +71
==========================================
+ Hits 4846 4912 +66
- Misses 452 457 +5
Continue to review full report at Codecov.
|
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.
LGTM. I really like that you are showing these awesome code samples
Only question I have is that we are logging req.body
for unhandled requests. In Bolt-js, usually we have stayed away from logging this in case folks are piping logs to some system and don't want sensitive information such as token
in their logs. Are you concerned about that?
Thanks, this is a good point. Basically, I don't think logging request payloads is a problem as payloads do not have any access tokens and credentials. Payloads can include the legacy verification token (we don't recommend using anymore and Bolt relies on only signature verification) and confidential data in text messages. I don't think we call these data as credentials. For this reason, I think logging the payload in debug-level logging does not have any issues as we already do in Bolt for JS's Developer Mode. With that being said, this logging is warning-level and it's not easy to omit only the log if a developer has to eliminate the possibility of including confidential information in product app logs. I will adjust the log message for this warning-level log and will add another debug level log for knowing the whole payload (in the development phase). Thanks for the great input 👍 |
ee6b09a
to
092bc51
Compare
092bc51
to
b6f52c9
Compare
…missing (improvement to slackapi#323)
This pull request improves the default warning message for unhandled requests by printing suggested code snippets as below:
With this suggestion, developers, particularly the ones new to bolt-python, can easily understand what to do for handling the requests in their app. This pull request covers all the possible patterns with corresponding code snippets. Refer to the added unit test code (
tests/slack_bolt/logger/test_unmatched_suggestions.py
) to learn the actual suggestion messages.If a developer does not want to have the message, we would recommend having
raise_error_for_unhandled_request=True
in theApp
/AsyncApp
constructor arguments and having their own global error handler (see "Customize Unhandled Error Handling" in the v1.5.0 release notes for more details).Category (place an
x
in each of the[ ]
)slack_bolt.App
and/or its core componentsslack_bolt.async_app.AsyncApp
and/or its core componentsslack_bolt.adapter
/docs
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.
./scripts/install_all_and_run_tests.sh
after making the changes.