Add Query Batching Support #3755
Conversation
Reviewer's Guide by SourceryThis pull request introduces batch query support for GraphQL APIs, enabling clients to send multiple queries in a single request. This is implemented by adding a "batch" parameter to the view classes and modifying the request handling logic to process lists of queries. Batching is NOT supported for multipart subscriptions. Sequence diagram for GraphQL batch query processingsequenceDiagram
participant Client
participant GraphQLView
participant Schema
Client->>GraphQLView: POST /graphql/batch
Note over Client,GraphQLView: Request with multiple queries
GraphQLView->>GraphQLView: parse_http_body()
GraphQLView->>GraphQLView: validate batch mode
loop For each query in batch
GraphQLView->>Schema: execute_single(query)
Schema-->>GraphQLView: execution result
GraphQLView->>GraphQLView: process_result()
end
GraphQLView-->>Client: Combined response array
Class diagram showing GraphQL view modifications for batch supportclassDiagram
class AsyncBaseHTTPView {
+bool batch
+execute_operation()
+execute_single()
+parse_http_body()
+create_response()
}
class SyncBaseHTTPView {
+bool batch
+execute_operation()
+execute_single()
+parse_http_body()
+create_response()
}
class GraphQLRequestData {
+str query
+dict variables
+str operation_name
+str protocol
}
AsyncBaseHTTPView ..> GraphQLRequestData
SyncBaseHTTPView ..> GraphQLRequestData
note for AsyncBaseHTTPView "Added batch support"
note for SyncBaseHTTPView "Added batch support"
Flow diagram for batch query request handlingflowchart TD
A[Client Request] --> B{Is Batch Request?}
B -->|Yes| C[Validate Batch Mode]
B -->|No| D[Process Single Query]
C --> E{Batch Enabled?}
E -->|No| F[Return 400 Error]
E -->|Yes| G[Process Multiple Queries]
G --> H[Execute Each Query]
H --> I[Combine Results]
I --> J[Return Response]
D --> J
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aryaniyaps - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks for adding the Here's a preview of the changelog: Add GraphQL Query batching supportGraphQL query batching is now supported across all frameworks (sync and async) This makes your GraphQL API compatible with batching features supported by various Example (FastAPI): import strawberry
from fastapi import FastAPI
from strawberry.fastapi import GraphQLRouter
from strawberry.schema.config import StrawberryConfig
@strawberry.type
class Query:
@strawberry.field
def hello(self) -> str:
return "Hello World"
schema = strawberry.Schema(
Query, config=StrawberryConfig(batching_config={"enabled": True})
)
graphql_app = GraphQLRouter(schema)
app = FastAPI()
app.include_router(graphql_app, prefix="/graphql")Example (Flask): import strawberry
from flask import Flask
from strawberry.flask.views import GraphQLView
app = Flask(__name__)
@strawberry.type
class Query:
@strawberry.field
def hello(self) -> str:
return "Hello World"
schema = strawberry.Schema(
Query, config=StrawberryConfig(batching_config={"enabled": True})
)
app.add_url_rule(
"/graphql/batch",
view_func=GraphQLView.as_view("graphql_view", schema=schema),
)
if __name__ == "__main__":
app.run()Note: Query Batching is not supported for multipart subscriptions Here's the tweet text: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3755 +/- ##
==========================================
- Coverage 97.27% 97.26% -0.01%
==========================================
Files 504 505 +1
Lines 33481 33589 +108
Branches 5503 5533 +30
==========================================
+ Hits 32567 32669 +102
- Misses 703 706 +3
- Partials 211 214 +3 |
CodSpeed Performance ReportMerging #3755 will not alter performanceComparing Summary
|
…assed in (even though batching is enabled)
| request: Request, | ||
| request_adapter: AsyncHTTPRequestAdapter, | ||
| sub_response: SubResponse, | ||
| context: Context, |
There was a problem hiding this comment.
Seems like context is shared between all operations?
This may cause unexpected side effects.
Although sometimes that may even be beneficial (for dataloaders etc).
There seems to be no way to isolate request, but it would be nice to have different context/root value per operation in my case.
There was a problem hiding this comment.
I think that the context should be shared, reusing dataloaders and other resources must be one of the main factors behind adopting query batching, if we want separate context, we can use separate requests with batching disabled, right?
There was a problem hiding this comment.
Well, maybe my use case not that common, because I use context not only for dataloaders, but also for some application logic and some tracing.
Since sharing is simpler - separate context might be a separate feature behind config flag.
There was a problem hiding this comment.
Well, maybe my use case not that common, because I use context not only for dataloaders, but also for some application logic and some tracing. Since sharing is simpler - separate context might be a separate feature behind config flag.
but Im curious @Object905 what benefits would batching bring if the context is not shared?
There was a problem hiding this comment.
Reducing request count mostly to avoid network round-trips.
For example one of my subgraphs is "external" and has ping of about 50ms. But even for "local" sub-graphs this might be noticeable.
Also there are other means to share global data (request itself, thread locals, etc.) besides context.
There was a problem hiding this comment.
@erikwrede got it, creating a separate context makes more sense to me now. I'll work on making the context sharing configurable
There was a problem hiding this comment.
I've been trying to implement this @erikwrede however I feel like I've hit a roadblock.
I cannot just get a new context like this for every request while using FastAPI:
if isinstance(request_data, list):
# batch GraphQL requests
if not self.schema.config.batching_config["share_context"]:
tasks = [
self.execute_single(
request=request,
request_adapter=request_adapter,
sub_response=sub_response,
# create a new context for each request data
context=await self.get_context(request, response=sub_response),
root_value=root_value,
request_data=data,
)
for data in request_data
]That's because the FastAPI context getter is deeply integrated with the FastAPI DI system, and is run only once per request. what should be done for this use case?
There was a problem hiding this comment.
also, I was trying to create a test case, replicating shopify's context driven translation.
I tried to create QUERY level schema directives like this but it wasn't working.
@strawberry.directive(locations=[DirectiveLocation.QUERY], name="in_context")
def in_context(language: str, info: strawberry.Info):
# put the language in the context here
print("in_context called")
info.context.update({"language": language})am I doing this correctly? I wasn't able to find any documentation/ test cases which were using this type of directive
There was a problem hiding this comment.
let me tag @DoctorJohn for a second opinion on the FastAPI Issue, he's done quite a bit of web view work recently.
Regarding the shopify example, this is not something we have as a test case in strawberry. I was just referring to it as an example where multiple operations could cause clashes in context.
To be fair, multiple queries in a single GraphQL Document are also a valid request that could cause clashes. However, in this case, the implementation can detect it by parsing the document containing all queries. With the current batching implementation, all batch entries are isolated, i.e. don't know of each other and the possiblity to clash, but still share the same context.
In general, I still think this is a trade off worth making. As mentioned on Discord, I'll look further into the problem with directives and get back.
There was a problem hiding this comment.
In case this is a blocker for too long, I'd be open to splitting this into 2 separate PRs as well. One that adds batching support, and the other adding an option to share or not share context.
|
@sourcery-ai review |
Add Query Batching Support
Description
This PR adds Query batching support for GraphQL APIs using Strawberry GraphQL.
This makes your GraphQL API compatible with batching features supported by various
client side libraries, such as Apollo GraphQL and Relay.
Example (FastAPI):
Example (Flask):
Note: Query Batching is not supported for multipart subscriptions
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Add support for batching GraphQL queries across all supported frameworks.
New Features:
Tests: