-
Notifications
You must be signed in to change notification settings - Fork 272
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
Move some GraphQL analysis earlier in the pipeline #6128
Conversation
A GraphQL document can contain multiple operations, but exactly one of them is relevant for a given request as specified by `operationName`.
✅ Docs Preview ReadyNo new or changed pages found. |
@SimonSapin, please consider creating a changeset entry in |
.operation | ||
.selection_set | ||
.selections | ||
.iter() | ||
.all(|sel| sel.as_field().is_some_and(|f| f.name == "__typename")) |
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 could/should also use operation.root_fields()
, i guess, but doesn't need to be addressed here
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.
root_fields()
is used to compute doc.has_root_typename
and implicitly descends into inline and named fragments regardless of any directive they might have. This specifically only looks at field, in order to not deal with potential @skip
or @include
that fragments might have.
… but now I realize that fields can also have @skip
or @include
, so I’ll add this restriction to the condition here.
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’ll simplify this further in the next PR, having a fast path only for literally {__typename}
and letting introspection code deal with any other combination of fragments, aliases, directives, etc.
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 figured that if we have a fast path for root typename, we might as well have a fast path for something like this?
fragment tyname on Query { __typename }
{
... { ...tyname }
}
Though I wonder how useful a __typename
fast path is in general with Rust introspection.
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.
LGTM, removing Vec is very nice 🥳
CI performance tests
|
The following is now done in
QueryAnalysisLayer
instead of in theBridgeQueryPlanner
service:operationName
. NowParsedDocument
in context contains a reference to the parsedapollo_compiler::executable::Operation
.__typename
, schema intropsection, and "normal" non-meta fields. This is a preliminary step to moving introspection out ofBridgeQueryPlanner
.As a side-effect observable to clients, the exact response for some error cases like an empty document changes slightly as the error no longer comes from JavaScript code.
Also optional drive-by refactor in the second commit: change
spec::Query
to contain a singleOperation
instead of aVec<Operation>
. This removes conditionals and the need to passoperationName
around in a lot of places, notably in execution code.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩