-
-
Notifications
You must be signed in to change notification settings - Fork 577
Add support for @defer
and @stream
#3819
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideIntegrates experimental incremental GraphQL execution by switching to graphql-core’s experimental_execute_incrementally when enabled; adds multipart streaming support in the async HTTP view; extends schema definitions and printer to include @defer and @stream directives; introduces Streamable annotations; and adds comprehensive incremental tests and E2E suites with a dedicated CI workflow. Sequence diagram for incremental execution and streaming HTTP responsesequenceDiagram
participant Client
participant AsyncBaseView
participant Schema
participant experimental_execute_incrementally
participant GraphQLIncrementalExecutionResults
Client->>AsyncBaseView: HTTP GraphQL request
AsyncBaseView->>Schema: execute(...)
Schema->>experimental_execute_incrementally: execute if incremental enabled
experimental_execute_incrementally-->>Schema: GraphQLIncrementalExecutionResults
Schema-->>AsyncBaseView: GraphQLIncrementalExecutionResults
AsyncBaseView->>AsyncBaseView: stream() (yields multipart chunks)
AsyncBaseView-->>Client: multipart/mixed streaming response
ER diagram for BlogPost, Comment, and Author (E2E test schema)erDiagram
BLOGPOST ||--o{ COMMENT : has
COMMENT }o--|| AUTHOR : written_by
BLOGPOST {
ID id
STRING title
STRING content
}
COMMENT {
ID id
STRING content
}
AUTHOR {
ID id
STRING name
}
Class diagram for Streamable and related typesclassDiagram
class StrawberryStreamable
class Streamable {
<<Annotated[AsyncGenerator[T, None], StrawberryStreamable()]>>
}
class AsyncGenerator
class T
Streamable --|> AsyncGenerator : Annotated
Streamable --* StrawberryStreamable : Annotated
Streamable ..> T : generic
Class diagram for incremental execution integration in SchemaclassDiagram
class Schema {
+execute(...)
+execute_sync(...)
+_handle_execution_result(...)
+_parse_and_validate_async(...)
-_schema
-config
}
class StrawberryConfig {
+enable_experimental_incremental_execution: bool
}
class GraphQLIncrementalExecutionResults
class experimental_execute_incrementally
Schema --> StrawberryConfig : config
Schema ..> GraphQLIncrementalExecutionResults : uses
Schema ..> experimental_execute_incrementally : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Apollo Federation Subgraph Compatibility Results
Learn more: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3819 +/- ##
==========================================
- Coverage 94.75% 94.45% -0.30%
==========================================
Files 520 526 +6
Lines 33902 34179 +277
Branches 1754 1782 +28
==========================================
+ Hits 32123 32285 +162
- Misses 1497 1604 +107
- Partials 282 290 +8 🚀 New features to boost your workflow:
|
if isinstance(result, GraphQLIncrementalExecutionResults): | ||
return result |
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.
we need to support this properly
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.
maybe in another PR :)
strawberry/http/async_base_view.py
Outdated
# for Apollo | ||
# content type is `multipart/mixed;deferSpec=20220824,application/json` | ||
"path": ["blogPost"], | ||
"label": "relayTestsBlogPostQuery$defer$relayTestsCommentsFragment", |
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.
this should not be hard-coded, in any case having both label and path seems to be ok for both relay and apollo, so we might want to keep them
tbd if we want to keep the .formatted instead of using our own wrappers
apparently there's no good way to define what spec a client is supporting (Apollo sends deferSpec=20220824
, but for relay it's up to whoever implements the fetch)
if self.config.enable_experimental_incremental_execution: | ||
directives = tuple(directives) + tuple(incremental_execution_directives) |
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.
we need to throw errors if user what experimental execution but we are not on graphql core 3.3+
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.
There are two raise RuntimeError
below, which check that. Maybe it should be here during __init__
instead, and in there just assert
?
# TODO: maybe here use the first result from incremental execution if it exists | ||
if isinstance(result, GraphQLExecutionResult) and result.errors: |
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.
not sure what I meant with that comment :D
but we should probably get rid of the isinstance check?
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.
If I remember it correctly, there was one incremental execution type that did not have errors
in it (not totally sure, it's been a while 😅)
If the idea is just to limit to results that have errors
, maybe hasattr(result, "errors")
would be easier?
@@ -0,0 +1,44 @@ | |||
import contextlib |
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.
I moved this from the previous location
"data": {"character": {"id": "1"}}, | ||
"hasNext": True, | ||
"pending": [{"path": ["character"], "id": "0"}], | ||
# TODO: check if we need this and how to handle it |
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.
we could support these in a future version
CodSpeed Performance ReportMerging #3819 will not alter performanceComparing Summary
|
Pre-release👋 Pre-release 0.276.0.dev.1750672223 [afe7eae] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.276.0.dev.1750672223 |
/pre-release |
…efer`) To optimize the provided code, we can reduce the number of checks by combining related conditionals and accessing properties once instead of multiple times. Furthermore, we can streamline the creation of the `data` dictionary. Here is an optimized version of the code. In this optimized version. - We access `result.errors` and `result.extensions` once and store their values in the `errors` and `extensions` variables, respectively. - We use dictionary unpacking with conditionals to add the "errors" and "extensions" keys to `data` only when they are present. This should provide a minor performance improvement while maintaining the same functionality.
⚡️ Codeflash found optimizations for this PR📄 100% (1.00x) speedup for
|
/pre-release |
…efer`) (#3827) Co-authored-by: codeflash-ai[bot] <148906541+codeflash-ai[bot]@users.noreply.github.com>
This PR is now faster! 🚀 @patrick91 accepted my optimizations from: |
This PR is now faster! 🚀 codeflash-ai[bot] accepted my code suggestion above. |
@matclayton sorry, I totally missed that message! do you know if you can write a small reproduction? or was that fixed with GraphQL-core 3.3.0a7? |
Sorry I didn't follow up here, this is fixed in GraphQL core 3.3.0va7, btw we're running GraphQL-core 3.3.0v7 in prod now, and not seen a single issue so far. Need to still confirm the other bits of this PR, I'm hoping the team can take it off me the next week or so. |
b35cb92
to
876d580
Compare
/pre-release |
this doesn't render/print the directives (to double check) |
worth checking if those directive should be printed, since we don't print directives like |
…fer`) Here is an optimized version. Improvements. - Removed the unnecessary intermediate variables (`errors`, `extensions`) that are only used once. - Avoided repeated dictionary unpacking by building the dictionary with direct assignments and only adding keys if necessary. - Used `if-else` statements for error and extensions blocks for slight speed-ups over dictionary unpacking on small dicts. Rewritten code. This version allocates less intermediate data and does not do work unless necessary.
⚡️ Codeflash found optimizations for this PR📄 12% (0.12x) speedup for
|
for more information, see https://pre-commit.ci
Thanks for adding the Here's a preview of the changelog: This release adds experimental support for GraphQL's Note: this only works when using Strawberry with Features
ConfigurationTo enable these experimental features, configure your schema with: from strawberry.schema.config import StrawberryConfig
schema = strawberry.Schema(
query=Query, config=StrawberryConfig(enable_experimental_incremental_execution=True)
) Here's the tweet text:
|
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 @patrick91 - I've reviewed your changes - here's some feedback:
- The multipart streaming logic in async_base_view has grown quite complex; consider extracting the
stream
generator and multipart encoding into a reusable helper or class to improve readability and maintainability. - In
encode_multipart_data
, computingContent-Length
usinglen(encoded_data)
may be incorrect for non-ASCII content or boundary bytes—consider using the byte length (len(encoded_data.encode('utf-8'))
) or otherwise validating the header length. - There are several
TODO
comments around error handling and result processing in the incremental execution paths—please address or formally track these work items so the core execution flow doesn’t rely on unfinished logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The multipart streaming logic in async_base_view has grown quite complex; consider extracting the `stream` generator and multipart encoding into a reusable helper or class to improve readability and maintainability.
- In `encode_multipart_data`, computing `Content-Length` using `len(encoded_data)` may be incorrect for non-ASCII content or boundary bytes—consider using the byte length (`len(encoded_data.encode('utf-8'))`) or otherwise validating the header length.
- There are several `TODO` comments around error handling and result processing in the incremental execution paths—please address or formally track these work items so the core execution flow doesn’t rely on unfinished logic.
## Individual Comments
### Comment 1
<location> `strawberry/http/async_base_view.py:437` </location>
<code_context>
)
def encode_multipart_data(self, data: Any, separator: str) -> str:
+ encoded_data = self.encode_json(data)
+
</code_context>
<issue_to_address>
The separator is used as a boundary but is hardcoded as '-', which may not be unique.
Consider generating a unique boundary string instead of using '-', to prevent parsing errors if the payload contains the same sequence.
Suggested implementation:
```python
import uuid
boundary = f"----strawberry-{uuid.uuid4().hex}"
headers={
"Transfer-Encoding": "chunked",
"Content-Type": f'multipart/mixed; boundary="{boundary}"',
},
)
```
```python
async def stream() -> AsyncGenerator[str, None]:
yield f"--{boundary}"
```
</issue_to_address>
### Comment 2
<location> `strawberry/http/async_base_view.py:424` </location>
<code_context>
+ sub_response,
+ headers={
+ "Transfer-Encoding": "chunked",
+ "Content-Type": 'multipart/mixed; boundary="-"',
+ },
+ )
</code_context>
<issue_to_address>
The Content-Type boundary value should match the actual boundary used in the body.
Make sure the boundary in the header exactly matches the one used in the multipart body. It's best to use a unique string for the boundary.
Suggested implementation:
```python
boundary = "graphql-subscription-12345"
yield self.encode_multipart_data(response, boundary)
yield f"--{boundary}--\r\n"
return await self.create_streaming_response(
request,
stream,
sub_response,
headers={
"Transfer-Encoding": "chunked",
```
```python
"Content-Type": f'multipart/mixed; boundary={boundary}',
},
)
```
</issue_to_address>
### Comment 3
<location> `strawberry/http/__init__.py:19` </location>
<code_context>
-def process_result(result: ExecutionResult) -> GraphQLHTTPResponse:
- data: GraphQLHTTPResponse = {"data": result.data}
+def process_result(result: ResultType) -> GraphQLHTTPResponse:
+ if isinstance(result, GraphQLIncrementalExecutionResults):
+ return result
</code_context>
<issue_to_address>
Returning 'result' directly when it's a GraphQLIncrementalExecutionResults may break expected return type.
Returning a GraphQLIncrementalExecutionResults instead of a GraphQLHTTPResponse may lead to type inconsistencies for consumers expecting a dictionary-like response.
</issue_to_address>
### Comment 4
<location> `strawberry/annotation.py:339` </location>
<code_context>
return _annotation_is_maybe(annotation)
+ @classmethod
+ def _is_streamable(cls, annotation: Any, args: list[Any]) -> bool:
+ return any(isinstance(arg, StrawberryStreamable) for arg in args)
+
@classmethod
</code_context>
<issue_to_address>
Type checking with isinstance on annotation arguments may not be robust.
Consider checking types directly or using typing.get_origin/get_args, as annotation arguments may not always be instances.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def _is_streamable(cls, annotation: Any, args: list[Any]) -> bool:
return any(isinstance(arg, StrawberryStreamable) for arg in args)
=======
def _is_streamable(cls, annotation: Any, args: list[Any]) -> bool:
from typing import get_origin, get_args
def is_streamable_type(arg):
# Check if the argument is the StrawberryStreamable type or a generic of it
origin = get_origin(arg) or arg
return origin is StrawberryStreamable
return any(is_streamable_type(arg) for arg in args)
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `strawberry/schema/schema.py:534` </location>
<code_context>
-def process_result(result: ExecutionResult) -> GraphQLHTTPResponse:
- data: GraphQLHTTPResponse = {"data": result.data}
+def process_result(result: ResultType) -> GraphQLHTTPResponse:
+ if isinstance(result, GraphQLIncrementalExecutionResults):
+ return result
- if result.errors:
</code_context>
<issue_to_address>
Returning the raw incremental execution result may break expected interfaces.
Returning GraphQLIncrementalExecutionResults instead of ExecutionResult may cause runtime errors if consumers expect the standard interface. Please ensure the returned type matches the expected interface.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -363,12 +435,16 @@ async def run( | |||
) | |||
|
|||
def encode_multipart_data(self, data: Any, separator: str) -> str: |
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.
suggestion (bug_risk): The separator is used as a boundary but is hardcoded as '-', which may not be unique.
Consider generating a unique boundary string instead of using '-', to prevent parsing errors if the payload contains the same sequence.
Suggested implementation:
import uuid
boundary = f"----strawberry-{uuid.uuid4().hex}"
headers={
"Transfer-Encoding": "chunked",
"Content-Type": f'multipart/mixed; boundary="{boundary}"',
},
)
async def stream() -> AsyncGenerator[str, None]:
yield f"--{boundary}"
sub_response, | ||
headers={ | ||
"Transfer-Encoding": "chunked", | ||
"Content-Type": 'multipart/mixed; boundary="-"', |
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.
suggestion (bug_risk): The Content-Type boundary value should match the actual boundary used in the body.
Make sure the boundary in the header exactly matches the one used in the multipart body. It's best to use a unique string for the boundary.
Suggested implementation:
boundary = "graphql-subscription-12345"
yield self.encode_multipart_data(response, boundary)
yield f"--{boundary}--\r\n"
return await self.create_streaming_response(
request,
stream,
sub_response,
headers={
"Transfer-Encoding": "chunked",
"Content-Type": f'multipart/mixed; boundary={boundary}',
},
)
def _is_streamable(cls, annotation: Any, args: list[Any]) -> bool: | ||
return any(isinstance(arg, StrawberryStreamable) for arg in args) |
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.
suggestion: Type checking with isinstance on annotation arguments may not be robust.
Consider checking types directly or using typing.get_origin/get_args, as annotation arguments may not always be instances.
def _is_streamable(cls, annotation: Any, args: list[Any]) -> bool: | |
return any(isinstance(arg, StrawberryStreamable) for arg in args) | |
def _is_streamable(cls, annotation: Any, args: list[Any]) -> bool: | |
from typing import get_origin, get_args | |
def is_streamable_type(arg): | |
# Check if the argument is the StrawberryStreamable type or a generic of it | |
origin = get_origin(arg) or arg | |
return origin is StrawberryStreamable | |
return any(is_streamable_type(arg) for arg in args) |
if isinstance(result, GraphQLIncrementalExecutionResults): | ||
return result |
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.
issue (bug_risk): Returning the raw incremental execution result may break expected interfaces.
Returning GraphQLIncrementalExecutionResults instead of ExecutionResult may cause runtime errors if consumers expect the standard interface. Please ensure the returned type matches the expected interface.
import pytest | ||
from inline_snapshot import snapshot | ||
|
||
from tests.conftest import skip_if_gql_32 |
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.
issue (code-quality): Don't import test modules. (dont-import-test-modules
)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
from inline_snapshot import snapshot | ||
|
||
from tests.conftest import skip_if_gql_32 | ||
from tests.http.clients.base import HttpClient |
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.
issue (code-quality): Don't import test modules. (dont-import-test-modules
)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
|
||
import strawberry | ||
from strawberry.schema.config import StrawberryConfig | ||
from tests.conftest import skip_if_gql_32 |
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.
issue (code-quality): Don't import test modules. (dont-import-test-modules
)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
"\n", | ||
"\r\n", | ||
"Content-Type: application/json; charset=utf-8\r\n", | ||
"Content-Length: " + str(len(encoded_data)) + "\r\n", |
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.
suggestion (code-quality): We've found these issues:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation
) - Remove unnecessary calls to
str()
from formatted values in f-strings (remove-str-from-fstring
)
"Content-Length: " + str(len(encoded_data)) + "\r\n", | |
f"Content-Length: {len(encoded_data)}" + "\r\n", |
@@ -561,9 +561,9 @@ def print_schema_definition( | |||
def print_directive( | |||
directive: GraphQLDirective, *, schema: BaseSchema | |||
) -> Optional[str]: | |||
strawberry_directive = directive.extensions["strawberry-definition"] | |||
strawberry_directive = directive.extensions.get("strawberry-definition") | |||
|
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.
issue (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else
) - Replace if statement with if expression (
assign-if-exp
) - Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation
)
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.
Greptile Summary
This PR implements experimental support for GraphQL incremental execution, specifically adding the @defer and @stream directives to Strawberry. These features allow for more efficient data loading patterns by enabling parts of a GraphQL response to be deferred or streamed, improving perceived performance for complex queries.
Key changes include:
- Adding a new
enable_experimental_incremental_execution
flag toStrawberryConfig
- Implementing support for @defer and @stream directives in the schema printer
- Adding a new
Streamable
type for handling async generators - Creating comprehensive end-to-end tests using both Apollo and Relay clients
- Setting up a new CI workflow for e2e testing with Playwright
The implementation is thoughtfully structured as an opt-in experimental feature, requiring explicit configuration to enable. The e2e test suite is particularly thorough, testing both immediate and deferred data loading scenarios across multiple GraphQL clients.
Confidence score: 3/5
- The PR is safe to merge but requires careful monitoring in production due to its experimental nature
- While the implementation is solid and well-tested, the experimental status of the features and reliance on graphql-core>=3.3.0a9 introduces some risk
- Key files needing attention:
- strawberry/schema/schema.py: Complex changes to core execution logic
- strawberry/http/init.py: Critical changes to response handling
- tests/http/incremental/: New test infrastructure needs verification
49 files reviewed, 12 comments
Edit PR Review Bot Settings | Greptile
strawberry/annotation.py
Outdated
if self._is_streamable(evaled_type, args): | ||
return self.create_list(list[evaled_type]) |
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.
logic: streamable types are converted to lists implicitly - ensure all consuming code expects list types for streamable fields
.alexrc
Outdated
"failed" | ||
"crash" |
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.
syntax: Missing comma after 'failed', which makes this invalid JSON
"failed" | |
"crash" | |
"failed", | |
"crash" |
import { clsx, type ClassValue } from "clsx" | ||
import { twMerge } from "tailwind-merge" | ||
|
||
export function cn(...inputs: ClassValue[]) { |
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.
style: consider adding a brief JSDoc comment explaining the purpose of the cn function and its parameters
tests/views/schema.py
Outdated
return Hero(id=strawberry.ID("1"), name="Thiago Bellini") | ||
|
||
@strawberry.field | ||
async def streambable_field(self) -> strawberry.Streamable[str]: |
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.
syntax: Typo in field name: 'streambable_field' should be 'streamable_field'
async def streambable_field(self) -> strawberry.Streamable[str]: | |
async def streamable_field(self) -> strawberry.Streamable[str]: |
return ( | ||
<div className="flex flex-col gap-4"> | ||
<h1 className="text-2xl font-bold">Apollo Tests</h1> | ||
<div className=" gap-4"> |
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.
syntax: Empty space in className string serves no purpose
<div className=" gap-4"> | |
<div className="gap-4"> |
for comment in fetch_comments(): | ||
yield comment |
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.
logic: The example code references undefined function fetch_comments()
for comment in fetch_comments(): | |
yield comment | |
# Example: fetch comments from database or API | |
for comment in [ | |
Comment(id="1", content="Great article!"), | |
Comment(id="2", content="Thanks for sharing") | |
]: | |
yield comment |
if schema.config.enable_experimental_incremental_execution: | ||
directives.append( | ||
"directive @defer(if: Boolean, label: String) on FRAGMENT_SPREAD | INLINE_FRAGMENT" | ||
) | ||
directives.append( | ||
"directive @stream(if: Boolean, label: String, initialCount: Int = 0) on FIELD" | ||
) |
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.
style: Consider placing directive definitions in a constant or configuration.
|
||
```python | ||
import strawberry | ||
from dataclasses import dataclass |
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.
style: dataclass import is unused in the example code
tests/http/incremental/test_defer.py
Outdated
|
||
async def test_basic_stream(http_client: HttpClient): | ||
response = await http_client.query( | ||
method="get", |
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.
style: test_basic_stream is only testing GET method, unlike test_basic_defer which tests both GET and POST. Consider parameterizing this test as well for consistency
tests/http/incremental/test_defer.py
Outdated
method="get", | ||
query=""" | ||
query Stream { | ||
streambableField @stream |
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.
syntax: Typo in field name: 'streambableField' should be 'streamableField'
streambableField @stream | |
streamableField @stream |
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.
Left some suggestions, leaving this already approved 😊
cd e2e | ||
poetry run strawberry server app:schema --port 8000 & | ||
echo $! > server.pid | ||
sleep 5 # Wait for server to start |
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.
Maybe:
max_attempts=30
attempt=0
echo "Waiting for server to start..."
while [ $attempt -lt $max_attempts ]; do
if curl -s http://localhost:8000/graphql -o /dev/null; then
echo "Server is up and running!"
break
fi
attempt=$((attempt+1))
echo "Attempt $attempt/$max_attempts: Server not ready yet, waiting..."
sleep 1
done
if [ $attempt -eq $max_attempts ]; then
echo "Server failed to start after $max_attempts seconds"
exit 1
fi
So we can remove the following check?
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.
you're better than claude :D
The response will be delivered incrementally: | ||
|
||
```json | ||
# Initial payload |
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.
Maybe this needs to be jsonc
or json5
to be able to have comments and render properly? Then the comments might need to be written with //
instead of #
if isinstance(result, GraphQLIncrementalExecutionResults): | ||
|
||
async def stream() -> AsyncGenerator[str, None]: | ||
yield "---" |
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.
I like the suggestion from sourcery of using a unique boundary
instead of -
. Wdyt?
if self.config.enable_experimental_incremental_execution: | ||
directives = tuple(directives) + tuple(incremental_execution_directives) |
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.
There are two raise RuntimeError
below, which check that. Maybe it should be here during __init__
instead, and in there just assert
?
# TODO: maybe here use the first result from incremental execution if it exists | ||
if isinstance(result, GraphQLExecutionResult) and result.errors: |
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.
If I remember it correctly, there was one incremental execution type that did not have errors
in it (not totally sure, it's been a while 😅)
If the idea is just to limit to results that have errors
, maybe hasattr(result, "errors")
would be easier?
tests/views/schema.py
Outdated
@@ -130,6 +138,16 @@ def set_header(self, info: strawberry.Info, name: str) -> str: | |||
|
|||
return name | |||
|
|||
@strawberry.field | |||
def character(self) -> Hero: | |||
return Hero(id=strawberry.ID("1"), name="Thiago Bellini") |
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.
🦸♂️
Another attempt :D
Summary by Sourcery
Enable experimental GraphQL incremental execution by adding support for @defer and @stream directives, streaming multipart responses, and related configuration, printer, and execution integrations.
New Features:
Enhancements:
CI:
Documentation:
Tests: