strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations 🍓
https://strawberry.rocks
MIT License
4.02k stars 533 forks source link

Fix status code for multipart subscriptions #3610

Closed patrick91 closed 2 months ago

patrick91 commented 2 months ago

Summary by Sourcery

Improve the multipart subscription handling by ensuring a default HTTP 200 OK status code is used when not explicitly set, and update tests to verify this behavior.

Bug Fixes:

Tests:

sourcery-ai[bot] commented 2 months ago

Reviewer's Guide by Sourcery

This pull request improves the multipart subscriptions code by addressing the status code handling in the streaming response and adding a test case to verify the status code. It also includes a TODO comment for potential future refactoring.

File-Level Changes

Change Details Files
Ensure a default status code for multipart responses
  • Add fallback to HTTP 200 OK if status_code is not set
  • Update both FastAPI and ASGI implementations
strawberry/fastapi/router.py
strawberry/asgi/__init__.py
Add test case for multipart subscription status code
  • Assert that the response status code is 200
tests/http/test_multipart_subscription.py
Add TODO comment for potential refactoring
  • Suggest renaming 'create_multipart_response' to 'streaming response'
strawberry/fastapi/router.py

Tips - Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - Continue your discussion with Sourcery by replying directly to review comments. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.
botberry commented 2 months ago

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release fixes an issue with the http multipart subscription where the status code would be returned as None, instead of 200.

We also took the opportunity to update the internals to better support additional protocols in future.

Here's the tweet text:

🆕 Release (next) is out! Thanks to @patrick91 for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)
codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.79%. Comparing base (98b8563) to head (c98c4c3). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3610 +/- ## ========================================== + Coverage 96.75% 96.79% +0.03% ========================================== Files 517 517 Lines 33528 33530 +2 Branches 5575 5576 +1 ========================================== + Hits 32441 32455 +14 + Misses 848 847 -1 + Partials 239 228 -11 ```
codspeed-hq[bot] commented 2 months ago

CodSpeed Performance Report

Merging #3610 will not alter performance

Comparing fix/multipart (c98c4c3) with main (98b8563)

Summary

✅ 15 untouched benchmarks

patrick91 commented 2 months ago

~I should probably also fix GET multiparts~

EDIT: that works already