Defer support on ENFs#3395
Conversation
99213e7 to
79a5c45
Compare
79a5c45 to
df767cb
Compare
| def result = compileToDocumentWithDeferSupport(schema, QUERY, null, fields, noVariables) | ||
| def printed = AstPrinter.printAst(new AstSorter().sort(result.document)) | ||
| then: | ||
| printed == '''{ |
There was a problem hiding this comment.
this test is currently failing. This is the result:
{
dog {
... @defer {
breed
name
owner {
firstname
}
}
}
}
There was a problem hiding this comment.
I see 2 ways going forward:
- introduce a new class to represent a @defer block. This new class would share a type with ExecutableNormalizedField , so the children of an ENF could either be other ENFs, or @defer blocks.
- Build a map that associates fields with @defer outside of the ENF hierarchy, similarly to normalizedFieldToQueryDirectives.
I'm leaning towards option 2 because it seems less intrusive
| * @param options the {@link Options} to use for parsing | ||
| * @return a runtime representation of the graphql operation. | ||
| */ | ||
| public static ExecutableNormalizedOperation createExecutableNormalizedOperation( |
There was a problem hiding this comment.
I created a version of createExecutableNormalizedOperation that takes Option
| ImmutableListMultimap.Builder<ExecutableNormalizedField, DeferExecution> normalizedFieldToDeferExecution = ImmutableListMultimap.builder(); | ||
| normalizedFieldToDeferExecution.putAll(collectFromOperationResult.normalizedFieldToDeferExecution); | ||
|
|
||
| Consumer<CollectNFResult> captureCollectNFResult = (collectNFResult -> { |
There was a problem hiding this comment.
I feel like working with an instance of some object created on a per-execution basis would make accessing these values much easier. We could create instance fields instead of having to pass them down the stack.
| * @return a map of {@link ExecutableNormalizedField} to its {@link DeferExecution} | ||
| */ | ||
| @ExperimentalApi | ||
| public ImmutableListMultimap<ExecutableNormalizedField, DeferExecution> getNormalizedFieldToDeferExecution() { |
There was a problem hiding this comment.
Make this a Map
| @Internal | ||
| public class IncrementalNodes { | ||
|
|
||
| public DeferExecution getDeferExecution( |
There was a problem hiding this comment.
rename method to createDeferExecution
| @Nullable String operationName, | ||
| @NotNull List<ExecutableNormalizedField> topLevelFields, | ||
| @Nullable VariablePredicate variablePredicate, | ||
| @Nullable ImmutableListMultimap<ExecutableNormalizedField, DeferExecution> normalizedFieldToDeferExecution) { |
There was a problem hiding this comment.
Use Map instead of Guava type
| printed == '''{ | ||
| dog { | ||
| name | ||
| ... @defer { |
There was a problem hiding this comment.
This should result in just 1 defer block
| def printed = AstPrinter.printAst(new AstSorter().sort(result.document)) | ||
| then: | ||
| printed == '''{ | ||
| dog { |
There was a problem hiding this comment.
Not a blocker, but we should double-check what is the expected behaviour here with the drivers of the defer/stream spec change.
For reference, the Apollo server execution will block the response until breed is return in the initial payload, then it will sent a second, deferred, payload with breed again.
There was a problem hiding this comment.
Based on the direction the working group is taking (graphql/defer-stream-wg#69) regarding de-duplication, I propose we keep only the non-deferred version of the field.
I can do it on a follow up PR.
- introduce NormalizedDeferExecution - add defer execution state to ENF
…ctory" This reverts commit b9b0f54.
| private void buildFieldWithChildren(ExecutableNormalizedField executableNormalizedField, | ||
| ImmutableList<FieldAndAstParent> fieldAndAstParents, | ||
| int curLevel, | ||
| int maxLevel) { |
There was a problem hiding this comment.
Minor change: use maxLevel from options instead of passing it as argument
Add support for
@deferonExecutableNormalizedField.This implementation is based on the state of Spec edits for
@defer/@streamPR. More specifically at the state of this commit graphql/graphql-spec@c630301.The execution behaviour should match what we get from running Apollo Server
4.9.5with graphql-jsv17.0.0-alpha.2