-
-
Notifications
You must be signed in to change notification settings - Fork 525
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
Check multipart subscriptions using accept header #3627
Conversation
Reviewer's Guide by SourceryThis pull request updates the handling of multipart subscriptions to align with the latest changes in the specification. The main change is to use the 'Accept' header instead of the 'Content-Type' header to determine if a request is a multipart subscription. 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 @patrick91 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider optimizing the header processing by only lowercasing the 'accept' header instead of all headers.
- It might be beneficial to add more test cases to cover different scenarios for multipart subscriptions.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue 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.
@@ -64,7 +64,8 @@ async def test_multipart_subscription( | |||
method=method, | |||
query='subscription { echo(message: "Hello world", delay: 0.2) }', | |||
headers={ | |||
"content-type": "multipart/mixed;boundary=graphql;subscriptionSpec=1.0,application/json", | |||
"accept": "multipart/mixed;boundary=graphql;subscriptionSpec=1.0,application/json", | |||
"content-type": "application/json", | |||
}, |
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.
suggestion (testing): Update test to use 'accept' header instead of 'content-type' for multipart subscriptions
The test has been updated to reflect the changes in the main code, where multipart subscriptions are now determined by the 'accept' header instead of the 'content-type'. This is a good change, but we should consider adding more test cases to cover different scenarios.
Thanks for adding the Here's a preview of the changelog: This release fixes how we check for multipart subscriptions to be Here's the tweet text:
|
/pre-release |
Pre-release👋 Releasing commit [d235ddd] to PyPi as pre-release! 📦 |
Pre-release👋 Pre-release 0.240.3.dev.1726159932 [d235ddd] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.240.3.dev.1726159932 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3627 +/- ##
==========================================
- Coverage 96.75% 96.75% -0.01%
==========================================
Files 521 521
Lines 33715 33714 -1
Branches 5622 5622
==========================================
- Hits 32622 32620 -2
- Misses 861 863 +2
+ Partials 232 231 -1 |
CodSpeed Performance ReportMerging #3627 will not alter performanceComparing Summary
|
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Update the multipart subscription detection logic to use the 'accept' header instead of the 'content-type' header, ensuring compliance with the latest specification. Adjust the corresponding test to reflect this change and document the fix in the release notes.
Bug Fixes:
Documentation:
Tests: