Skip to content

Commit 8a24dca

Browse files
andimarekandimarek
committed
testing and fixes delayed list of objects case
1 parent 583c278 commit 8a24dca

File tree

6 files changed

+78
-10
lines changed

6 files changed

+78
-10
lines changed

‎src/main/java/graphql/execution/DataLoaderDispatchStrategy.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,16 @@ default void subscriptionEventCompletionDone(AlternativeCallContext alternativeC
6868
default void finishedFetching(ExecutionContext executionContext, ExecutionStrategyParameters newParameters) {
6969

7070
}
71+
72+
default void deferFieldFetched(ExecutionStrategyParameters executionStrategyParameters) {
73+
74+
}
75+
76+
default void startComplete(ExecutionStrategyParameters parameters) {
77+
78+
}
79+
80+
default void stopComplete(ExecutionStrategyParameters parameters) {
81+
82+
}
7183
}

‎src/main/java/graphql/execution/ExecutionStrategy.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -361,15 +361,21 @@ protected Object resolveFieldWithInfo(ExecutionContext executionContext, Executi
361361
Object fetchedValueObj = fetchField(executionContext, parameters);
362362
if (fetchedValueObj instanceof CompletableFuture) {
363363
CompletableFuture<Object> fetchFieldFuture = (CompletableFuture<Object>) fetchedValueObj;
364-
CompletableFuture<FieldValueInfo> result = fetchFieldFuture.thenApply((fetchedValue) ->
365-
completeField(fieldDef, executionContext, parameters, fetchedValue));
364+
CompletableFuture<FieldValueInfo> result = fetchFieldFuture.thenApply((fetchedValue) -> {
365+
executionContext.getDataLoaderDispatcherStrategy().startComplete(parameters);
366+
FieldValueInfo completeFieldResult = completeField(fieldDef, executionContext, parameters, fetchedValue);
367+
executionContext.getDataLoaderDispatcherStrategy().stopComplete(parameters);
368+
return completeFieldResult;
369+
});
366370

367371
fieldCtx.onDispatched();
368372
result.whenComplete(fieldCtx::onCompleted);
369373
return result;
370374
} else {
371375
try {
376+
executionContext.getDataLoaderDispatcherStrategy().startComplete(parameters);
372377
FieldValueInfo fieldValueInfo = completeField(fieldDef, executionContext, parameters, fetchedValueObj);
378+
executionContext.getDataLoaderDispatcherStrategy().stopComplete(parameters);
373379
fieldCtx.onDispatched();
374380
fieldCtx.onCompleted(FetchedValue.getFetchedValue(fetchedValueObj), null);
375381
return fieldValueInfo;

‎src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,8 @@ private Supplier<CompletableFuture<DeferredFragmentCall.FieldWithExecutionResult
182182
InstrumentationContext<Object> deferredFieldCtx = nonNullCtx(instrumentation.beginDeferredField(fieldParameters, executionContext.getInstrumentationState()));
183183

184184
CompletableFuture<FieldValueInfo> fieldValueResult = resolveFieldWithInfoFn.apply(this.executionContext, executionStrategyParameters);
185+
executionContext.getDataLoaderDispatcherStrategy().deferFieldFetched(executionStrategyParameters);
186+
185187

186188
deferredFieldCtx.onDispatched();
187189

‎src/main/java/graphql/execution/instrumentation/dataloader/ExhaustedDataLoaderDispatchStrategy.java‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
import graphql.execution.DataLoaderDispatchStrategy;
77
import graphql.execution.ExecutionContext;
88
import graphql.execution.ExecutionStrategyParameters;
9-
import graphql.execution.FieldValueInfo;
109
import graphql.execution.incremental.AlternativeCallContext;
1110
import org.dataloader.DataLoader;
1211
import org.dataloader.DataLoaderRegistry;
@@ -76,13 +75,13 @@ public static boolean getCurrentlyDispatching(int state) {
7675
}
7776

7877

79-
public void incrementObjectRunningCount() {
78+
public int incrementObjectRunningCount() {
8079
while (true) {
8180
int oldState = getState();
8281
int objectRunningCount = getObjectRunningCount(oldState);
8382
int newState = setObjectRunningCount(oldState, objectRunningCount + 1);
8483
if (tryUpdateState(oldState, newState)) {
85-
return;
84+
return newState;
8685
}
8786
}
8887
}
@@ -149,7 +148,7 @@ public ExhaustedDataLoaderDispatchStrategy(ExecutionContext executionContext) {
149148
public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters, int fieldCount) {
150149
Assert.assertTrue(parameters.getExecutionStepInfo().getPath().isRootPath());
151150
// no concurrency access happening
152-
initialCallStack.incrementObjectRunningCount();
151+
int newState = initialCallStack.incrementObjectRunningCount();
153152
}
154153

155154
@Override
@@ -170,7 +169,7 @@ public void executionSerialStrategy(ExecutionContext executionContext, Execution
170169
@Override
171170
public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters, int fieldCount) {
172171
CallStack callStack = getCallStack(parameters);
173-
callStack.incrementObjectRunningCount();
172+
int newState = callStack.incrementObjectRunningCount();
174173
}
175174

176175

@@ -188,14 +187,24 @@ public void subscriptionEventCompletionDone(AlternativeCallContext alternativeCa
188187
}
189188

190189
@Override
191-
public void deferredOnFieldValue(String resultKey, FieldValueInfo fieldValueInfo, Throwable throwable, ExecutionStrategyParameters parameters) {
190+
public void deferFieldFetched(ExecutionStrategyParameters parameters) {
192191
CallStack callStack = getCallStack(parameters);
193192
int deferredFragmentRootFieldsCompleted = callStack.deferredFragmentRootFieldsCompleted.incrementAndGet();
194193
Assert.assertNotNull(parameters.getDeferredCallContext());
195194
if (deferredFragmentRootFieldsCompleted == parameters.getDeferredCallContext().getFields()) {
196195
decrementObjectRunningAndMaybeDispatch(callStack);
197196
}
197+
}
198+
199+
@Override
200+
public void startComplete(ExecutionStrategyParameters parameters) {
201+
getCallStack(parameters).incrementObjectRunningCount();
202+
}
198203

204+
@Override
205+
public void stopComplete(ExecutionStrategyParameters parameters) {
206+
CallStack callStack = getCallStack(parameters);
207+
decrementObjectRunningAndMaybeDispatch(callStack);
199208
}
200209

201210
private CallStack getCallStack(ExecutionStrategyParameters parameters) {
@@ -213,6 +222,7 @@ private CallStack getCallStack(@Nullable AlternativeCallContext alternativeCallC
213222
The reason we are doing this lazily is, because we don't have explicit startDeferred callback.
214223
*/
215224
CallStack callStack = new CallStack();
225+
callStack.incrementObjectRunningCount();
216226
return callStack;
217227
});
218228
}

‎src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ private CallStack getCallStack(@Nullable AlternativeCallContext alternativeCallC
422422
// how many fields are deferred on this level
423423
int fields = k.getFields();
424424
if (startLevel > 1) {
425-
// parent level is considered dispatched and all fields completed
425+
// parent level is considered dispatched and all fields completed (meaning the grandparent level has all object completion call happened)
426426
callStack.dispatchedLevels.add(startLevel - 1);
427427
CallStack.StateForLevel stateForLevel = callStack.get(startLevel - 2);
428428
CallStack.StateForLevel newStateForLevel = stateForLevel.increaseHappenedExecuteObjectCalls().increaseHappenedCompletionFinishedCount();

‎src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ class DeferWithDataLoaderTest extends Specification {
6161
}
6262
}
6363

64+
@Unroll
6465
def "query with single deferred field"() {
6566
given:
6667
def query = getQuery(true, false)
@@ -82,6 +83,9 @@ class DeferWithDataLoaderTest extends Specification {
8283
.dataLoaderRegistry(dataLoaderRegistry)
8384
.graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true])
8485
.build()
86+
if (exhaustedStrategy) {
87+
executionInput.getGraphQLContext().put(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_EXHAUSTED_DISPATCHING, true)
88+
}
8589

8690
IncrementalExecutionResult result = graphQL.execute(executionInput)
8791

@@ -103,8 +107,12 @@ class DeferWithDataLoaderTest extends Specification {
103107

104108
batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3
105109
batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 3
110+
111+
where:
112+
exhaustedStrategy << [false, true]
106113
}
107114

115+
@Unroll
108116
def "multiple fields on same defer block"() {
109117
given:
110118
def query = """
@@ -139,6 +147,9 @@ class DeferWithDataLoaderTest extends Specification {
139147
.dataLoaderRegistry(dataLoaderRegistry)
140148
.graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true])
141149
.build()
150+
if (exhaustedStrategy) {
151+
executionInput.getGraphQLContext().put(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_EXHAUSTED_DISPATCHING, true)
152+
}
142153

143154
IncrementalExecutionResult result = graphQL.execute(executionInput)
144155

@@ -176,8 +187,13 @@ class DeferWithDataLoaderTest extends Specification {
176187
]
177188
batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3
178189
batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 0
190+
191+
where:
192+
exhaustedStrategy << [false, true]
193+
179194
}
180195

196+
@Unroll
181197
def "query with nested deferred fields"() {
182198
given:
183199
def query = getQuery(true, true)
@@ -199,6 +215,10 @@ class DeferWithDataLoaderTest extends Specification {
199215
.dataLoaderRegistry(dataLoaderRegistry)
200216
.graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true])
201217
.build()
218+
if (exhaustedStrategy) {
219+
executionInput.getGraphQLContext().put(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_EXHAUSTED_DISPATCHING, true)
220+
}
221+
202222

203223
ExecutionResult result = graphQL.execute(executionInput)
204224

@@ -227,8 +247,13 @@ class DeferWithDataLoaderTest extends Specification {
227247

228248
batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3
229249
batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 9
250+
251+
where:
252+
exhaustedStrategy << [false, true]
253+
230254
}
231255

256+
@Unroll
232257
def "query with top-level deferred field"() {
233258
given:
234259
def query = """
@@ -242,7 +267,7 @@ class DeferWithDataLoaderTest extends Specification {
242267
expensiveShops {
243268
name
244269
}
245-
}
270+
}
246271
}
247272
"""
248273

@@ -262,6 +287,10 @@ class DeferWithDataLoaderTest extends Specification {
262287
.dataLoaderRegistry(dataLoaderRegistry)
263288
.graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true])
264289
.build()
290+
if (exhaustedStrategy) {
291+
executionInput.getGraphQLContext().put(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_EXHAUSTED_DISPATCHING, true)
292+
}
293+
265294

266295
IncrementalExecutionResult result = graphQL.execute(executionInput)
267296

@@ -290,8 +319,13 @@ class DeferWithDataLoaderTest extends Specification {
290319

291320
batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 1
292321
batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 0
322+
323+
where:
324+
exhaustedStrategy << [false, true]
325+
293326
}
294327

328+
@Unroll
295329
def "query with multiple deferred fields"() {
296330
given:
297331
def query = getExpensiveQuery(true)
@@ -347,6 +381,10 @@ class DeferWithDataLoaderTest extends Specification {
347381
// TODO: Why the load counters are only 1?
348382
batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 1
349383
batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 1
384+
385+
where:
386+
exhaustedStrategy << [false, true]
387+
350388
}
351389

352390
@Unroll

0 commit comments

Comments
 (0)