Unify single-response-requests with streaming requests on `graphql-transport-ws` #1792

kristjanvalur · 2022-04-04T10:55:21Z

(replaces closed #1739)

Various problems with single-response-requests existed as a result of their having a separate control
path from streaming requests. Single-response-requests are really just special cases of streaming requests
that deliver at most one Next message and need to behave as such.

Description

single-response-request are now handled as tasks and can be overlapped with other requests
on the same connection, can be cancelled, and share the same unique requests ID namespace.

Types of Changes

Issues Fixed or Closed by This PR

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

test overlapped single result queries

Single result operations are now executed on a task. They can be cancelled like regular streamed operations. They can be overlapped like regular streamed operations. They don't have an associated generator, merely a task.

codecov · 2022-04-04T10:57:54Z

Codecov Report

Merging #1792 (27694f3) into main (c775fd1) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1792      +/-   ##
==========================================
+ Coverage   98.11%   98.15%   +0.04%     
==========================================
  Files         139      139              
  Lines        5250     5260      +10     
  Branches      957      958       +1     
==========================================
+ Hits         5151     5163      +12     
+ Misses         53       52       -1     
+ Partials       46       45       -1

…ions

botberry · 2022-04-04T14:02:47Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This release fixes a number of problems with single-result-operations over
graphql-transport-ws protocol

operation IDs now share the same namespace as streaming operations
meaning that they cannot be reused while the others are in operation
single-result-operations now run as tasks meaning that messages related
to them can be overlapped with other messages on the websocket.
single-result-operations can be cancelled with the complete message.
IDs for single result and streaming result operations are now released
once the operation is done, allowing them to be re-used later, as well as
freeing up resources related to previous requests.

Here's the preview release card for twitter:

Here's the tweet text:

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

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

.pre-commit-config.yaml

strawberry/subscriptions/protocols/graphql_transport_ws/handlers.py

BryceBeagle · 2022-04-11T02:00:01Z

tests/aiohttp/test_graphql_transport_ws.py

@@ -285,6 +285,55 @@ async def test_duplicated_operation_ids(aiohttp_client):
        assert data.extra == "Subscriber for sub1 already exists"


+async def test_reused_operation_ids(aiohttp_client):


As these are fairly complicated tests, could you add some docstrings describing what functionality is being tested

Added docstrings.
It is a bit annoying that we have three identical sets of tests, testing internal functionality, that is in theory quite independent of the integration in use. Would it make sense to separate "integration" tests from unit tests?

cc @patrick91, I think we were discussing ideas for doing this last week

@kristjanvalur totally, I've opened up an issue for this: #1809

patrick91

this looks great! thanks so much and sorry for the late review!

patrick91 · 2022-04-14T23:26:55Z

strawberry/subscriptions/protocols/graphql_transport_ws/handlers.py

+        if operation_type == OperationType.SUBSCRIPTION:
+            result_source = await self.schema.subscribe(
+                query=message.payload.query,
+                variable_values=message.payload.variables,
+                operation_name=message.payload.operationName,
+                context_value=context,
+                root_value=root_value,
+            )
+        else:
+            # create AsyncGenerator returning a single result
+            async def get_result_source():
+                yield await self.schema.execute(
+                    query=message.payload.query,
+                    variable_values=message.payload.variables,
+                    context_value=context,
+                    root_value=root_value,
+                    operation_name=message.payload.operationName,
+                )
+
+            result_source = get_result_source()


maybe in a future PR we can move this to a classmethod to clean this method 😊

kristjanvalur added 5 commits April 4, 2022 10:28

Add failing unittest

52db967

More failing unittests

00ca78b

test overlapped single result queries

Add fixes to single-result-operations

3fd991c

Single result operations are now executed on a task. They can be cancelled like regular streamed operations. They can be overlapped like regular streamed operations. They don't have an associated generator, merely a task.

Add remaining unittests files

13cf2b3

Add RELEASE.md

6799c88

kristjanvalur added 2 commits April 4, 2022 12:21

Add fixes and tests for issue strawberry-graphql#1733

ad25a84

Add test for exceptions in single-result-operations

8b3e7ca

kristjanvalur force-pushed the pr04 branch from f2e7026 to 87982df Compare April 4, 2022 12:54

kristjanvalur changed the title ~~Uniri single-response-requests with streaming requests on graphql-transport-ws~~ Unify single-response-requests with streaming requests on graphql-transport-ws Apr 4, 2022

kristjanvalur mentioned this pull request Apr 4, 2022

Fixes to single-response-requests on graphql-transport-ws #1739

Closed

11 tasks

Unify the handling of single-result and streaming subscription operat…

8b44393

…ions

kristjanvalur force-pushed the pr04 branch from 87982df to 8b44393 Compare April 4, 2022 13:17

kristjanvalur added 2 commits April 4, 2022 13:43

Update black version for pre-commit

b381179

Disable coverage for exception cleanup code

75898b9

kristjanvalur force-pushed the pr04 branch from fe47ca8 to 75898b9 Compare April 4, 2022 13:55

kristjanvalur marked this pull request as ready for review April 4, 2022 13:59

botberry added bot:has-release-file labels Apr 4, 2022

kristjanvalur mentioned this pull request Apr 4, 2022

Issue #1731 - Allow complete messages for single-result-operations #1732

Closed

11 tasks

estyxx reviewed Apr 6, 2022

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

BryceBeagle suggested changes Apr 11, 2022

View reviewed changes

Refactor the graphql_transport_ws task toplevel method for clarity

634b076

kristjanvalur force-pushed the pr04 branch from 5e36444 to 634b076 Compare April 11, 2022 13:32

Add docstrings to unittests.

27694f3

patrick91 approved these changes Apr 14, 2022

View reviewed changes

patrick91 merged commit e7dd1ef into strawberry-graphql:main Apr 14, 2022

kristjanvalur deleted the pr04 branch April 15, 2022 11:21

DoctorJohn mentioned this pull request Jul 30, 2022

graphql_transport_ws: Calling complete for a single request operation results in a disconnect. #1731

Closed

This was referenced Jul 30, 2022

graphql-transport-ws handler doesn't free up IDs #1733

Closed

Single-result-operations don't reserve the Subscription ID #1734

Closed

Single-result-operation cannot be overlapped or cancelled #1738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify single-response-requests with streaming requests on `graphql-transport-ws` #1792

Unify single-response-requests with streaming requests on `graphql-transport-ws` #1792

kristjanvalur commented Apr 4, 2022

codecov bot commented Apr 4, 2022 •

edited

Loading

botberry commented Apr 4, 2022 •

edited

Loading

BryceBeagle Apr 11, 2022

kristjanvalur Apr 11, 2022

kristjanvalur Apr 11, 2022

BryceBeagle Apr 11, 2022

patrick91 Apr 14, 2022

patrick91 left a comment

patrick91 Apr 14, 2022

		@@ -285,6 +285,55 @@ async def test_duplicated_operation_ids(aiohttp_client):
		assert data.extra == "Subscriber for sub1 already exists"


		async def test_reused_operation_ids(aiohttp_client):

Unify single-response-requests with streaming requests on graphql-transport-ws #1792

Unify single-response-requests with streaming requests on graphql-transport-ws #1792

Conversation

kristjanvalur commented Apr 4, 2022

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

codecov bot commented Apr 4, 2022 • edited Loading

Codecov Report

botberry commented Apr 4, 2022 • edited Loading

BryceBeagle Apr 11, 2022

Choose a reason for hiding this comment

kristjanvalur Apr 11, 2022

Choose a reason for hiding this comment

kristjanvalur Apr 11, 2022

Choose a reason for hiding this comment

BryceBeagle Apr 11, 2022

Choose a reason for hiding this comment

patrick91 Apr 14, 2022

Choose a reason for hiding this comment

patrick91 left a comment

Choose a reason for hiding this comment

patrick91 Apr 14, 2022

Choose a reason for hiding this comment

Unify single-response-requests with streaming requests on `graphql-transport-ws` #1792

Unify single-response-requests with streaming requests on `graphql-transport-ws` #1792

codecov bot commented Apr 4, 2022 •

edited

Loading

botberry commented Apr 4, 2022 •

edited

Loading