Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Query Batching Support #3755
base: main
Are you sure you want to change the base?
Add Query Batching Support #3755
Changes from 9 commits
dd6722b
a0c3d29
c8c0d7c
88f1961
287f615
a15a4f7
f592bec
9ba0109
f0d14ef
6308825
eb6c9cb
a885b67
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but Im curious @Object905 what benefits would batching bring if the context is not shared?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
@Object905 can you show us some example of this? 😊
I'm leaning towards always have a new context for each operation
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.
Sure. I can't give the full code, but here is the gist of it.
This is an "entry point" into location api. It can't handle multiple addresses in a single query by design (due to context sharing, etc.).
reduced_location_input
- might be quite heavy, because it goes through geocoding and a lot of db queries, so doing it twice in a single request is a no-no.Also in context there are other "heavy" objects that depend on location and building them a second time during query is heavy perf hit.
So, full info about location and heavy stuff is collected once and stored in context, so its available at any point below.
While its possible to forward such context in other ways - given that
LocationTerms
is quite big - its much more convenient to just use context.