Support subscriptions extensions #3554

nrbnlulu · 2024-07-02T06:24:19Z

Description

Support extensions in subscriptions operations.
Thing to note is that GraphQL core ~3.2 doesn't support middlewares (which are used by our schema extension resolve) on subscription. Support for this was added in 876aef6.
We can initially remove resolve support until we upgrade to 3.3 Please LMK what you guys think.

PS: There are few tests that are not passing since I upgraded GraphQL-core (for the midleware support) and there are some incompatibilities (mostly due to spaces in the SDL).

Types of Changes

Issues Fixed or Closed by This PR

fix #2097
closes #3598

TODO

fix integrations tests
test extensions hooks for many yields
test first yield is an error
test that extensions results are cleaned between yields
remove deprecation's about execution context. we'll do that in another pr
resolve optimization TODO's after review is accepted.

Things I am not certain about

on_execute was chosen the mitigate the normal on_execute per subscription message.
The thing is after the last yield of the subscription we still wrap with on_execution causing a last on_execution lifetime although the async gen raised StopAsyncIteration I am not sure this is the wanted behavior.
Currently I clear ExecutionContext.extensions_result on every yield do you think this is the wanted behaviour?

Summary by Sourcery

This pull request introduces support for schema extensions in subscription operations, allowing hooks to be executed for each yield in a subscription. It also includes significant refactoring to improve error handling and performance, along with comprehensive tests and updated documentation.

New Features:
- Added support for schema extensions in subscription operations, allowing hooks like on_execute to be called for each subscription yield.
Enhancements:
- Refactored the execution and subscription handling to use internal types instead of graphql-core result types.
- Improved error handling in subscription operations to differentiate between immediate errors and execution errors.
- Introduced caching for extensions and middleware manager in the schema to optimize performance.
Documentation:
- Updated documentation to include examples and explanations for using schema extensions with subscriptions.
Tests:
- Added comprehensive tests for subscription extensions, including tests for immediate errors, errors after yields, and extension results clearing between yields.
- Refactored existing tests to align with the new subscription handling and error processing logic.

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

…ons' into support_extensions_on_subscriptions

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

codecov · 2024-07-02T06:30:23Z

Codecov Report

Attention: Patch coverage is 98.70130% with 7 lines in your changes missing coverage. Please review.

Project coverage is 96.77%. Comparing base (fc25e04) to head (56e0a6f).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3554      +/-   ##
==========================================
- Coverage   96.79%   96.77%   -0.03%     
==========================================
  Files         517      521       +4     
  Lines       33517    33731     +214     
  Branches     5570     5627      +57     
==========================================
+ Hits        32444    32644     +200     
- Misses        845      855      +10     
- Partials      228      232       +4

codspeed-hq · 2024-07-02T07:05:57Z

CodSpeed Performance Report

Merging #3554 will degrade performances by 80%

_{Comparing nrbnlulu:support_extensions_on_subscriptions (56e0a6f) with main (fc25e04)}

Summary

❌ 1 regressions
✅ 14 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`nrbnlulu:support_extensions_on_subscriptions`	Change
❌	`test_subscription`	43.3 ms	216.3 ms	-80%

DoctorJohn

Just finished reviewing the websocket protocol parts. Looks like readability went up in some places and we still follow the protocol specs 🎉 I left some minor comments and my approval because at its core this PR is pretty much good to go.

strawberry/subscriptions/protocols/graphql_ws/handlers.py

DoctorJohn · 2024-08-13T21:58:17Z

tests/websockets/test_graphql_transport_ws.py

+    if extensions is not None:
+        assert response["payload"]["extensions"] == extensions


Would it be assert response["payload"]["extensions"] == extensions or {} because extensions, if set, would be a map? I would prefer including the or {}, because that should technically be a valid value as well.

tests/websockets/test_graphql_transport_ws.py

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

bellini666

One last review round. After this one I'm ready to actually approve this

strawberry/schema/schema.py

bellini666 · 2024-08-18T12:21:23Z

strawberry/schema/schema.py


-        if self.directives:
-            extensions.append(DirectivesExtensionSync if sync else DirectivesExtension)
+    def _get_middleware_manager(


The same suggestion as @patrick91 made applies for this. You can probably move this to a cached_property called middleware_manager, then you don't need to handle caching by yourself

This one is kinda trickier because extensions are not just a boolean argument. I think the current approuch suffice.
LMK if thats a blocker, I'll think of something better.

bellini666 · 2024-08-18T12:34:53Z

strawberry/schema/subscribe.py

+        yield first
+        async for result in asyncgen:
+            yield result


Wondering what happens if this gets cancelled, specially with asyncio.CancelledError, maybe the asyncgen would not be closed?

Maybe:

Suggested change

yield first

async for result in asyncgen:

yield result

try:

yield first

async for result in asyncgen:

yield result

finally:

with suppress(RuntimeError):

await asyncgen.aclose()

or to do that specifically for cancellederror:

Suggested change

yield first

async for result in asyncgen:

yield result

try:

yield first

async for result in asyncgen:

yield result

except asyncio.CancelledError:

with suppress(RuntimeError):

await asyncgen.aclose()

raise

Can you think of a cenario that this would happend? I'll add a test for it.

Wondering what happens if this gets cancelled, specially with asyncio.CancelledError, maybe the asyncgen would not be closed?

I'm pretty sure the async generator would also live in the canceled task and, therefore, be automatically canceled as well.

bellini666 · 2024-08-18T12:38:58Z

strawberry/schema/subscribe.py

+    # To overcome this while maintaining the extension contexts we do this trick.
+    first = await asyncgen.__anext__()
+    if isinstance(first, PreExecutionError):
+        await asyncgen.aclose()


This is going to raise RuntimeError in case the generator is already closed

Suggested change

await asyncgen.aclose()

with suppress(RuntimeError):

await asyncgen.aclose()

Why would it be though?

bellini666 · 2024-08-18T12:45:20Z

tests/websockets/test_graphql_transport_ws.py

+    if extensions is not None:
+        assert response["payload"]["extensions"] == extensions


Not sure I understand. how would that differ from what it does RN?

What you have right now will not check what response["payload"]["extensions"] is at all

My suggestion (and I think @DoctorJohn is saying the same) is to also check it when extensions is None, to make sure that it contains an empty list/dict/whatever would be returned in that case

tests/websockets/test_graphql_ws.py

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

…subscriptions

…cute hook

patrick91

let's go!

nrbnlulu and others added 8 commits June 4, 2023 18:59

Support subscriptions extensions

2c534a1

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

6525ee9

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

Add RELEASE.md

b92c51b

Merge remote-tracking branch 'origin/support_extensions_on_subscripti…

01a76cd

…ons' into support_extensions_on_subscriptions

mypy fixes.

6ff3315

remove positional only / (py3.7)

20bcfed

merge main

9ba707a

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

f69d746

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

reverts

060c719

nrbnlulu added 11 commits July 2, 2024 11:40

intial subscription test pass

62a130b

wip. pass manager to graphql core

7b45bfa

wip: migrate to new graphql core

e1db94b

don't use supports resolve

70d2f4c

revert

7f44c77

revert unneeded changes

dc32183

restore tests; inject execution_context on operation

5701d21

don't use class; depecate

9dcb3fc

wip: ensure execution context is distinct.

7e1c770

wip: first inital successful run after refactor

6a84415

wip: more tests pass

222a4e5

DoctorJohn self-requested a review July 4, 2024 21:17

nrbnlulu added 7 commits July 5, 2024 11:35

better release.md

4a0f1be

all previous extensions tests pass

8fa0cec

update release.md

fc7977b

improve tests readability

81914a2

test_subscription_success_many_fields pass

89e501b

test_subscription_first_yields_error

29b5db8

test_extensino_results_are_cleared_between_yields

1438390

DoctorJohn approved these changes Aug 13, 2024

View reviewed changes

nrbnlulu mentioned this pull request Aug 14, 2024

GraphQL Core 3.3 tests appear to be running 3.2 instead #3598

Closed

nrbnlulu and others added 2 commits August 15, 2024 12:04

rectify @DoctorJohn review

66b0266

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

ee1ee1c

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

bellini666 reviewed Aug 18, 2024

View reviewed changes

nrbnlulu and others added 8 commits August 18, 2024 16:35

rectify @bellini666 comments

fd6bdaf

Merge branch 'main' into support_extensions_on_subscriptions

26f8437

Improve protocol handling for subscriptions

835d1f4

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

9ce6ff0

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

Lint

da3b12a

Restore tests

7af8bc9

Update wrong return type

3ba6a83

Merge branch 'feature/multiple-protocols' into support_extensions_on_…

83061ed

…subscriptions

patrick91 mentioned this pull request Sep 2, 2024

Improve support for multipart subscriptions #3612

Closed

patrick91 added 2 commits September 2, 2024 19:40

Update doc

b65322e

Temporarily skip on_execute, see strawberry-graphql#3613

87fd45d

patrick91 mentioned this pull request Sep 2, 2024

Hook for new results in subscription (and also on defer/stream) #3613

Open

patrick91 added 3 commits September 2, 2024 20:01

Update multipart tests

5d37f83

Fix bad merge

cbd7c90

Fix type

ea1e4fd

patrick91 mentioned this pull request Sep 4, 2024

Add support for multipart subscriptions #3076

Merged

6 tasks

nrbnlulu and others added 4 commits September 7, 2024 22:42

remove unneeded async gen wrapper since we removed support for on_exe…

fdc2e4e

…cute hook

Merge branch 'main' into support_extensions_on_subscriptions

9e0d0aa

Update release notes

27b626b

Add tweet.md

56e0a6f

patrick91 approved these changes Sep 10, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

patrick91 merged commit 0dcf23d into strawberry-graphql:main Sep 10, 2024
103 of 105 checks passed

kkom mentioned this pull request Sep 19, 2024

Misleading static type for "execution result" attribute (introduced in 0.240.0) #3635

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support subscriptions extensions #3554

Support subscriptions extensions #3554

nrbnlulu commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

codspeed-hq bot commented Jul 2, 2024 •

edited

Loading

DoctorJohn left a comment •

edited

Loading

DoctorJohn Aug 13, 2024

bellini666 left a comment

bellini666 Aug 18, 2024

nrbnlulu Aug 18, 2024

bellini666 Aug 18, 2024

nrbnlulu Aug 18, 2024

DoctorJohn Sep 10, 2024

bellini666 Aug 18, 2024

nrbnlulu Aug 18, 2024

bellini666 Aug 18, 2024

patrick91 left a comment

This comment was marked as duplicate.

This comment was marked as duplicate.

		if extensions is not None:
		assert response["payload"]["extensions"] == extensions

	await asyncgen.aclose()
	with suppress(RuntimeError):
	await asyncgen.aclose()

Support subscriptions extensions #3554

Support subscriptions extensions #3554

Conversation

nrbnlulu commented Jul 2, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

TODO

Things I am not certain about

Summary by Sourcery

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Jul 2, 2024 • edited Loading

Merging #3554 will degrade performances by 80%

Summary

Benchmarks breakdown

DoctorJohn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellini666 left a 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

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

patrick91 left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

nrbnlulu commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

codspeed-hq bot commented Jul 2, 2024 •

edited

Loading

DoctorJohn left a comment •

edited

Loading