Skip to content

fix: improve the robustness of the payload extract logic#1464

Merged
WilliamBergamin merged 1 commit intomainfrom
make-extraction-more-robust
Mar 19, 2026
Merged

fix: improve the robustness of the payload extract logic#1464
WilliamBergamin merged 1 commit intomainfrom
make-extraction-more-robust

Conversation

@WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Mar 19, 2026

Summary

These changes aim to improve the robustness of the payload extraction logic!

#1447 highlights that in some edge cases our payload extraction logic will fail and raise a TypeError before the RequestVerification middleware can execute.

The ideal solution to this issue is to move the context building from the request initialization to a middleware that runs immediately after RequestVerification middleware. But this would constitute a breaking change.

This PR aims to introduce none breaking changes that make the context building more robust and prevents errors from raising before RequestVerification middleware executes.

Testing

Unit tests should be sufficient, but feel free to spin up an app tests some edge cases

Category

  • 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

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.

@WilliamBergamin WilliamBergamin added this to the 1.27.1 milestone Mar 19, 2026
@WilliamBergamin WilliamBergamin self-assigned this Mar 19, 2026
@WilliamBergamin WilliamBergamin requested a review from a team as a code owner March 19, 2026 15:09
@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.47%. Comparing base (f1bc61f) to head (4f2a1fd).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
slack_bolt/request/internals.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1464      +/-   ##
==========================================
+ Coverage   91.43%   91.47%   +0.04%     
==========================================
  Files         232      232              
  Lines        7334     7334              
==========================================
+ Hits         6706     6709       +3     
+ Misses        628      625       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@WilliamBergamin Super nice fix with minimal changes! 🌟

I'm also curious to improving the middleware sequences but let's keep discussing this ongoing. It's a promising pattern I think!


for _, payload in invalid_payloads.items():
# We only verify no TypeError/AttributeError is raised and that functions which
# would try to subscript the string value return None instead of crashing.
Copy link
Member

Choose a reason for hiding this comment

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

🧪 praise: These are solid test cases!

🔬 suggestion(non-blocking): Would assertions also that these values are None be useful?

Copy link
Contributor Author

@WilliamBergamin WilliamBergamin Mar 19, 2026

Choose a reason for hiding this comment

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

Yess I would of liked to get some asserts in there but some of these payloads are actually valid and won't result in null, the goal of the test it to make sure we don't raise an errors

I hope the comment is sufficient 🙏 we can always improve the test in a follow up PR 🚀

@WilliamBergamin WilliamBergamin merged commit 7aa415f into main Mar 19, 2026
16 checks passed
@WilliamBergamin WilliamBergamin deleted the make-extraction-more-robust branch March 19, 2026 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants