Skip to content

Defer support on ENFs#3395

Merged
andimarek merged 34 commits into
graphql-java:masterfrom
felipe-gdr:defer-support-in-enf
Jan 10, 2024
Merged

Defer support on ENFs#3395
andimarek merged 34 commits into
graphql-java:masterfrom
felipe-gdr:defer-support-in-enf

Conversation

@felipe-gdr

@felipe-gdr felipe-gdr commented Dec 12, 2023

Copy link
Copy Markdown
Member

Add support for @defer on ExecutableNormalizedField.

This implementation is based on the state of Spec edits for @defer / @stream PR. 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.5 with graphql-js v17.0.0-alpha.2

@felipe-gdr felipe-gdr marked this pull request as draft December 12, 2023 03:38
@felipe-gdr felipe-gdr changed the title [WIP] Defer support in enf Defer support on ENFs Dec 14, 2023
@felipe-gdr felipe-gdr marked this pull request as ready for review December 14, 2023 23:17
def result = compileToDocumentWithDeferSupport(schema, QUERY, null, fields, noVariables)
def printed = AstPrinter.printAst(new AstSorter().sort(result.document))
then:
printed == '''{

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is currently failing. This is the result:

{
  dog {
    ... @defer {
      breed
      name
      owner {
        firstname
      }
    }
  }
}

@felipe-gdr felipe-gdr Dec 15, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a version of createExecutableNormalizedOperation that takes Option

ImmutableListMultimap.Builder<ExecutableNormalizedField, DeferExecution> normalizedFieldToDeferExecution = ImmutableListMultimap.builder();
normalizedFieldToDeferExecution.putAll(collectFromOperationResult.normalizedFieldToDeferExecution);

Consumer<CollectNFResult> captureCollectNFResult = (collectNFResult -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@felipe-gdr-atlassian felipe-gdr-atlassian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • apply the code official code formatter for this repo
  • add a reference to the version of the spec that this implementation is based on

* @return a map of {@link ExecutableNormalizedField} to its {@link DeferExecution}
*/
@ExperimentalApi
public ImmutableListMultimap<ExecutableNormalizedField, DeferExecution> getNormalizedFieldToDeferExecution() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a Map

@Internal
public class IncrementalNodes {

public DeferExecution getDeferExecution(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename method to createDeferExecution

@Nullable String operationName,
@NotNull List<ExecutableNormalizedField> topLevelFields,
@Nullable VariablePredicate variablePredicate,
@Nullable ImmutableListMultimap<ExecutableNormalizedField, DeferExecution> normalizedFieldToDeferExecution) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Map instead of Guava type

printed == '''{
dog {
name
... @defer {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should result in just 1 defer block

def printed = AstPrinter.printAst(new AstSorter().sort(result.document))
then:
printed == '''{
dog {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private void buildFieldWithChildren(ExecutableNormalizedField executableNormalizedField,
ImmutableList<FieldAndAstParent> fieldAndAstParents,
int curLevel,
int maxLevel) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change: use maxLevel from options instead of passing it as argument

@andimarek andimarek added this pull request to the merge queue Jan 10, 2024
Merged via the queue into graphql-java:master with commit 5a0fb3d Jan 10, 2024
@felipe-gdr felipe-gdr mentioned this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants