-
-
Notifications
You must be signed in to change notification settings - Fork 543
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 incorrect query params type #3558
Fix incorrect query params type #3558
Conversation
Reviewer's Guide by SourceryThis pull request simplifies the handling of GET query parameters by removing unnecessary list handling and updating the File-Level Changes
Tips
|
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.
Hey @DoctorJohn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Thanks for adding the Here's a preview of the changelog: This release removes an unnecessary check from our internal GET query parsing logic making it simpler and (insignificantly) faster. Here's the tweet text:
|
Thanks for adding the Here's a preview of the changelog: This release removes an unnecessary check from our internal GET query parsing logic making it simpler and (insignificantly) faster. Here's the tweet text:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3558 +/- ##
==========================================
+ Coverage 96.57% 96.58% +0.01%
==========================================
Files 524 524
Lines 33630 33632 +2
Branches 5578 5577 -1
==========================================
+ Hits 32478 32484 +6
+ Misses 916 914 -2
+ Partials 236 234 -2 |
CodSpeed Performance ReportMerging #3558 will not alter performanceComparing Summary
|
98b3698
to
36da65e
Compare
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.
Thank you! 😊
Description
This is the result of my deep-dive into this comment.
Our GET query parsing method incorrectly assumed that our query parameters could be lists. As a result some of our types were sligtly incorrect and we had an unnecessary block handling the case.
Generally it would be correct that GET query parameters could contain lists (e.g.
?variables={}&variables={}
could be parsed as a list). The sanic integration even contains a warning about this. However, it turns out none of our integrations interprets query parameters which are defined multiple times as lists by default (not even sanic). I manually checked every integration and also added a test to prove this. Also double-checked the graphql http spec to verify the "variables" query parameter must indeed not be a list.(fyi: most of these frameworks provide a
getlist
method which can be used to explicitly interpret multi-defined GET query parameters as a list).Types of Changes
Checklist
Summary by Sourcery
This pull request fixes the incorrect type handling of query parameters in GET requests by ensuring they are not interpreted as lists. It simplifies the internal GET query parsing logic by removing unnecessary checks, adds a test to verify the correct behavior, and includes a release note documenting the changes.