diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 74651d884..ffcf66197 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -16,7 +16,7 @@ jobs: steps: - uses: actions/checkout@v4 - - uses: gradle/wrapper-validation-action@v3 + - uses: gradle/actions/wrapper-validation@v4 - name: Set up JDK 11 uses: actions/setup-java@v4 with: diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 98a5a3e35..61f24f3d2 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -21,7 +21,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: gradle/wrapper-validation-action@v3 + - uses: gradle/actions/wrapper-validation@v4 - name: Set up JDK 11 uses: actions/setup-java@v4 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 21b31d32e..2d630314c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,7 +18,7 @@ jobs: steps: - uses: actions/checkout@v4 - - uses: gradle/wrapper-validation-action@v3 + - uses: gradle/actions/wrapper-validation@v4 - name: Set up JDK 11 uses: actions/setup-java@v4 with: diff --git a/src/main/java/graphql/GraphqlErrorHelper.java b/src/main/java/graphql/GraphqlErrorHelper.java index 391b223d9..901c25b5a 100644 --- a/src/main/java/graphql/GraphqlErrorHelper.java +++ b/src/main/java/graphql/GraphqlErrorHelper.java @@ -73,7 +73,10 @@ public static Object location(SourceLocation location) { if (line < 1 || column < 1) { return null; } - return Map.of("line", line, "column", column); + LinkedHashMap map = new LinkedHashMap<>(2); + map.put("line", line); + map.put("column", column); + return map; } static List fromSpecification(List> specificationMaps) { diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java index 1a9ec7c82..d7c84f478 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java @@ -44,55 +44,81 @@ private static class CallStack { private final LockKit.ReentrantLock lock = new LockKit.ReentrantLock(); private final LevelMap expectedFetchCountPerLevel = new LevelMap(); private final LevelMap fetchCountPerLevel = new LevelMap(); - private final LevelMap expectedStrategyCallsPerLevel = new LevelMap(); - private final LevelMap happenedStrategyCallsPerLevel = new LevelMap(); + + private final LevelMap expectedExecuteObjectCallsPerLevel = new LevelMap(); + private final LevelMap happenedExecuteObjectCallsPerLevel = new LevelMap(); + private final LevelMap happenedOnFieldValueCallsPerLevel = new LevelMap(); private final Set dispatchedLevels = new LinkedHashSet<>(); public CallStack() { - expectedStrategyCallsPerLevel.set(1, 1); + expectedExecuteObjectCallsPerLevel.set(1, 1); } void increaseExpectedFetchCount(int level, int count) { expectedFetchCountPerLevel.increment(level, count); } + void clearExpectedFetchCount() { + expectedFetchCountPerLevel.clear(); + } + void increaseFetchCount(int level) { fetchCountPerLevel.increment(level, 1); } - void increaseExpectedStrategyCalls(int level, int count) { - expectedStrategyCallsPerLevel.increment(level, count); + void clearFetchCount() { + fetchCountPerLevel.clear(); + } + + void increaseExpectedExecuteObjectCalls(int level, int count) { + expectedExecuteObjectCallsPerLevel.increment(level, count); } - void increaseHappenedStrategyCalls(int level) { - happenedStrategyCallsPerLevel.increment(level, 1); + void clearExpectedObjectCalls() { + expectedExecuteObjectCallsPerLevel.clear(); + } + + void increaseHappenedExecuteObjectCalls(int level) { + happenedExecuteObjectCallsPerLevel.increment(level, 1); + } + + void clearHappenedExecuteObjectCalls() { + happenedExecuteObjectCallsPerLevel.clear(); } void increaseHappenedOnFieldValueCalls(int level) { happenedOnFieldValueCallsPerLevel.increment(level, 1); } - boolean allStrategyCallsHappened(int level) { - return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); + void clearHappenedOnFieldValueCalls() { + happenedOnFieldValueCallsPerLevel.clear(); + } + + boolean allExecuteObjectCallsHappened(int level) { + return happenedExecuteObjectCallsPerLevel.get(level) == expectedExecuteObjectCallsPerLevel.get(level); } boolean allOnFieldCallsHappened(int level) { - return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); + return happenedOnFieldValueCallsPerLevel.get(level) == expectedExecuteObjectCallsPerLevel.get(level); } boolean allFetchesHappened(int level) { return fetchCountPerLevel.get(level) == expectedFetchCountPerLevel.get(level); } + void clearDispatchLevels() { + dispatchedLevels.clear(); + } + @Override public String toString() { return "CallStack{" + "expectedFetchCountPerLevel=" + expectedFetchCountPerLevel + ", fetchCountPerLevel=" + fetchCountPerLevel + - ", expectedStrategyCallsPerLevel=" + expectedStrategyCallsPerLevel + - ", happenedStrategyCallsPerLevel=" + happenedStrategyCallsPerLevel + + ", expectedExecuteObjectCallsPerLevel=" + expectedExecuteObjectCallsPerLevel + + ", happenedExecuteObjectCallsPerLevel=" + happenedExecuteObjectCallsPerLevel + ", happenedOnFieldValueCallsPerLevel=" + happenedOnFieldValueCallsPerLevel + ", dispatchedLevels" + dispatchedLevels + '}'; @@ -125,16 +151,14 @@ public void executionStrategy(ExecutionContext executionContext, ExecutionStrate return; } int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - increaseCallCounts(curLevel, parameters); + increaseHappenedExecuteObjectAndIncreaseExpectedFetchCount(curLevel, parameters); + } @Override - public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { - if (this.startedDeferredExecution.get()) { - return; - } - int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - increaseCallCounts(curLevel, parameters); + public void executionSerialStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + resetCallStack(); + increaseHappenedExecuteObjectAndIncreaseExpectedFetchCount(1, 1); } @Override @@ -145,13 +169,24 @@ public void executionStrategyOnFieldValuesInfo(List fieldValueIn onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, 1); } - @Override public void executionStrategyOnFieldValuesException(Throwable t) { callStack.lock.runLocked(() -> callStack.increaseHappenedOnFieldValueCalls(1) ); } + + @Override + public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + if (this.startedDeferredExecution.get()) { + return; + } + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; + increaseHappenedExecuteObjectAndIncreaseExpectedFetchCount(curLevel, parameters); + } + + + @Override public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { if (this.startedDeferredExecution.get()) { @@ -170,45 +205,34 @@ public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyPa ); } - @Override - public void fieldFetched(ExecutionContext executionContext, - ExecutionStrategyParameters parameters, - DataFetcher dataFetcher, - Object fetchedValue) { - - final boolean dispatchNeeded; - - if (parameters.getField().isDeferred() || this.startedDeferredExecution.get()) { - this.startedDeferredExecution.set(true); - dispatchNeeded = true; - } else { - int level = parameters.getPath().getLevel(); - dispatchNeeded = callStack.lock.callLocked(() -> { - callStack.increaseFetchCount(level); - return dispatchIfNeeded(level); - }); - } - - if (dispatchNeeded) { - dispatch(); - } - - } - - private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters) { - int count = 0; + private void increaseHappenedExecuteObjectAndIncreaseExpectedFetchCount(int curLevel, ExecutionStrategyParameters parameters) { + int nonDeferredFields = 0; for (MergedField field : parameters.getFields().getSubFieldsList()) { if (!field.isDeferred()) { - count++; + nonDeferredFields++; } } - int nonDeferredFieldCount = count; + increaseHappenedExecuteObjectAndIncreaseExpectedFetchCount(curLevel, nonDeferredFields); + } + + private void increaseHappenedExecuteObjectAndIncreaseExpectedFetchCount(int curLevel, int fieldCount) { callStack.lock.runLocked(() -> { - callStack.increaseExpectedFetchCount(curLevel, nonDeferredFieldCount); - callStack.increaseHappenedStrategyCalls(curLevel); + callStack.increaseHappenedExecuteObjectCalls(curLevel); + callStack.increaseExpectedFetchCount(curLevel, fieldCount); }); } + private void resetCallStack() { + callStack.lock.runLocked(() -> { + callStack.clearDispatchLevels(); + callStack.clearExpectedObjectCalls(); + callStack.clearExpectedFetchCount(); + callStack.clearFetchCount(); + callStack.clearHappenedExecuteObjectCalls(); + callStack.clearHappenedOnFieldValueCalls(); + callStack.expectedExecuteObjectCallsPerLevel.set(1, 1); + }); + } private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel) { boolean dispatchNeeded = callStack.lock.callLocked(() -> handleOnFieldValuesInfo(fieldValueInfoList, curLevel) @@ -223,23 +247,53 @@ private void onFieldValuesInfoDispatchIfNeeded(List fieldValueIn // private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel) { callStack.increaseHappenedOnFieldValueCalls(curLevel); - int expectedStrategyCalls = getCountForList(fieldValueInfos); - callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + int expectedStrategyCalls = getObjectCountForList(fieldValueInfos); + callStack.increaseExpectedExecuteObjectCalls(curLevel + 1, expectedStrategyCalls); return dispatchIfNeeded(curLevel + 1); } - private int getCountForList(List fieldValueInfos) { + /** + * the amount of (non nullable) objects that will require an execute object call + */ + private int getObjectCountForList(List fieldValueInfos) { int result = 0; for (FieldValueInfo fieldValueInfo : fieldValueInfos) { if (fieldValueInfo.getCompleteValueType() == FieldValueInfo.CompleteValueType.OBJECT) { result += 1; } else if (fieldValueInfo.getCompleteValueType() == FieldValueInfo.CompleteValueType.LIST) { - result += getCountForList(fieldValueInfo.getFieldValueInfos()); + result += getObjectCountForList(fieldValueInfo.getFieldValueInfos()); } } return result; } + + @Override + public void fieldFetched(ExecutionContext executionContext, + ExecutionStrategyParameters executionStrategyParameters, + DataFetcher dataFetcher, + Object fetchedValue) { + + final boolean dispatchNeeded; + + if (executionStrategyParameters.getField().isDeferred() || this.startedDeferredExecution.get()) { + this.startedDeferredExecution.set(true); + dispatchNeeded = true; + } else { + int level = executionStrategyParameters.getPath().getLevel(); + dispatchNeeded = callStack.lock.callLocked(() -> { + callStack.increaseFetchCount(level); + return dispatchIfNeeded(level); + }); + } + + if (dispatchNeeded) { + dispatch(); + } + + } + + // // thread safety : called with callStack.lock // @@ -260,7 +314,7 @@ private boolean levelReady(int level) { return callStack.allFetchesHappened(1); } if (levelReady(level - 1) && callStack.allOnFieldCallsHappened(level - 1) - && callStack.allStrategyCallsHappened(level) && callStack.allFetchesHappened(level)) { + && callStack.allExecuteObjectCallsHappened(level) && callStack.allFetchesHappened(level)) { return true; } diff --git a/src/test/groovy/graphql/GraphqlErrorHelperTest.groovy b/src/test/groovy/graphql/GraphqlErrorHelperTest.groovy index 018c9f157..0736b1671 100644 --- a/src/test/groovy/graphql/GraphqlErrorHelperTest.groovy +++ b/src/test/groovy/graphql/GraphqlErrorHelperTest.groovy @@ -3,6 +3,7 @@ package graphql import graphql.language.SourceLocation import graphql.validation.ValidationError import graphql.validation.ValidationErrorType +import spock.lang.RepeatUntilFailure import spock.lang.Specification class GraphqlErrorHelperTest extends Specification { @@ -154,4 +155,15 @@ class GraphqlErrorHelperTest extends Specification { assert gErr.getExtensions() == null } } + + @RepeatUntilFailure(maxAttempts = 1_000) + def "can deterministically serialize SourceLocation"() { + when: + def specMap = GraphqlErrorHelper.toSpecification(new TestError()) + + then: + def location = specMap["locations"][0] as Map + def keys = location.keySet().toList() + keys == ["line", "column"] + } } diff --git a/src/test/groovy/graphql/MutationTest.groovy b/src/test/groovy/graphql/MutationTest.groovy index 5c872b1f8..9ad61f6dd 100644 --- a/src/test/groovy/graphql/MutationTest.groovy +++ b/src/test/groovy/graphql/MutationTest.groovy @@ -141,16 +141,17 @@ class MutationTest extends Specification { ]]) def graphQL = GraphQL.newGraphQL(schema).build() - - when: - def er = graphQL.execute(""" + def ei = ExecutionInput.newExecutionInput(""" mutation m { plus1(arg:10) plus2(arg:10) plus3(arg:10) } - """) + """).build() + ei.getGraphQLContext().put(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT, defeEnabled) + when: + def er = graphQL.execute(ei) then: er.errors.isEmpty() er.data == [ @@ -158,6 +159,8 @@ class MutationTest extends Specification { plus2: 12, plus3: 13, ] + where: + defeEnabled << [true, false] } def "simple async mutation with DataLoader"() { @@ -213,6 +216,7 @@ class MutationTest extends Specification { plus3(arg:10) } """).dataLoaderRegistry(dlReg).build() + ei.getGraphQLContext().put(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT, defeEnabled) when: def er = graphQL.execute(ei) @@ -223,12 +227,16 @@ class MutationTest extends Specification { plus2: 12, plus3: 13, ] + + where: + defeEnabled << [true, false] } /* This test shows a dataloader being called at the mutation field level, in serial via AsyncSerialExecutionStrategy, and then again at the sub field level, in parallel, via AsyncExecutionStrategy. */ + def "more complex async mutation with DataLoader"() { def sdl = """ type Query { @@ -436,6 +444,7 @@ class MutationTest extends Specification { } } """).dataLoaderRegistry(dlReg).build() + ei.getGraphQLContext().put(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT, defeEnabled) when: def cf = graphQL.executeAsync(ei) @@ -459,5 +468,8 @@ class MutationTest extends Specification { topLevelF3: expectedMap, topLevelF4: expectedMap, ] + + where: + defeEnabled << [true, false] } }