strawberry
strawberry copied to clipboard
Add support for extensions to the HTTP protocol
Description
The GQL spec specifies a field called extensions as part of the request parameters spec in the HTTP protocol. We are missing support for this
Adding the field to be parsed and propagating it as part of the Info object. This is done by wrapping the entire context object in a temporary container, and then unwrapping it inside the Info context getter. A new function is also added to the Info class called input_extensions. It exposes the extensions data.
Example usage by the end-user:
@strawberry.field
def value_from_extensions(self, key: str, info: strawberry.Info) -> str:
return info.input_extensions[key]
Types of Changes
- [X] Core
- [ ] Bugfix
- [X] New feature
- [ ] Enhancement/optimization
- [X] Documentation
Issues Fixed or Closed by This PR
- fixes https://github.com/strawberry-graphql/strawberry/issues/3224
Checklist
- [x] My code follows the code style of this project.
- [x] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the CONTRIBUTING document.
- [x] I have added tests to cover my changes.
- [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).
Summary by Sourcery
This pull request introduces support for the 'extensions' field in HTTP requests, allowing it to be parsed and included in the ExecutionContext. This change is accompanied by updates to the documentation and new tests to ensure proper functionality.
- New Features:
- Added support for the 'extensions' field in HTTP requests, allowing it to be parsed and propagated as part of the ExecutionContext.
- Documentation:
- Updated documentation to reflect the new support for the 'extensions' field in HTTP requests.
- Tests:
- Added tests to verify the correct handling and propagation of the 'extensions' field in both GET and POST HTTP requests.
Hi, thanks for contributing to Strawberry 🍓!
We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!
So as soon as this PR is merged, a release will be made 🚀.
Here's an example of RELEASE.md:
Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.
Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)
Here's the tweet text:
🆕 Release (next) is out! Thanks to Omar Marzouk for the PR 👏
Get it here 👉 https://strawberry.rocks/release/(next)
Hi, thanks for contributing to Strawberry 🍓!
We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!
So as soon as this PR is merged, a release will be made 🚀.
Here's an example of RELEASE.md:
Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.
Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)
Here's the tweet text:
🆕 Release (next) is out! Thanks to Omar Marzouk for the PR 👏
Get it here 👉 https://strawberry.rocks/release/(next)
Codecov Report
Attention: Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.
Project coverage is 96.21%. Comparing base (
581f508) to head (82708d7).
Additional details and impacted files
@@ Coverage Diff @@
## main #3461 +/- ##
==========================================
- Coverage 96.22% 96.21% -0.01%
==========================================
Files 526 527 +1
Lines 33999 34037 +38
Branches 5610 5621 +11
==========================================
+ Hits 32714 32750 +36
Misses 1048 1048
- Partials 237 239 +2
CodSpeed Performance Report
Merging #3461 will not alter performance
Comparing omarzouk:add_http_extensions (82708d7) with main (581f508)
Summary
✅ 13 untouched benchmarks
@erikwrede Hi! yea I think I need to do a fresh take on this, I think I hit a dead end with my current approach, I've been investigating how to do it, and I think we have a couple options:
1- Wrap the context field in another object that holds the extensions and the context, i.e. wrap the field before this line, then unwrap it inside the Info class. We would then introduce an input_extensions or protocol_extensions field to Info that reads the extensions from the wrapper.
2- Extend the definition of the get_context function, add a 3rd parameter called extensions. However, this would require moving the body parsing and data fetching to happen before the get_context is called, i.e. this operation would have to move above this line. It will also require some refactoring to get a similar pattern to work with subscriptions.
The benefit of doing Option 1 would be that we can add this right away to http, without having to modify other protocols, and just include a type check for the wrapper in the info.context getter. It can then be added to other protocols over time, in other PRs.
What do u think? should I go ahead and do Option 1?
hi @erikwrede, I went ahead and implemented Option 1 mentioned above, which is to wrap the context object and unwrap it in the Info getter. I also added a new test that is passing successfully. Please let me know what you think. 🙏
Reviewer's Guide by Sourcery
This pull request adds support for the 'extensions' field in the HTTP protocol for GraphQL requests. The changes involve parsing the 'extensions' field from the request, propagating it through the ExecutionContext, and making it accessible to applications. Additionally, tests have been added to ensure the new functionality works correctly.
File-Level Changes
| Files | Changes |
|---|---|
tests/http/clients/base.pytests/http/clients/channels.pytests/http/clients/aiohttp.pytests/http/clients/asgi.pytests/http/clients/chalice.pytests/http/clients/django.pytests/http/clients/fastapi.pytests/http/clients/flask.pytests/http/clients/litestar.pytests/http/clients/quart.pytests/http/clients/sanic.pytests/http/clients/starlite.py |
Added 'extensions' parameter to various HTTP client request functions and updated request body construction to include 'extensions'. |
strawberry/http/async_base_view.pystrawberry/http/sync_base_view.py |
Wrapped context in ContextWrapper to include 'extensions' in execute_operation and parse_http_body functions. |
Tips
- Trigger a new Sourcery review by commenting
@sourcery-ai reviewon the pull request. - You can change your review settings at any time by accessing your dashboard:
- Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
- Change the review language;
- You can always contact us if you have any questions or feedback.
pinging @patrick91 as well, since we discussed this originally in https://github.com/strawberry-graphql/strawberry/issues/3224
Hello! apart from the extra check for list in the parameter parsing which is potentially redundant and might be removed, as I understand it, that change is also not really blocking for this PR, right? https://github.com/strawberry-graphql/strawberry/pull/3558 could be updated to remove the list parsing for extensions after this one is merged?
Is there any other comments regarding this PR? the approach in general around wrapping context and exposing the extra field in the Info object? how would you like me to proceed?
@bellini666 @DoctorJohn @erikwrede @patrick91
Hi! I have merged main to include https://github.com/strawberry-graphql/strawberry/pull/3558 and removed the extra list type check for the extensions param. Anything else you guys would like me to change about this PR? if not I can should I create a release file?
@bellini666 @DoctorJohn @erikwrede @patrick91
hello! sorry to keep pinging, we are really eager to use this in our APIs, do u think its possible to get some traction on it? I'm happy to help any way I can @patrick91 🙏
@omarzouk sure, I think I'll have time this week(end) (I'm currently travelling) 😊
And please consider sponsoring us, so we can prioritise issues better ;)
@omarzouk an update on this, I think I found a nice way to pass the request extensions to info, but it is quite different from this, so I might try it in another PR