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

Add missing listener suggestion to the default unhandled error message #323

Merged
merged 2 commits into from
May 7, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented May 5, 2021

This pull request improves the default warning message for unhandled requests by printing suggested code snippets as below:

$ python examples/socket_mode.py
⚡️ Bolt app is running!

Unhandled request ({'token': 'xxx', 'team_id': 'T111', 'api_app_id': 'A111', 'event': {'client_msg_id': 'xxx', 'type': 'message', 'text': 'Hi there!', 'user': 'U222', 'ts': '1620202537.001600', 'team': 'T111', 'channel': 'C111', 'event_ts': '1620202537.001600', 'channel_type': 'channel'}, 'type': 'event_callback', 'event_id': 'Ev111', 'event_time': 1620202537, 'authorizations': [{'enterprise_id': None, 'team_id': 'T111', 'user_id': 'U222', 'is_bot': True, 'is_enterprise_install': False}], 'is_ext_shared_channel': False, 'event_context': '1-message-T111-C111'})
---
[Suggestion] You can handle this type of event with the following listener function:

@app.event("message")
def handle_message_events(body, logger):
    logger.info(body)

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 the App/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 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.

@seratch seratch added this to the 1.6.0 milestone May 5, 2021
@seratch seratch self-assigned this May 5, 2021
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #323 (b6f52c9) into main (1e18c0d) will increase coverage by 0.01%.
The diff coverage is 93.15%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
slack_bolt/app/app.py 87.61% <ø> (ø)
slack_bolt/app/async_app.py 94.01% <ø> (ø)
slack_bolt/logger/messages.py 90.62% <93.15%> (+2.90%) ⬆️

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 1e18c0d...b6f52c9. Read the comment docs.

Copy link
Member

@stevengill stevengill left a 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?

@seratch
Copy link
Member Author

seratch commented May 7, 2021

@stevengill

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 👍

@seratch seratch force-pushed the missing-listener-suggestion branch from ee6b09a to 092bc51 Compare May 7, 2021 07:40
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.

2 participants