Add support for extensions to the HTTP protocol #3461

omarzouk · 2024-04-19T17:38:08Z

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

Issues Fixed or Closed by This PR

fixes Support the extensions Request Parameter for the HTTP protocol #3224

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
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.

for more information, see https://pre-commit.ci

botberry · 2024-04-19T17:39:37Z

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)

sourcery-ai

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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

strawberry/http/__init__.py

codecov · 2024-04-19T17:44:57Z

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-hq · 2024-04-19T17:45:25Z

CodSpeed Performance Report

Merging #3461 will not alter performance

_{Comparing omarzouk:add_http_extensions (82708d7) with main (581f508)}

Summary

✅ 13 untouched benchmarks

erikwrede

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

omarzouk · 2024-06-24T10:37:44Z

@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?

for more information, see https://pre-commit.ci

omarzouk · 2024-06-24T14:22:34Z

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. 🙏

sourcery-ai · 2024-06-24T14:24:36Z

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.py` `tests/http/clients/channels.py` `tests/http/clients/aiohttp.py` `tests/http/clients/asgi.py` `tests/http/clients/chalice.py` `tests/http/clients/django.py` `tests/http/clients/fastapi.py` `tests/http/clients/flask.py` `tests/http/clients/litestar.py` `tests/http/clients/quart.py` `tests/http/clients/sanic.py` `tests/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.py` `strawberry/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 review on 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.

sourcery-ai

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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

strawberry/types/info.py

strawberry/http/__init__.py

strawberry/types/context_wrapper.py

tests/http/test_query.py

strawberry/http/async_base_view.py

strawberry/http/sync_base_view.py

omarzouk · 2024-06-24T14:31:11Z

pinging @patrick91 as well, since we discussed this originally in #3224

bellini666

Some comments, but I'm not totally aware of this, so other @strawberry-graphql/core will be able to do a better review :)

strawberry/http/__init__.py

strawberry/http/async_base_view.py

strawberry/http/sync_base_view.py

strawberry/types/info.py

bellini666 · 2024-07-06T18:39:54Z

strawberry/types/context_wrapper.py

+@dataclass
+class ContextWrapper:
+    context: Optional[Any]
+    extensions: Optional[Dict[str, Any]]


Same suggestion as above

Suggested change

extensions: Optional[Dict[str, Any]]

extensions: Optional[Dict[str, Any]] = None

tests/views/schema.py

strawberry/http/base.py

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

for more information, see https://pre-commit.ci

omarzouk · 2024-07-07T22:35:40Z

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? #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

omarzouk · 2024-07-16T14:45:58Z

Hi! I have merged main to include #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

patrick91 · 2024-07-16T14:51:03Z

strawberry/types/info.py

+    @property
+    def input_extensions(self) -> Dict[str, Any]:
+        if isinstance(self._raw_info.context, ContextWrapper):
+            return self._raw_info.context.extensions
+        return {}


how come this is called input_extensions? 😊

honestly, I just didn't have a better name 😅 I just felt extensions is used in so many contexts in strawberry and people might be confused as to which one this is. But I'm happy to change it to whatever you think makes most sense

patrick91 · 2024-07-16T14:51:46Z

strawberry/http/sync_base_view.py

+        context_wrapper = ContextWrapper(
+            context=context, extensions=request_data.extensions
+        )


is this context wrapper only to pass request data?

yes exactly. Since the context data comes from get_context and that can be any type of object the user decides to return, this felt like a safe way to attach more data

I'll try to review and merge this this week 😊 thanks for the patience!

thank you so much! 🙏

omarzouk · 2024-08-22T09:27:36Z

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 🙏

patrick91 · 2024-08-22T11:13:52Z

@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 ;)

patrick91 · 2024-08-27T09:09:16Z

@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

omarzouk and others added 2 commits April 19, 2024 19:35

first attempt, introducing new param

e97afee

[pre-commit.ci] auto fixes from pre-commit.com hooks

41fad16

for more information, see https://pre-commit.ci

omarzouk marked this pull request as draft April 19, 2024 17:38

sourcery-ai bot reviewed Apr 19, 2024

View reviewed changes

strawberry/http/__init__.py Show resolved Hide resolved

missing params

cbd138b

strawberry-graphql deleted a comment from botberry Jun 22, 2024

erikwrede reviewed Jun 22, 2024

View reviewed changes

omarzouk and others added 6 commits June 24, 2024 12:55

revert trying to pass to execute

69b35c3

implement Option 1

2841b69

[pre-commit.ci] auto fixes from pre-commit.com hooks

419f2f9

for more information, see https://pre-commit.ci

fix: missing parsing of extensions in params for GET requests

393b976

chore: add new test and adjust http test code to pass the parameter

3eafc92

[pre-commit.ci] auto fixes from pre-commit.com hooks

ae3456b

for more information, see https://pre-commit.ci

omarzouk requested a review from erikwrede June 24, 2024 14:16

omarzouk marked this pull request as ready for review June 24, 2024 14:24

sourcery-ai bot reviewed Jun 24, 2024

View reviewed changes

bellini666 reviewed Jul 6, 2024

View reviewed changes

omarzouk and others added 5 commits July 6, 2024 21:18

Change parameter order in test function

ea1b053

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

Use isinstance

351c48e

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

Use isinstance

494d2b6

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

change to absolute imports

2b16e45

[pre-commit.ci] auto fixes from pre-commit.com hooks

d22c2d9

for more information, see https://pre-commit.ci

DoctorJohn mentioned this pull request Jul 7, 2024

Fix incorrect query params type #3558

Merged

11 tasks

sourcery-ai-experiments-bot mentioned this pull request Jul 8, 2024

Fix incorrect query params type sourcery-ai-experiments/strawberry#13

Open

11 tasks

This was referenced Jul 8, 2024

Fix incorrect query params type sourcery-ai-experiments/strawberry#15

Open

Fix incorrect query params type sourcery-ai-experiments/strawberry#17

Open

omarzouk added 2 commits July 16, 2024 16:37

Merge branch 'main' into add_http_extensions

8f1ce55

remove list check

e3afec3

omarzouk requested review from DoctorJohn and bellini666 July 16, 2024 14:45

patrick91 reviewed Jul 16, 2024

View reviewed changes

omarzouk requested a review from patrick91 July 24, 2024 09:04

Merge branch 'main' into add_http_extensions

82708d7

patrick91 mentioned this pull request Aug 27, 2024

How to pass additional information to GraphQLResolveInfo graphql-python/graphql-core#226

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for extensions to the HTTP protocol #3461

Add support for extensions to the HTTP protocol #3461

omarzouk commented Apr 19, 2024 •

edited

Loading

botberry commented Apr 19, 2024 •

edited

Loading

sourcery-ai bot left a comment

codecov bot commented Apr 19, 2024 •

edited

Loading

codspeed-hq bot commented Apr 19, 2024 •

edited

Loading

erikwrede left a comment •

edited

Loading

omarzouk commented Jun 24, 2024 •

edited

Loading

omarzouk commented Jun 24, 2024

sourcery-ai bot commented Jun 24, 2024 •

edited

Loading

sourcery-ai bot left a comment

omarzouk commented Jun 24, 2024

bellini666 left a comment

bellini666 Jul 6, 2024

omarzouk commented Jul 7, 2024

omarzouk commented Jul 16, 2024

patrick91 Jul 16, 2024

omarzouk Jul 16, 2024

patrick91 Jul 16, 2024

omarzouk Jul 16, 2024

patrick91 Jul 24, 2024

omarzouk Jul 24, 2024

omarzouk commented Aug 22, 2024

patrick91 commented Aug 22, 2024

patrick91 commented Aug 27, 2024

	extensions: Optional[Dict[str, Any]]
	extensions: Optional[Dict[str, Any]] = None

Add support for extensions to the HTTP protocol #3461

Are you sure you want to change the base?

Add support for extensions to the HTTP protocol #3461

Conversation

omarzouk commented Apr 19, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Summary by Sourcery

botberry commented Apr 19, 2024 • edited Loading

sourcery-ai bot left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Apr 19, 2024 • edited Loading

Merging #3461 will not alter performance

Summary

erikwrede left a comment • edited Loading

Choose a reason for hiding this comment

omarzouk commented Jun 24, 2024 • edited Loading

omarzouk commented Jun 24, 2024

sourcery-ai bot commented Jun 24, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

omarzouk commented Jun 24, 2024

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarzouk commented Jul 7, 2024

omarzouk commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarzouk commented Aug 22, 2024

patrick91 commented Aug 22, 2024

patrick91 commented Aug 27, 2024

omarzouk commented Apr 19, 2024 •

edited

Loading

botberry commented Apr 19, 2024 •

edited

Loading

codecov bot commented Apr 19, 2024 •

edited

Loading

codspeed-hq bot commented Apr 19, 2024 •

edited

Loading

erikwrede left a comment •

edited

Loading

omarzouk commented Jun 24, 2024 •

edited

Loading

sourcery-ai bot commented Jun 24, 2024 •

edited

Loading