Defer execution 2024 #3421
Defer execution 2024 #3421
Conversation
This reverts commit 0025495.
Revert "Merge pull request graphql-java#1964 from graphql-java/remove-deferred-2" This reverts commit a0d327d, reversing changes made to 3101f48. Revert "Merge pull request graphql-java#1961 from graphql-java/remove-deferred-support" This reverts commit 3101f48, reversing changes made to 10eeacc.
| * Will result on 1 instance of `DeferredCall`, containing calls for the 2 fields: "text" and "summary". | ||
| */ | ||
| @Internal | ||
| public class DeferredCall implements IncrementalCall<DeferPayload> { |
There was a problem hiding this comment.
Naming is hard but this (ans the ENF version) should be some other name
- DeferredBlockCall
- DeferredFragmentCall
- DeferredFragmentInvocation
| } | ||
|
|
||
| private void collectFragmentSpread(FieldCollectorParameters parameters, Set<String> visitedFragments, Map<String, MergedField> fields, FragmentSpread fragmentSpread) { | ||
| private void collectFragmentSpread(FieldCollectorParameters parameters, Set<String> visitedFragments, Map<String, MergedField> fields, FragmentSpread fragmentSpread, boolean incrementalSupport) { |
There was a problem hiding this comment.
maybe we dont need the incrementalSupport FF here - eg always do it BUT then never used it elsewhere ?
There was a problem hiding this comment.
I think the less new code we execute the better. So I'm with the opinion that we can keep this behind the FF as well
| static { | ||
| incrementalSupport = false | ||
| } | ||
| } |
| */ | ||
| @ExperimentalApi | ||
| public static final String ENABLE_INCREMENTAL_SUPPORT = "ENABLE_INCREMENTAL_SUPPORT"; | ||
|
|
There was a problem hiding this comment.
I think we should love this anotehr class - yes the mechanism is this - but the static should be elsewhere
One thought is this
public @interface ExperimentalApi {
String ENABLE_INCREMENTAL_SUPPORT = "ENABLE_INCREMENTAL_SUPPORT";
}
which then allows
GraphQLContext.newContext().put(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT,true);
There was a problem hiding this comment.
That makes sense 👍 I'm making the change
| public void enqueue(Collection<IncrementalCall<? extends IncrementalPayload>> calls) { | ||
| if (!calls.isEmpty()) { | ||
| calls.forEach(this::enqueue); | ||
| } |
There was a problem hiding this comment.
why !calls.isEmpty())
is this just and allAll kinda thing ??
There was a problem hiding this comment.
You're right, there's no need for this empty check. Let me remove it.
| path: ["post"], | ||
| data: [summary: "A summary"] | ||
| ] | ||
| ] |
There was a problem hiding this comment.
sghould we check for a label as null or missing ?
There was a problem hiding this comment.
Ah yep, good call. Let me add one 👍
The spec doesn't explicitly mentions what the behaviour should be in the scenario when label: null.
It clearly states that the value must be a static string (i.e: it can't be passed as a variable).
So we have 2 options here:
1- we treat queries that have label: null as invalid - we can write a new validation rule for that. This kind of makes sense. Since the value has to be static, there's no reason for query owners to set label: null, ever. They should just omit that argument altogether.
2- we treat queries that have label: null the same way we do for @defers that don't have a label at all
I honestly think it doesn't matter that much at this point, so I'll go with option 2, simply because it's the easier to implement (we don't need to write a new validation rule)
| @@ -0,0 +1,1371 @@ | |||
| package graphql.execution.incremental | |||
There was a problem hiding this comment.
I like this test a lot - great testing
bbakerman
left a comment
There was a problem hiding this comment.
Feb 12
I am happy to give this a tick but I want the FF to be moved to its own class - bit GraphqlContext
So either ExperimentalApi as suggested OR some where like that
Add support for @defer in the GraphQL execution.
Note: this implementation is still experimental.
This implementation is based on the state of graphql/graphql-spec#742. 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
In this already merged PR, we added support for
@deferin Executable Normalized Fields: #3395