-
-
Notifications
You must be signed in to change notification settings - Fork 577
Add support for extensions to the HTTP protocol #3461
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
Add support for extensions to the HTTP protocol #3461
Conversation
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 @omarzouk - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3461 +/- ##
==========================================
- Coverage 95.06% 95.01% -0.06%
==========================================
Files 508 508
Lines 33018 33072 +54
Branches 1712 1719 +7
==========================================
+ Hits 31388 31422 +34
- Misses 1356 1370 +14
- Partials 274 280 +6 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #3461 will not alter performanceComparing Summary
|
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.
Thanks for the PR! Can you add a test to ensure the exension dict is actually correctly passed down the chain? 🙂
Additionally, we can investigate merging the http protocol extensions with subscription extensions (need to double check if we support this already in strawberry). It might also make sense to add the protocol extensions to Info
@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 2- Extend the definition of the get_context function, add a 3rd parameter called 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 What do u think? should I go ahead and do Option 1? |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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 SourceryThis 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 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 @omarzouk - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
pinging @patrick91 as well, since we discussed this originally in #3224 |
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.
Some comments, but I'm not totally aware of this, so other @strawberry-graphql/core will be able to do a better review :)
strawberry/types/context_wrapper.py
Outdated
@dataclass | ||
class ContextWrapper: | ||
context: Optional[Any] | ||
extensions: Optional[Dict[str, Any]] |
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.
Same suggestion as above
extensions: Optional[Dict[str, Any]] | |
extensions: Optional[Dict[str, Any]] = None |
Co-authored-by: Thiago Bellini Ribeiro <[email protected]>
Co-authored-by: Thiago Bellini Ribeiro <[email protected]>
Co-authored-by: Thiago Bellini Ribeiro <[email protected]>
for more information, see https://pre-commit.ci
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 |
@patrick91 Hi Patrick! I talked with our management about it, it took a while to get done because github's invoice system for sponsorships is a big mess, but Soundtrack Technologies is now a 🍓 Sponsor! you guys built an awesome library, we use it for a lot of our apps, so it makes all the sense in the world to help you maintain it! |
@omarzouk looks there's an alpha version of GraphQl-core which makes passing data around much easier, would you be ok with using their alpha? |
yea sure, building towards the future setup of |
hi @patrick91 what is the status of this now? I see graphql-core have implemented some support for it in the alpha version. Are we working on something in Strawberry so we can make use of the new capability? |
@omarzouk I'll take a look at this tomorrow! sorry! |
2dd8a98
to
73dca8d
Compare
600f07e
to
43e274e
Compare
/pre-release |
@omarzouk I think we (@bellini666) got a nice implementation, would be interested in trying the pre-release? 😊 |
Pre-release👋 Releasing commit [832ad16] to PyPi as pre-release! 📦 |
Pre-release👋 Pre-release 0.269.0.dev.1747164009 [832ad16] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.269.0.dev.1747164009 |
@@ -113,6 +113,11 @@ def context(self) -> ContextType: | |||
"""The context passed to the query execution.""" | |||
return self._raw_info.context | |||
|
|||
@property | |||
def input_extensions(self) -> dict[str, Any]: |
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.
@patrick91 First of all, thanks a lot for taking the time to do it! I tested it in one of our APIs, the data does go through all the way! However it is returned as a string and not as a dict, so we need to json.loads
the data before we return it from this function. The spec states that the parameter is expected to be a map so parsing it should be safe to do.
Edit: I was wrong, its actually correct
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.
@patrick91 actually nvm I was wrong, I was wrongly escaping the input in my curl request, totally my bad. The implementation is actually correct! and I can confirm it works end to end 👍
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.
@omarzouk awesome! I'll release this after we fix the unrelated issue with GlobalID :D
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.
The pre-release works perfectly, thanks a lot guys! ❤️
Thanks for contributing to Strawberry! 🎉 You've been invited to join You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67 And don't forget to join our discord server: https://strawberry.rocks/discord 🔥 |
Input extension information only being available from the Are there any plans to make this info available to the various extension lifecycle hook methods? |
@mdgilene can you open an issue with an example use case? We'll try to make it happen 😊 |
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 theInfo
context getter. A new function is also added to theInfo
class calledinput_extensions
. It exposes the extensions data.Example usage by the end-user:
Types of Changes
Issues Fixed or Closed by This PR
extensions
Request Parameter for the HTTP protocol #3224Checklist
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.