fix: improve the robustness of the payload extract logic#1464
fix: improve the robustness of the payload extract logic#1464WilliamBergamin merged 1 commit intomainfrom
Conversation
Codecov Report❌ Patch coverage is
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. |
zimeg
left a comment
There was a problem hiding this comment.
@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. |
There was a problem hiding this comment.
🧪 praise: These are solid test cases!
🔬 suggestion(non-blocking): Would assertions also that these values are None be useful?
There was a problem hiding this comment.
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 🚀
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
TypeErrorbefore theRequestVerificationmiddleware 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
RequestVerificationmiddleware. 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
RequestVerificationmiddleware executes.Testing
Unit tests should be sufficient, but feel free to spin up an app tests some edge cases
Category
slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements
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.shafter making the changes.