From f61baaa3b5e80c64745cac9f0a969b1c9a8de875 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Fri, 7 Jun 2024 17:16:06 +1000 Subject: [PATCH 01/21] WIP --- .../PerLevelDataLoaderDispatchStrategy.java | 2 +- .../DataLoaderPerformanceData.groovy | 249 +++++++----------- .../DataLoaderPerformanceTest.groovy | 87 +----- ...manceWithChainedInstrumentationTest.groovy | 20 +- .../dataloader/DeferWithDataLoaderTest.groovy | 141 ++++++++++ 5 files changed, 253 insertions(+), 246 deletions(-) create mode 100644 src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index a407346954..f74c0c5a4f 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -99,7 +99,7 @@ public PerLevelDataLoaderDispatchStrategy(ExecutionContext executionContext) { @Override public void deferredField(ExecutionContext executionContext, MergedField currentField) { - throw new UnsupportedOperationException("Data Loaders cannot be used to resolve deferred fields"); +// throw new UnsupportedOperationException("Data Loaders cannot be used to resolve deferred fields"); } @Override diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy index 6246f883d8..8af72b0bfe 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy @@ -1,5 +1,6 @@ package graphql.execution.instrumentation.dataloader +import com.fasterxml.jackson.databind.ObjectMapper import graphql.Directives import graphql.GraphQL import graphql.execution.instrumentation.Instrumentation @@ -61,19 +62,23 @@ class DataLoaderPerformanceData { ]] ] - static def query = """ + static String getQuery(boolean enableDeferred) { + return """ query { shops { id name - departments { - id name - products { + ... @defer(if: $enableDeferred) { + departments { id name + products { + id name + } } - } + } } } """ + } static def expectedExpensiveData = [ shops : [[name : "Shop 1", @@ -162,7 +167,8 @@ class DataLoaderPerformanceData { assert incrementalResultsItems.any { it == [path: [], data: [expensiveShops: [[id: "exshop-1", name: "ExShop 1"], [id: "exshop-2", name: "ExShop 2"], [id: "exshop-3", name: "ExShop 3"]]]] } } - static def expensiveQuery = """ + static String getExpensiveQuery(boolean deferredEnabled) { + return """ query { shops { name @@ -171,180 +177,59 @@ class DataLoaderPerformanceData { products { name } - expensiveProducts { - name - } - } - expensiveDepartments { - name - products { - name - } - expensiveProducts { - name - } - } - } - expensiveShops { - name - departments { - name - products { - name - } - expensiveProducts { - name - } + ... @defer(if: $deferredEnabled) { + expensiveProducts { + name + } + } } - expensiveDepartments { - name - products { - name - } - expensiveProducts { + ... @defer(if: $deferredEnabled) { + expensiveDepartments { name - } - } - } - } - """ - - static def expectedInitialDeferredData = [ - data : [ - shops: [ - [id: "shop-1", name: "Shop 1"], - [id: "shop-2", name: "Shop 2"], - [id: "shop-3", name: "Shop 3"], - ] - ], - hasNext: true - ] - - static def expectedListOfDeferredData = [ - [ - hasNext : true, - incremental: [[ - path: ["shops", 0], - data: [ - departments: [ - [id: "department-1", name: "Department 1", products: [[id: "product-1", name: "Product 1"]]], - [id: "department-2", name: "Department 2", products: [[id: "product-2", name: "Product 2"]]], - [id: "department-3", name: "Department 3", products: [[id: "product-3", name: "Product 3"]]] - ] - ] - ]], - ], - [ - hasNext : true, - incremental: [[ - path: ["shops", 1], - data: [ - departments: [ - [id: "department-4", name: "Department 4", products: [[id: "product-4", name: "Product 4"]]], - [id: "department-5", name: "Department 5", products: [[id: "product-5", name: "Product 5"]]], - [id: "department-6", name: "Department 6", products: [[id: "product-6", name: "Product 6"]]] - ] - ], - ]], - ], - [ - hasNext : false, - incremental: [[ - path: ["shops", 2], - data: [ - departments: [ - [id: "department-7", name: "Department 7", products: [[id: "product-7", name: "Product 7"]]], - [id: "department-8", name: "Department 8", products: [[id: "product-8", name: "Product 8"]]], - [id: "department-9", name: "Department 9", products: [[id: "product-9", name: "Product 9"]]] - ] - ] - ]], - ] - ] - - - static def deferredQuery = """ - query { - shops { - id name - ... @defer { - departments { - id name products { - id name + name } - } - } - } - } - """ - - static def expensiveDeferredQuery = """ - query { - shops { - id name - ... @defer { - departments { - name - ... @defer { - products { - name - } - } - ... @defer { + ... @defer(if: $deferredEnabled) { expensiveProducts { name } } } } - ... @defer { - expensiveDepartments { + } + ... @defer(if: $deferredEnabled) { + expensiveShops { + name + departments { name products { name } - expensiveProducts { + ... @defer(if: $deferredEnabled) { + expensiveProducts { + name + } + } + } + ... @defer(if: $deferredEnabled) { + expensiveDepartments { name + products { + name + } + ... @defer(if: $deferredEnabled) { + expensiveProducts { + name + } + } } - } - } - } - ... @defer { - expensiveShops { - id name + } } } } - """ +""" - static def expectedExpensiveDeferredData = [ - [[id: "exshop-1", name: "ExShop 1"], [id: "exshop-2", name: "ExShop 2"], [id: "exshop-3", name: "ExShop 3"]], - [[name: "Department 1",products:null, expensiveProducts:null], [name: "Department 2",products:null, expensiveProducts:null], [name: "Department 3",products:null, expensiveProducts:null]], - [[name: "Department 1", products: [[name: "Product 1"]], expensiveProducts: [[name: "Product 1"]]], [name: "Department 2", products: [[name: "Product 2"]], expensiveProducts: [[name: "Product 2"]]], [name: "Department 3", products: [[name: "Product 3"]], expensiveProducts: [[name: "Product 3"]]]], - [[name: "Department 4",products:null, expensiveProducts:null], [name: "Department 5",products:null, expensiveProducts:null], [name: "Department 6",products:null, expensiveProducts:null]], - [[name: "Department 4", products: [[name: "Product 4"]], expensiveProducts: [[name: "Product 4"]]], [name: "Department 5", products: [[name: "Product 5"]], expensiveProducts: [[name: "Product 5"]]], [name: "Department 6", products: [[name: "Product 6"]], expensiveProducts: [[name: "Product 6"]]]], - [[name: "Department 7",products:null, expensiveProducts:null], [name: "Department 8",products:null, expensiveProducts:null], [name: "Department 9",products:null, expensiveProducts:null]], - [[name: "Department 7", products: [[name: "Product 7"]], expensiveProducts: [[name: "Product 7"]]], [name: "Department 8", products: [[name: "Product 8"]], expensiveProducts: [[name: "Product 8"]]], [name: "Department 9", products: [[name: "Product 9"]], expensiveProducts: [[name: "Product 9"]]]], - [[name: "Product 1"]], - [[name: "Product 1"]], - [[name: "Product 2"]], - [[name: "Product 2"]], - [[name: "Product 3"]], - [[name: "Product 3"]], - [[name: "Product 4"]], - [[name: "Product 4"]], - [[name: "Product 5"]], - [[name: "Product 5"]], - [[name: "Product 6"]], - [[name: "Product 6"]], - [[name: "Product 7"]], - [[name: "Product 7"]], - [[name: "Product 8"]], - [[name: "Product 8"]], - [[name: "Product 9"]], - [[name: "Product 9"]], - ] + } static List> getIncrementalResults(IncrementalExecutionResult initialResult) { Publisher deferredResultStream = initialResult.incrementalItemPublisher @@ -356,4 +241,48 @@ class DataLoaderPerformanceData { return subscriber.getEvents() .collect { it.toSpecification() } } + + + static Map combineExecutionResults(Map initialResult, List> incrementalResults) { + Map combinedResult = deepClone(initialResult, Map.class) + + incrementalResults + // groovy's flatMap + .collectMany { (List) it.incremental } + .each { result -> + def parent = findByPath((Map) combinedResult.data, (List) result.path) + if (parent instanceof Map) { + parent.putAll((Map) result.data) + } else if (parent instanceof List) { + parent.addAll(result.data) + } else { + throw new RuntimeException("Unexpected parent type: ${parent.getClass()}") + } + + if(combinedResult.errors != null && !result.errors.isEmpty()) { + if(combinedResult.errors == null) { + combinedResult.errors = [] + } + + combinedResult.errors.addAll(result.errors) + } + } + + combinedResult.remove("hasNext") + + combinedResult + } + + private static ObjectMapper objectMapper = new ObjectMapper() + private static T deepClone(Object obj, Class clazz) { + return objectMapper.readValue(objectMapper.writeValueAsString(obj), clazz) + } + + private static Object findByPath(Map data, List path) { + def current = data + path.each { key -> + current = current[key] + } + current + } } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy index 61fba228bf..38acaf79c2 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy @@ -2,21 +2,13 @@ package graphql.execution.instrumentation.dataloader import graphql.ExecutionInput import graphql.GraphQL -import graphql.incremental.IncrementalExecutionResult import org.dataloader.DataLoaderRegistry -import spock.lang.Ignore import spock.lang.Specification import static graphql.ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.assertIncrementalExpensiveData import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedExpensiveData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedInitialDeferredData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getDeferredQuery import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpectedData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpectedListOfDeferredData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpensiveDeferredQuery import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpensiveQuery -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getIncrementalResults import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getQuery class DataLoaderPerformanceTest extends Specification { @@ -35,7 +27,7 @@ class DataLoaderPerformanceTest extends Specification { def "760 ensure data loader is performant for lists"() { when: ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(query) + .query(getQuery(false)) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -57,7 +49,7 @@ class DataLoaderPerformanceTest extends Specification { when: ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(expensiveQuery) + .query(getExpensiveQuery(false)) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -80,7 +72,7 @@ class DataLoaderPerformanceTest extends Specification { batchCompareDataFetchers.useAsyncBatchLoading(true) ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(query) + .query(getQuery(false)) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -105,7 +97,7 @@ class DataLoaderPerformanceTest extends Specification { batchCompareDataFetchers.useAsyncBatchLoading(true) ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(expensiveQuery) + .query(getExpensiveQuery(false)) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -121,75 +113,4 @@ class DataLoaderPerformanceTest extends Specification { where: incrementalSupport << [true, false] } - - def "data loader will not work with deferred queries"() { - when: - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(deferredQuery) - .dataLoaderRegistry(dataLoaderRegistry) - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .build() - - def result = graphQL.execute(executionInput) - println(result); - - then: - def exception = thrown(UnsupportedOperationException) - exception.message == "Data Loaders cannot be used to resolve deferred fields" - } - - @Ignore("Resolution of deferred fields via Data loaders is not yet supported") - def "data loader will work with deferred queries"() { - - when: - - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(deferredQuery) - .dataLoaderRegistry(dataLoaderRegistry) - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .build() - - IncrementalExecutionResult result = graphQL.execute(executionInput) - - then: - result.toSpecification() == expectedInitialDeferredData - - when: - def incrementalResults = getIncrementalResults(result) - - then: - incrementalResults == expectedListOfDeferredData - - // With deferred results, we don't achieve the same efficiency. - batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 - batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 3 - } - - @Ignore("Resolution of deferred fields via Data loaders is not yet supported") - def "data loader will work with deferred queries on multiple levels deep"() { - - when: - - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(expensiveDeferredQuery) - .dataLoaderRegistry(dataLoaderRegistry) - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .build() - - IncrementalExecutionResult result = graphQL.execute(executionInput) - - then: - result.toSpecification() == expectedInitialDeferredData - - when: - def incrementalResults = getIncrementalResults(result) - - then: - assertIncrementalExpensiveData(incrementalResults) - - // With deferred results, we don't achieve the same efficiency. - // The final number of loader calls is non-deterministic, so we can't assert an exact number. - batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() >= 3 - batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() >= 3 - } } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy index e5d1089bab..403dd92ded 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy @@ -141,7 +141,6 @@ class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification exception.message == "Data Loaders cannot be used to resolve deferred fields" } - @Ignore("Resolution of deferred fields via Data loaders is not yet supported") def "chainedInstrumentation: data loader will work with deferred queries"() { when: @@ -161,7 +160,24 @@ class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification def incrementalResults = getIncrementalResults(result) then: - incrementalResults == expectedListOfDeferredData + + println(incrementalResults) + // Ordering is non-deterministic, so we assert on the things we know are going to be true. + + incrementalResults.size() == 3 + // only the last payload has "hasNext=true" + incrementalResults[0].hasNext == true + incrementalResults[1].hasNext == true + incrementalResults[2].hasNext == false + + // every payload has only 1 incremental item, and the data is the same for all of them + incrementalResults.every { it.incremental.size() == 1 } + incrementalResults.every { it.incremental[0].data == [wordCount: 45999] } + + // path is different for every payload + incrementalResults.any { it.incremental[0].path == ["posts", 0] } + incrementalResults.any { it.incremental[0].path == ["posts", 1] } + incrementalResults.any { it.incremental[0].path == ["posts", 2] } // With deferred results, we don't achieve the same efficiency. batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy new file mode 100644 index 0000000000..4816a26101 --- /dev/null +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -0,0 +1,141 @@ +package graphql.execution.instrumentation.dataloader + +import graphql.ExecutionInput +import graphql.GraphQL +import graphql.incremental.IncrementalExecutionResult +import org.dataloader.DataLoaderRegistry +import spock.lang.Specification + +import static graphql.ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.combineExecutionResults +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedData +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedExpensiveData +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpensiveQuery +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getIncrementalResults +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getQuery + +class DeferWithDataLoaderTest extends Specification { + + GraphQL graphQL + DataLoaderRegistry dataLoaderRegistry + BatchCompareDataFetchers batchCompareDataFetchers + + + void setup() { + batchCompareDataFetchers = new BatchCompareDataFetchers() + DataLoaderPerformanceData dataLoaderPerformanceData = new DataLoaderPerformanceData(batchCompareDataFetchers) + + dataLoaderRegistry = dataLoaderPerformanceData.setupDataLoaderRegistry() + graphQL = dataLoaderPerformanceData.setupGraphQL() + } + + + def "data loader will work with deferred queries"() { + given: + def query = getQuery(true) + + def expectedInitialData = [ + data : [ + shops: [ + [id: "shop-1", name: "Shop 1"], + [id: "shop-2", name: "Shop 2"], + [id: "shop-3", name: "Shop 3"], + ] + ], + hasNext: true + ] + + when: + ExecutionInput executionInput = ExecutionInput.newExecutionInput() + .query(query) + .dataLoaderRegistry(dataLoaderRegistry) + .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) + .build() + + IncrementalExecutionResult result = graphQL.execute(executionInput) + + then: + result.toSpecification() == expectedInitialData + + when: + def incrementalResults = getIncrementalResults(result) + + then: + // Ordering is non-deterministic, so we assert on the things we know are going to be true. + + incrementalResults.size() == 3 + // only the last payload has "hasNext=true" + incrementalResults[0].hasNext == true + incrementalResults[1].hasNext == true + incrementalResults[2].hasNext == false + + // every payload has only 1 incremental item + incrementalResults.every { it.incremental.size() == 1 } + + // path is different for every payload + incrementalResults.any { it.incremental[0].path == ["shops", 0] } + incrementalResults.any { it.incremental[0].path == ["shops", 1] } + incrementalResults.any { it.incremental[0].path == ["shops", 2] } + + when: + def combined = combineExecutionResults(result.toSpecification(), incrementalResults) + then: + combined.errors == null + combined.data == expectedData + } + + def "data loader will work with expensive deferred queries"() { + given: + def query = getExpensiveQuery(true) + + def expectedInitialData = [ + data : [ + shops: [ + [id: "shop-1", name: "Shop 1"], + [id: "shop-2", name: "Shop 2"], + [id: "shop-3", name: "Shop 3"], + ] + ], + hasNext: true + ] + + when: + ExecutionInput executionInput = ExecutionInput.newExecutionInput() + .query(query) + .dataLoaderRegistry(dataLoaderRegistry) + .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) + .build() + + IncrementalExecutionResult result = graphQL.execute(executionInput) + + then: + result.toSpecification() == expectedInitialData + + when: + def incrementalResults = getIncrementalResults(result) + + then: + // Ordering is non-deterministic, so we assert on the things we know are going to be true. + + incrementalResults.size() == 3 + // only the last payload has "hasNext=true" + incrementalResults[0].hasNext == true + incrementalResults[1].hasNext == true + incrementalResults[2].hasNext == false + + // every payload has only 1 incremental item + incrementalResults.every { it.incremental.size() == 1 } + + // path is different for every payload + incrementalResults.any { it.incremental[0].path == ["shops", 0] } + incrementalResults.any { it.incremental[0].path == ["shops", 1] } + incrementalResults.any { it.incremental[0].path == ["shops", 2] } + + when: + def combined = combineExecutionResults(result.toSpecification(), incrementalResults) + then: + combined.errors == null + combined.data == expectedExpensiveData + } + +} From 5b5823648dd813d9600a88ca2c223582197b6e14 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Wed, 12 Jun 2024 10:32:29 +1000 Subject: [PATCH 02/21] Add more tests for defer+data loaders scenarios --- .../DataLoaderPerformanceData.groovy | 17 ++- .../DataLoaderPerformanceTest.groovy | 4 +- ...manceWithChainedInstrumentationTest.groovy | 5 - .../dataloader/DeferWithDataLoaderTest.groovy | 136 ++++++++++++++++-- 4 files changed, 134 insertions(+), 28 deletions(-) diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy index 8af72b0bfe..21877b5917 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy @@ -61,18 +61,23 @@ class DataLoaderPerformanceData { [id: "department-9", name: "Department 9", products: [[id: "product-9", name: "Product 9"]]]] ]] ] + static String getQuery() { + return getQuery(false, false) + } - static String getQuery(boolean enableDeferred) { + static String getQuery(boolean deferDepartments, boolean deferProducts) { return """ query { shops { id name - ... @defer(if: $enableDeferred) { + ... @defer(if: $deferDepartments) { departments { id name - products { - id name - } + ... @defer(if: $deferProducts) { + products { + id name + } + } } } } @@ -197,7 +202,7 @@ class DataLoaderPerformanceData { } } } - ... @defer(if: $deferredEnabled) { + ... @defer(if: false) { expensiveShops { name departments { diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy index 38acaf79c2..c4239243ca 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceTest.groovy @@ -27,7 +27,7 @@ class DataLoaderPerformanceTest extends Specification { def "760 ensure data loader is performant for lists"() { when: ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(getQuery(false)) + .query(getQuery()) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -72,7 +72,7 @@ class DataLoaderPerformanceTest extends Specification { batchCompareDataFetchers.useAsyncBatchLoading(true) ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(getQuery(false)) + .query(getQuery()) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy index 403dd92ded..83058b8c28 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy @@ -10,12 +10,7 @@ import spock.lang.Specification import static graphql.ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.assertIncrementalExpensiveData import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedExpensiveData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedInitialDeferredData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedListOfDeferredData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getDeferredQuery import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpectedData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpensiveDeferredQuery -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpensiveQuery import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getIncrementalResults import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getQuery diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index 4816a26101..b876917eae 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -29,10 +29,23 @@ class DeferWithDataLoaderTest extends Specification { graphQL = dataLoaderPerformanceData.setupGraphQL() } + private static void assertIncrementalResults(List> results, List> expectedPaths) { + assert results.size() == expectedPaths.size(), "Expected ${expectedPaths.size()} results, got ${results.size()}" - def "data loader will work with deferred queries"() { + assert results.dropRight(1).every { it.hasNext == true }, "Expected all but the last result to have hasNext=true" + assert results.last().hasNext == false, "Expected last result to have hasNext=false" + + assert results.every { it.incremental.size() == 1 }, "Expected every result to have exactly one incremental item" + + expectedPaths.each { path -> + assert results.any { it.incremental[0].path == path }, "Expected path $path not found in $results" + } + } + + + def "query with single deferred field"() { given: - def query = getQuery(true) + def query = getQuery(true, false) def expectedInitialData = [ data : [ @@ -61,21 +74,114 @@ class DeferWithDataLoaderTest extends Specification { def incrementalResults = getIncrementalResults(result) then: - // Ordering is non-deterministic, so we assert on the things we know are going to be true. - incrementalResults.size() == 3 - // only the last payload has "hasNext=true" - incrementalResults[0].hasNext == true - incrementalResults[1].hasNext == true - incrementalResults[2].hasNext == false + assertIncrementalResults(incrementalResults, [["shops", 0], ["shops", 1], ["shops", 3]]) - // every payload has only 1 incremental item - incrementalResults.every { it.incremental.size() == 1 } + when: + def combined = combineExecutionResults(result.toSpecification(), incrementalResults) + then: + combined.errors == null + combined.data == expectedData + } - // path is different for every payload - incrementalResults.any { it.incremental[0].path == ["shops", 0] } - incrementalResults.any { it.incremental[0].path == ["shops", 1] } - incrementalResults.any { it.incremental[0].path == ["shops", 2] } + def "query with nested deferred fields"() { + given: + def query = getQuery(true, true) + + def expectedInitialData = [ + data : [ + shops: [ + [id: "shop-1", name: "Shop 1"], + [id: "shop-2", name: "Shop 2"], + [id: "shop-3", name: "Shop 3"], + ] + ], + hasNext: true + ] + + when: + ExecutionInput executionInput = ExecutionInput.newExecutionInput() + .query(query) + .dataLoaderRegistry(dataLoaderRegistry) + .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) + .build() + + IncrementalExecutionResult result = graphQL.execute(executionInput) + + then: + result.toSpecification() == expectedInitialData + + when: + def incrementalResults = getIncrementalResults(result) + + then: + + assertIncrementalResults(incrementalResults, + [ + ["shops", 0], ["shops", 1], ["shops", 2], + ["shops", 0, "departments", 0], ["shops", 1, "departments", 0], ["shops", 2, "departments", 0], + ["shops", 0, "departments", 1], ["shops", 1, "departments", 1], ["shops", 2, "departments", 1], + ["shops", 0, "departments", 2], ["shops", 1, "departments", 2], ["shops", 2, "departments", 2], + ] + ) + + when: + def combined = combineExecutionResults(result.toSpecification(), incrementalResults) + then: + combined.errors == null + combined.data == expectedData + } + + def "query with top-level deferred field"() { + given: + def query = """ + query { + shops { + expensiveDepartments { + name + } + } + ... @defer { + expensiveShops { + name + } + } + } +""" + + def expectedInitialData = [ + data : [ + shops: [ + [id: "shop-1", name: "Shop 1"], + [id: "shop-2", name: "Shop 2"], + [id: "shop-3", name: "Shop 3"], + ] + ], + hasNext: true + ] + + when: + ExecutionInput executionInput = ExecutionInput.newExecutionInput() + .query(query) + .dataLoaderRegistry(dataLoaderRegistry) + .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) + .build() + + IncrementalExecutionResult result = graphQL.execute(executionInput) + + then: + result.toSpecification() == expectedInitialData + + when: + def incrementalResults = getIncrementalResults(result) + + then: + + assertIncrementalResults(incrementalResults, + [ + [] + ] + ) when: def combined = combineExecutionResults(result.toSpecification(), incrementalResults) @@ -84,7 +190,7 @@ class DeferWithDataLoaderTest extends Specification { combined.data == expectedData } - def "data loader will work with expensive deferred queries"() { + def "query with multiple deferred fields"() { given: def query = getExpensiveQuery(true) From 3403589e46f3e0d2ba1257e44cd15456c7b4a3cd Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Tue, 18 Jun 2024 06:43:33 +1000 Subject: [PATCH 03/21] WIP --- .../execution/DataLoaderDispatchStrategy.java | 2 +- .../graphql/execution/ExecutionStrategy.java | 2 +- .../incremental/DeferredExecutionSupport.java | 28 ++-- .../instrumentation/dataloader/LevelMap.java | 17 +- .../PerLevelDataLoaderDispatchStrategy.java | 156 ++++++++++++++---- .../dataloader/BatchCompareDataFetchers.java | 17 +- .../DataLoaderPerformanceData.groovy | 4 + .../dataloader/DeferWithDataLoaderTest.groovy | 70 ++++---- 8 files changed, 200 insertions(+), 96 deletions(-) diff --git a/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java index ee3c0bd97c..18397d1227 100644 --- a/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java @@ -50,7 +50,7 @@ default DataFetcher modifyDataFetcher(DataFetcher dataFetcher) { return dataFetcher; } - default void deferredField(ExecutionContext executionContext, MergedField currentField) { + default void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { } } diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index de1ae0e178..480bc3dbaa 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -318,7 +318,7 @@ Async.CombinedBuilder getAsyncFieldValueInfo( ) { MergedSelectionSet fields = parameters.getFields(); - executionContext.getIncrementalCallState().enqueue(deferredExecutionSupport.createCalls()); + executionContext.getIncrementalCallState().enqueue(deferredExecutionSupport.createCalls(parameters)); // Only non-deferred fields should be considered for calculating the expected size of futures. Async.CombinedBuilder futures = Async diff --git a/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java b/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java index 034138d110..2c752d10cd 100644 --- a/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java +++ b/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java @@ -44,7 +44,7 @@ public interface DeferredExecutionSupport { List getNonDeferredFieldNames(List allFieldNames); - Set> createCalls(); + Set> createCalls(ExecutionStrategyParameters executionStrategyParameters); DeferredExecutionSupport NOOP = new DeferredExecutionSupport.NoOp(); @@ -105,19 +105,19 @@ public List getNonDeferredFieldNames(List allFieldNames) { } @Override - public Set> createCalls() { + public Set> createCalls(ExecutionStrategyParameters executionStrategyParameters) { return deferredExecutionToFields.keySet().stream() - .map(this::createDeferredFragmentCall) + .map(deferredExecution -> this.createDeferredFragmentCall(deferredExecution, executionStrategyParameters)) .collect(Collectors.toSet()); } - private DeferredFragmentCall createDeferredFragmentCall(DeferredExecution deferredExecution) { + private DeferredFragmentCall createDeferredFragmentCall(DeferredExecution deferredExecution, ExecutionStrategyParameters executionStrategyParameters) { DeferredCallContext deferredCallContext = new DeferredCallContext(); List mergedFields = deferredExecutionToFields.get(deferredExecution); List>> calls = mergedFields.stream() - .map(currentField -> this.createResultSupplier(currentField, deferredCallContext)) + .map(currentField -> this.createResultSupplier(currentField, deferredCallContext, executionStrategyParameters)) .collect(Collectors.toList()); return new DeferredFragmentCall( @@ -130,7 +130,8 @@ private DeferredFragmentCall createDeferredFragmentCall(DeferredExecution deferr private Supplier> createResultSupplier( MergedField currentField, - DeferredCallContext deferredCallContext + DeferredCallContext deferredCallContext, + ExecutionStrategyParameters executionStrategyParameters ) { Map fields = new LinkedHashMap<>(); fields.put(currentField.getResultKey(), currentField); @@ -149,7 +150,6 @@ private Supplier fieldValueResult = resolveFieldWithInfoFn .apply(executionContext, callParameters); - // Create a reference to the CompletableFuture that resolves an ExecutionResult - // so we can pass it to the Instrumentation "onDispatched" callback. CompletableFuture executionResultCF = fieldValueResult - .thenCompose(fvi -> fvi - .getFieldValueFuture() - .thenApply(fv -> ExecutionResultImpl.newExecutionResult().data(fv).build()) + .thenCompose(fvi -> { + executionContext.getDataLoaderDispatcherStrategy().deferredField(fvi, executionStrategyParameters); + + return fvi + .getFieldValueFuture() + .thenApply(fv -> ExecutionResultImpl.newExecutionResult().data(fv).build()); + } ); return executionResultCF @@ -199,7 +201,7 @@ public List getNonDeferredFieldNames(List allFieldNames) { } @Override - public Set> createCalls() { + public Set> createCalls(ExecutionStrategyParameters executionStrategyParameters) { return Collections.emptySet(); } } diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java b/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java index ddf46f643b..1df527d3d6 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java @@ -11,7 +11,7 @@ public class LevelMap { // A reasonable default that guarantees no additional allocations for most use cases. - private static final int DEFAULT_INITIAL_SIZE = 16; + private static final int DEFAULT_INITIAL_SIZE = 8; // this array is mutable in both size and contents. private int[] countsByLevel; @@ -54,13 +54,14 @@ private void maybeResize(int level) { @Override public String toString() { - StringBuilder result = new StringBuilder(); - result.append("IntMap["); - for (int i = 0; i < countsByLevel.length; i++) { - result.append("level=").append(i).append(",count=").append(countsByLevel[i]).append(" "); - } - result.append("]"); - return result.toString(); + return Arrays.toString(this.countsByLevel); +// StringBuilder result = new StringBuilder(); +// result.append("IntMap["); +// for (int i = 0; i < countsByLevel.length; i++) { +// result.append("level=").append(i).append(",count=").append(countsByLevel[i]).append(" "); +// } +// result.append("]"); +// return result.toString(); } public String toString(int level) { diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index f74c0c5a4f..2cf7e0c0e1 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -6,14 +6,15 @@ import graphql.execution.ExecutionContext; import graphql.execution.ExecutionStrategyParameters; import graphql.execution.FieldValueInfo; -import graphql.execution.MergedField; import graphql.schema.DataFetcher; import graphql.util.LockKit; import org.dataloader.DataLoaderRegistry; +import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; @Internal public class PerLevelDataLoaderDispatchStrategy implements DataLoaderDispatchStrategy { @@ -31,6 +32,11 @@ private static class CallStack { private final LevelMap happenedStrategyCallsPerLevel = new LevelMap(); private final LevelMap happenedOnFieldValueCallsPerLevel = new LevelMap(); + private final LevelMap expectedDeferredStrategyCallsPerLevel = new LevelMap(); + private final LevelMap happenedOnDeferredFieldValueCallsPerLevel = new LevelMap(); + + private final AtomicBoolean hasDeferredCalls = new AtomicBoolean(false); + private final Set dispatchedLevels = new LinkedHashSet<>(); public CallStack() { @@ -49,6 +55,11 @@ void increaseExpectedStrategyCalls(int level, int count) { expectedStrategyCallsPerLevel.increment(level, count); } + void increaseExpectedDeferredStrategyCalls(int level, int count) { + expectedDeferredStrategyCallsPerLevel.increment(level, count); + hasDeferredCalls.set(true); + } + void increaseHappenedStrategyCalls(int level) { happenedStrategyCallsPerLevel.increment(level, 1); } @@ -57,33 +68,52 @@ void increaseHappenedOnFieldValueCalls(int level) { happenedOnFieldValueCallsPerLevel.increment(level, 1); } + void increaseHappenedOnDeferredFieldValueCalls(int level) { + happenedOnDeferredFieldValueCallsPerLevel.increment(level, 1); + hasDeferredCalls.set(true); + } + boolean allStrategyCallsHappened(int level) { - return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); + return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); } boolean allOnFieldCallsHappened(int level) { - return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); + return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); } boolean allFetchesHappened(int level) { return fetchCountPerLevel.get(level) == expectedFetchCountPerLevel.get(level); } + private boolean hasDeferredCalls() { + return hasDeferredCalls.get(); + } + + private boolean hasDeferredCalls(int level) { + return expectedDeferredStrategyCallsPerLevel.get(level) > 0 || happenedOnDeferredFieldValueCallsPerLevel.get(level) > 0; + } + @Override public String toString() { - return "CallStack{" + - "expectedFetchCountPerLevel=" + expectedFetchCountPerLevel + - ", fetchCountPerLevel=" + fetchCountPerLevel + - ", expectedStrategyCallsPerLevel=" + expectedStrategyCallsPerLevel + - ", happenedStrategyCallsPerLevel=" + happenedStrategyCallsPerLevel + - ", happenedOnFieldValueCallsPerLevel=" + happenedOnFieldValueCallsPerLevel + - ", dispatchedLevels" + dispatchedLevels + + return "{" + + "efc=" + expectedFetchCountPerLevel + + ", fc=" + fetchCountPerLevel + + ", esc=" + expectedStrategyCallsPerLevel + + ", hsc=" + happenedStrategyCallsPerLevel + + ", hofvc=" + happenedOnFieldValueCallsPerLevel + + ", de=" + expectedDeferredStrategyCallsPerLevel + + ", dh=" + happenedOnDeferredFieldValueCallsPerLevel + + ", dl" + dispatchedLevels + '}'; } - public boolean dispatchIfNotDispatchedBefore(int level) { + public boolean dispatchIfNotDispatchedBefore(int level, String origin) { if (dispatchedLevels.contains(level)) { + if(this.hasDeferredCalls(level - 1)) { + System.out.println("df: " + level + " already dispatched."); + return true; + } Assert.assertShouldNeverHappen("level " + level + " already dispatched"); return false; } @@ -98,22 +128,47 @@ public PerLevelDataLoaderDispatchStrategy(ExecutionContext executionContext) { } @Override - public void deferredField(ExecutionContext executionContext, MergedField currentField) { -// throw new UnsupportedOperationException("Data Loaders cannot be used to resolve deferred fields"); + public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { + int curLevel = executionStrategyParameters.getExecutionStepInfo().getPath().getLevel() + 1; + + boolean dispatchNeeded = callStack.lock.callLocked(() -> { + callStack.increaseHappenedOnDeferredFieldValueCalls(curLevel); + + int expectedStrategyCalls = getCountForList(Collections.singletonList(fieldValueInfo)); + callStack.increaseExpectedDeferredStrategyCalls(curLevel + 1, expectedStrategyCalls); + +// callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + + System.out.println( + "df: " + curLevel + " :: " + + callStack + " :: " + + executionStrategyParameters.getPath() + " :: " + ); + +// return false; + return dispatchIfNeeded(curLevel + 1, "df"); + } + ); + if (dispatchNeeded) { + dispatch(curLevel); + } } @Override public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - increaseCallCounts(curLevel, parameters); + + increaseCallCounts(curLevel, parameters, "st"); } @Override public void executionStrategyOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { int curLevel = parameters.getPath().getLevel() + 1; - onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters); + + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters, "ev"); } + @Override public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters executionStrategyParameters) { int curLevel = executionStrategyParameters.getPath().getLevel() + 1; callStack.lock.runLocked(() -> @@ -125,13 +180,21 @@ public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrate @Override public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - increaseCallCounts(curLevel, parameters); + + increaseCallCounts(curLevel, parameters, "ob"); } @Override public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { int curLevel = parameters.getPath().getLevel() + 1; - onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters); + +// System.out.println( +// "ef: " + curLevel + " :: " + +// callStack + " :: " + +// parameters.getPath() +// ); + + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters, "of"); } @@ -144,17 +207,23 @@ public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyPa } - private void increaseCallCounts(int curLevel, ExecutionStrategyParameters executionStrategyParameters) { + private void increaseCallCounts(int curLevel, ExecutionStrategyParameters executionStrategyParameters, String origin) { int fieldCount = executionStrategyParameters.getFields().size(); callStack.lock.runLocked(() -> { callStack.increaseExpectedFetchCount(curLevel, fieldCount); callStack.increaseHappenedStrategyCalls(curLevel); }); + + System.out.println( + origin + ": " + curLevel + " :: " + + callStack + " :: " + + executionStrategyParameters.getPath() + ); } - private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, ExecutionStrategyParameters parameters) { + private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, ExecutionStrategyParameters parameters, String origin) { boolean dispatchNeeded = callStack.lock.callLocked(() -> - handleOnFieldValuesInfo(fieldValueInfoList, curLevel) + handleOnFieldValuesInfo(fieldValueInfoList, curLevel, origin, parameters) ); if (dispatchNeeded) { dispatch(curLevel); @@ -162,13 +231,20 @@ private void onFieldValuesInfoDispatchIfNeeded(List fieldValueIn } // -// thread safety: called with callStack.lock -// - private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel) { + // thread safety: called with callStack.lock + // + private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel, String origin, ExecutionStrategyParameters parameters) { callStack.increaseHappenedOnFieldValueCalls(curLevel); int expectedStrategyCalls = getCountForList(fieldValueInfos); callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); - return dispatchIfNeeded(curLevel + 1); + + System.out.println( + origin + ": " + curLevel + " :: " + + callStack + " :: " + + parameters.getPath() + " :: " + ); + + return dispatchIfNeeded(curLevel + 1, origin); } private int getCountForList(List fieldValueInfos) { @@ -192,8 +268,16 @@ public void fieldFetched(ExecutionContext executionContext, int level = executionStrategyParameters.getPath().getLevel(); boolean dispatchNeeded = callStack.lock.callLocked(() -> { callStack.increaseFetchCount(level); - return dispatchIfNeeded(level); + return dispatchIfNeeded(level, "ff"); }); + + System.out.println( + "ff: " + level + " :: " + + callStack + " :: " + + executionStrategyParameters.getPath() + " :: " + + fetchedValue + ); + if (dispatchNeeded) { dispatch(level); } @@ -204,25 +288,31 @@ public void fieldFetched(ExecutionContext executionContext, // // thread safety : called with callStack.lock // - private boolean dispatchIfNeeded(int level) { - boolean ready = levelReady(level); + private boolean dispatchIfNeeded(int level, String origin) { + boolean ready = levelReady(level, origin); if (ready) { - return callStack.dispatchIfNotDispatchedBefore(level); + System.out.println(level + " dispatched :: " + origin); + return callStack.dispatchIfNotDispatchedBefore(level, origin); } return false; } // -// thread safety: called with callStack.lock -// - private boolean levelReady(int level) { + // thread safety: called with callStack.lock + // + private boolean levelReady(int level, String origin) { if (level == 1) { // level 1 is special: there is only one strategy call and that's it return callStack.allFetchesHappened(1); } - if (levelReady(level - 1) && callStack.allOnFieldCallsHappened(level - 1) - && callStack.allStrategyCallsHappened(level) && callStack.allFetchesHappened(level)) { + + boolean lr = levelReady(level - 1, "-1"); + boolean aofch = callStack.allOnFieldCallsHappened(level - 1); + boolean atch = callStack.allStrategyCallsHappened(level); + boolean afh = callStack.allFetchesHappened(level); + + if (lr && aofch && atch && afh) { return true; } return false; diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java index 7f20938b3b..53a957e04f 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java @@ -51,11 +51,11 @@ public void useAsyncBatchLoading(boolean flag) { } - public DataFetcher>> shopsDataFetcher = - environment -> supplyAsyncWithSleep(() -> new ArrayList<>(shops.values())); + public DataFetcher>> shopsDataFetcher = environment -> + supplyAsyncWithSleep(() -> new ArrayList<>(shops.values()), new Random().nextInt(100)); public DataFetcher>> expensiveShopsDataFetcher = environment -> - supplyAsyncWithSleep(() -> new ArrayList<>(expensiveShops.values())); + supplyAsyncWithSleep(() -> new ArrayList<>(expensiveShops.values()), new Random().nextInt(100)); // Departments private static Map departments = new LinkedHashMap<>(); @@ -142,22 +142,23 @@ private static List> getProductsForDepartments(List de private CompletableFuture maybeAsyncWithSleep(Supplier> supplier) { if (useAsyncBatchLoading.get()) { - return supplyAsyncWithSleep(supplier) + return supplyAsyncWithSleep(supplier, 100) .thenCompose(cf -> cf); } else { return supplier.get(); } } - private static CompletableFuture supplyAsyncWithSleep(Supplier supplier) { - Supplier sleepSome = sleepSome(supplier); + private static CompletableFuture supplyAsyncWithSleep(Supplier supplier, long time) { + Supplier sleepSome = sleepSome(supplier, time); return CompletableFuture.supplyAsync(sleepSome); } - private static Supplier sleepSome(Supplier supplier) { + private static Supplier sleepSome(Supplier supplier, long time) { return () -> { try { - Thread.sleep(new Random().nextInt(100)); +// Thread.sleep(new Random().nextInt(100)); + Thread.sleep(time); return supplier.get(); } catch (Exception e) { throw new RuntimeException(e); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy index 21877b5917..54a46b69ac 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy @@ -243,6 +243,10 @@ class DataLoaderPerformanceData { deferredResultStream.subscribe(subscriber) Awaitility.await().untilTrue(subscriber.isDone()) + if(subscriber.getThrowable() != null) { + throw subscriber.getThrowable() + } + return subscriber.getEvents() .collect { it.toSpecification() } } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index b876917eae..f9b2ba50c3 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -1,6 +1,7 @@ package graphql.execution.instrumentation.dataloader import graphql.ExecutionInput +import graphql.ExecutionResult import graphql.GraphQL import graphql.incremental.IncrementalExecutionResult import org.dataloader.DataLoaderRegistry @@ -75,7 +76,7 @@ class DeferWithDataLoaderTest extends Specification { then: - assertIncrementalResults(incrementalResults, [["shops", 0], ["shops", 1], ["shops", 3]]) + assertIncrementalResults(incrementalResults, [["shops", 0], ["shops", 1], ["shops", 2]]) when: def combined = combineExecutionResults(result.toSpecification(), incrementalResults) @@ -106,7 +107,10 @@ class DeferWithDataLoaderTest extends Specification { .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) .build() - IncrementalExecutionResult result = graphQL.execute(executionInput) + ExecutionResult result = graphQL.execute(executionInput) + + println "dept for shops dl: " + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() + println "prod for depts dl: " + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() then: result.toSpecification() == expectedInitialData @@ -137,11 +141,11 @@ class DeferWithDataLoaderTest extends Specification { def query = """ query { shops { - expensiveDepartments { + departments { name } } - ... @defer { + ... @defer(if: true) { expensiveShops { name } @@ -151,10 +155,9 @@ class DeferWithDataLoaderTest extends Specification { def expectedInitialData = [ data : [ - shops: [ - [id: "shop-1", name: "Shop 1"], - [id: "shop-2", name: "Shop 2"], - [id: "shop-3", name: "Shop 3"], + shops: [[departments: [[name: "Department 1"], [name: "Department 2"], [name: "Department 3"]]], + [departments: [[name: "Department 4"], [name: "Department 5"], [name: "Department 6"]]], + [departments: [[name: "Department 7"], [name: "Department 8"], [name: "Department 9"]]], ] ], hasNext: true @@ -187,23 +190,30 @@ class DeferWithDataLoaderTest extends Specification { def combined = combineExecutionResults(result.toSpecification(), incrementalResults) then: combined.errors == null - combined.data == expectedData + combined.data == [shops : [[departments: [[name: "Department 1"], [name: "Department 2"], [name: "Department 3"]]], + [departments: [[name: "Department 4"], [name: "Department 5"], [name: "Department 6"]]], + [departments: [[name: "Department 7"], [name: "Department 8"], [name: "Department 9"]]]], + expensiveShops: [[name: "ExShop 1"], [name: "ExShop 2"], [name: "ExShop 3"]]] } def "query with multiple deferred fields"() { given: def query = getExpensiveQuery(true) - def expectedInitialData = [ - data : [ - shops: [ - [id: "shop-1", name: "Shop 1"], - [id: "shop-2", name: "Shop 2"], - [id: "shop-3", name: "Shop 3"], - ] - ], - hasNext: true - ] + def expectedInitialData = + [data : [shops : [[name : "Shop 1", + departments: [[name: "Department 1", products: [[name: "Product 1"]]], [name: "Department 2", products: [[name: "Product 2"]]], [name: "Department 3", products: [[name: "Product 3"]]]]], + [name : "Shop 2", + departments: [[name: "Department 4", products: [[name: "Product 4"]]], [name: "Department 5", products: [[name: "Product 5"]]], [name: "Department 6", products: [[name: "Product 6"]]]]], + [name : "Shop 3", + departments: [[name: "Department 7", products: [[name: "Product 7"]]], [name: "Department 8", products: [[name: "Product 8"]]], [name: "Department 9", products: [[name: "Product 9"]]]]]], + expensiveShops: [[name : "ExShop 1", + departments: [[name: "Department 1", products: [[name: "Product 1"]]], [name: "Department 2", products: [[name: "Product 2"]]], [name: "Department 3", products: [[name: "Product 3"]]]]], + [name : "ExShop 2", + departments: [[name: "Department 4", products: [[name: "Product 4"]]], [name: "Department 5", products: [[name: "Product 5"]]], [name: "Department 6", products: [[name: "Product 6"]]]]], + [name : "ExShop 3", + departments: [[name: "Department 7", products: [[name: "Product 7"]]], [name: "Department 8", products: [[name: "Product 8"]]], [name: "Department 9", products: [[name: "Product 9"]]]]]]], + hasNext: true] when: ExecutionInput executionInput = ExecutionInput.newExecutionInput() @@ -223,19 +233,15 @@ class DeferWithDataLoaderTest extends Specification { then: // Ordering is non-deterministic, so we assert on the things we know are going to be true. - incrementalResults.size() == 3 - // only the last payload has "hasNext=true" - incrementalResults[0].hasNext == true - incrementalResults[1].hasNext == true - incrementalResults[2].hasNext == false - - // every payload has only 1 incremental item - incrementalResults.every { it.incremental.size() == 1 } - - // path is different for every payload - incrementalResults.any { it.incremental[0].path == ["shops", 0] } - incrementalResults.any { it.incremental[0].path == ["shops", 1] } - incrementalResults.any { it.incremental[0].path == ["shops", 2] } + assertIncrementalResults(incrementalResults, + [ + ["expensiveShops", 0], ["expensiveShops", 1], ["expensiveShops", 2], + ["shops", 0], ["shops", 1], ["shops", 2], + ["shops", 2, "departments", 0], ["shops", 1, "departments", 2],["shops", 1, "departments", 1], ["shops", 1, "departments", 0],["shops", 0, "departments", 2], ["shops", 0, "departments", 1], ["shops", 0, "departments", 0],["shops", 2, "departments", 2],["shops", 2, "departments", 1], + ["shops", 1, "expensiveDepartments", 2], ["shops", 1, "expensiveDepartments", 1], ["shops", 1, "expensiveDepartments", 0], ["shops", 0, "expensiveDepartments", 2], ["shops", 0, "expensiveDepartments", 1], ["shops", 0, "expensiveDepartments", 0], ["shops", 2, "expensiveDepartments", 1], ["shops", 2, "expensiveDepartments", 2], + ["expensiveShops", 2, "expensiveDepartments", 2], ["expensiveShops", 2, "expensiveDepartments", 1], ["expensiveShops", 2, "expensiveDepartments", 0], ["expensiveShops", 2, "departments", 2], ["expensiveShops", 1, "expensiveDepartments", 2], ["expensiveShops", 1, "expensiveDepartments", 1], ["expensiveShops", 1, "expensiveDepartments", 0], ["expensiveShops", 0, "expensiveDepartments", 2], ["expensiveShops", 0, "expensiveDepartments", 1], ["expensiveShops", 0, "expensiveDepartments", 0], + ["expensiveShops", 2, "departments", 1], ["expensiveShops", 2, "departments", 0], ["expensiveShops", 1, "departments", 2], ["expensiveShops", 1, "departments", 1], ["expensiveShops", 1, "departments", 0], ["expensiveShops", 0, "departments", 2], ["expensiveShops", 0, "departments", 1], ["expensiveShops", 0, "departments", 0], ["shops", 2, "expensiveDepartments", 0]] + ) when: def combined = combineExecutionResults(result.toSpecification(), incrementalResults) From d4e3ba4ae4b3dc073c2f9da5809833bc219fe247 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Thu, 20 Jun 2024 14:25:46 +1000 Subject: [PATCH 04/21] WIP: working state - dirty --- .../java/graphql/execution/MergedField.java | 9 +++ .../PerLevelDataLoaderDispatchStrategy.java | 71 ++++++++++++++----- .../dataloader/DeferWithDataLoaderTest.groovy | 14 ++++ 3 files changed, 77 insertions(+), 17 deletions(-) diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index e66afb63f8..f70a46fad3 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -139,6 +139,15 @@ public List getDeferredExecutions() { return deferredExecutions; } + /** + * TODO Javadoc + * @return + */ + @ExperimentalApi + public boolean isDeferred() { + return !deferredExecutions.isEmpty(); + } + public static Builder newMergedField() { return new Builder(); } diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index 2cf7e0c0e1..364f41119f 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -6,6 +6,7 @@ import graphql.execution.ExecutionContext; import graphql.execution.ExecutionStrategyParameters; import graphql.execution.FieldValueInfo; +import graphql.execution.MergedField; import graphql.schema.DataFetcher; import graphql.util.LockKit; import org.dataloader.DataLoaderRegistry; @@ -13,6 +14,7 @@ import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; @@ -32,6 +34,8 @@ private static class CallStack { private final LevelMap happenedStrategyCallsPerLevel = new LevelMap(); private final LevelMap happenedOnFieldValueCallsPerLevel = new LevelMap(); + private final LevelMap expectedDeferredFetchCountPerLevel = new LevelMap(); + private final LevelMap deferredFetchCountPerLevel = new LevelMap(); private final LevelMap expectedDeferredStrategyCallsPerLevel = new LevelMap(); private final LevelMap happenedOnDeferredFieldValueCallsPerLevel = new LevelMap(); @@ -47,10 +51,18 @@ void increaseExpectedFetchCount(int level, int count) { expectedFetchCountPerLevel.increment(level, count); } + void increaseExpectedDeferredFetchCount(int level, int count) { + expectedDeferredFetchCountPerLevel.increment(level, count); + } + void increaseFetchCount(int level) { fetchCountPerLevel.increment(level, 1); } + void increaseDeferredFetchCount(int level) { + deferredFetchCountPerLevel.increment(level, 1); + } + void increaseExpectedStrategyCalls(int level, int count) { expectedStrategyCallsPerLevel.increment(level, count); } @@ -90,7 +102,8 @@ private boolean hasDeferredCalls() { } private boolean hasDeferredCalls(int level) { - return expectedDeferredStrategyCallsPerLevel.get(level) > 0 || happenedOnDeferredFieldValueCallsPerLevel.get(level) > 0; + return expectedDeferredStrategyCallsPerLevel.get(level) > 0 || happenedOnDeferredFieldValueCallsPerLevel.get(level) > 0 || + deferredFetchCountPerLevel.get(level) > 0 || expectedDeferredFetchCountPerLevel.get(level) > 0; } @Override @@ -104,13 +117,15 @@ public String toString() { ", de=" + expectedDeferredStrategyCallsPerLevel + ", dh=" + happenedOnDeferredFieldValueCallsPerLevel + ", dl" + dispatchedLevels + + ", edc=" + expectedDeferredFetchCountPerLevel + + ", dc=" + deferredFetchCountPerLevel + '}'; } public boolean dispatchIfNotDispatchedBefore(int level, String origin) { if (dispatchedLevels.contains(level)) { - if(this.hasDeferredCalls(level - 1)) { + if(this.hasDeferredCalls(level) || this.hasDeferredCalls(level - 1)) { System.out.println("df: " + level + " already dispatched."); return true; } @@ -128,8 +143,8 @@ public PerLevelDataLoaderDispatchStrategy(ExecutionContext executionContext) { } @Override - public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { - int curLevel = executionStrategyParameters.getExecutionStepInfo().getPath().getLevel() + 1; + public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; boolean dispatchNeeded = callStack.lock.callLocked(() -> { callStack.increaseHappenedOnDeferredFieldValueCalls(curLevel); @@ -142,7 +157,7 @@ public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParame System.out.println( "df: " + curLevel + " :: " + callStack + " :: " + - executionStrategyParameters.getPath() + " :: " + parameters.getPath() + " :: " ); // return false; @@ -152,6 +167,8 @@ public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParame if (dispatchNeeded) { dispatch(curLevel); } + +// onFieldValuesInfoDispatchIfNeeded(Collections.singletonList(fieldValueInfo), curLevel, parameters, "df"); } @Override @@ -169,8 +186,8 @@ public void executionStrategyOnFieldValuesInfo(List fieldValueIn } @Override - public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters executionStrategyParameters) { - int curLevel = executionStrategyParameters.getPath().getLevel() + 1; + public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getPath().getLevel() + 1; callStack.lock.runLocked(() -> callStack.increaseHappenedOnFieldValueCalls(curLevel) ); @@ -207,17 +224,24 @@ public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyPa } - private void increaseCallCounts(int curLevel, ExecutionStrategyParameters executionStrategyParameters, String origin) { - int fieldCount = executionStrategyParameters.getFields().size(); + private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters, String origin) { + int fieldCount = parameters.getFields().size(); + + int deferredFieldCount = (int) parameters.getFields().getSubFieldsList().stream() + .filter(MergedField::isDeferred) + .count(); + callStack.lock.runLocked(() -> { - callStack.increaseExpectedFetchCount(curLevel, fieldCount); + callStack.increaseExpectedFetchCount(curLevel, fieldCount - deferredFieldCount); + callStack.increaseExpectedDeferredFetchCount(curLevel, deferredFieldCount); callStack.increaseHappenedStrategyCalls(curLevel); }); + System.out.println( origin + ": " + curLevel + " :: " + callStack + " :: " + - executionStrategyParameters.getPath() + parameters.getPath() ); } @@ -234,9 +258,17 @@ private void onFieldValuesInfoDispatchIfNeeded(List fieldValueIn // thread safety: called with callStack.lock // private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel, String origin, ExecutionStrategyParameters parameters) { - callStack.increaseHappenedOnFieldValueCalls(curLevel); + boolean isDeferred = Optional.ofNullable(parameters.getField()).map(MergedField::isDeferred).orElse(false); + int expectedStrategyCalls = getCountForList(fieldValueInfos); - callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + if(isDeferred) { + System.out.println("deffZ"); + callStack.increaseExpectedDeferredStrategyCalls(10+curLevel + 1, expectedStrategyCalls); + callStack.increaseHappenedOnDeferredFieldValueCalls(10+curLevel); + } else { + callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + callStack.increaseHappenedOnFieldValueCalls(curLevel); + } System.out.println( origin + ": " + curLevel + " :: " + @@ -262,19 +294,24 @@ private int getCountForList(List fieldValueInfos) { @Override public void fieldFetched(ExecutionContext executionContext, - ExecutionStrategyParameters executionStrategyParameters, + ExecutionStrategyParameters parameters, DataFetcher dataFetcher, Object fetchedValue) { - int level = executionStrategyParameters.getPath().getLevel(); + int level = parameters.getPath().getLevel(); + boolean isDeferred = parameters.getField().isDeferred(); boolean dispatchNeeded = callStack.lock.callLocked(() -> { - callStack.increaseFetchCount(level); + if(isDeferred) { + callStack.increaseDeferredFetchCount(level); + } else { + callStack.increaseFetchCount(level); + } return dispatchIfNeeded(level, "ff"); }); System.out.println( "ff: " + level + " :: " + callStack + " :: " + - executionStrategyParameters.getPath() + " :: " + + parameters.getPath() + " :: " + fetchedValue ); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index f9b2ba50c3..e73c321ad0 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -47,6 +47,20 @@ class DeferWithDataLoaderTest extends Specification { def "query with single deferred field"() { given: def query = getQuery(true, false) +// def defer = true +// def query = """ +// query { +// shops { +// name +// ... @defer(if: $defer) { +// departments { +// name +// } +// } +// } +// } +// +// """ def expectedInitialData = [ data : [ From d84114d0c86165cd02009be43a7f7d20ebe85f89 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Thu, 20 Jun 2024 16:24:16 +1000 Subject: [PATCH 05/21] Create separate data loader dispatch strategy for defer support --- .../java/graphql/execution/Execution.java | 10 +- .../DataLoaderDispatcherInstrumentation.java | 0 .../PerLevelDataLoaderDispatchStrategy.java | 209 +++---------- ...elDataLoaderDispatchStrategyWithDefer.java | 276 ++++++++++++++++++ .../dataloader/BatchCompareDataFetchers.java | 17 +- ...manceWithChainedInstrumentationTest.groovy | 95 +----- .../dataloader/DeferWithDataLoaderTest.groovy | 90 +++++- 7 files changed, 414 insertions(+), 283 deletions(-) delete mode 100644 src/main/java/graphql/execution/instrumentation/dataloader/DataLoaderDispatcherInstrumentation.java create mode 100644 src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java diff --git a/src/main/java/graphql/execution/Execution.java b/src/main/java/graphql/execution/Execution.java index a3fff5a8e8..4055268362 100644 --- a/src/main/java/graphql/execution/Execution.java +++ b/src/main/java/graphql/execution/Execution.java @@ -14,6 +14,7 @@ import graphql.execution.instrumentation.InstrumentationState; import graphql.execution.instrumentation.dataloader.FallbackDataLoaderDispatchStrategy; import graphql.execution.instrumentation.dataloader.PerLevelDataLoaderDispatchStrategy; +import graphql.execution.instrumentation.dataloader.PerLevelDataLoaderDispatchStrategyWithDefer; import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters; import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters; import graphql.extensions.ExtensionsBuilder; @@ -228,7 +229,14 @@ private DataLoaderDispatchStrategy createDataLoaderDispatchStrategy(ExecutionCon return DataLoaderDispatchStrategy.NO_OP; } if (executionStrategy instanceof AsyncExecutionStrategy) { - return new PerLevelDataLoaderDispatchStrategy(executionContext); + boolean deferEnabled = Optional.ofNullable(executionContext.getGraphQLContext()) + .map(graphqlContext -> graphqlContext.getBoolean(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT)) + .orElse(false); + + // Dedicated strategy for defer support, for safety purposes. + return deferEnabled ? + new PerLevelDataLoaderDispatchStrategyWithDefer(executionContext) : + new PerLevelDataLoaderDispatchStrategy(executionContext); } else { return new FallbackDataLoaderDispatchStrategy(executionContext); } diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/DataLoaderDispatcherInstrumentation.java b/src/main/java/graphql/execution/instrumentation/dataloader/DataLoaderDispatcherInstrumentation.java deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index 364f41119f..4402fa1576 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -11,12 +11,9 @@ import graphql.util.LockKit; import org.dataloader.DataLoaderRegistry; -import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; -import java.util.Optional; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; @Internal public class PerLevelDataLoaderDispatchStrategy implements DataLoaderDispatchStrategy { @@ -34,13 +31,6 @@ private static class CallStack { private final LevelMap happenedStrategyCallsPerLevel = new LevelMap(); private final LevelMap happenedOnFieldValueCallsPerLevel = new LevelMap(); - private final LevelMap expectedDeferredFetchCountPerLevel = new LevelMap(); - private final LevelMap deferredFetchCountPerLevel = new LevelMap(); - private final LevelMap expectedDeferredStrategyCallsPerLevel = new LevelMap(); - private final LevelMap happenedOnDeferredFieldValueCallsPerLevel = new LevelMap(); - - private final AtomicBoolean hasDeferredCalls = new AtomicBoolean(false); - private final Set dispatchedLevels = new LinkedHashSet<>(); public CallStack() { @@ -51,27 +41,14 @@ void increaseExpectedFetchCount(int level, int count) { expectedFetchCountPerLevel.increment(level, count); } - void increaseExpectedDeferredFetchCount(int level, int count) { - expectedDeferredFetchCountPerLevel.increment(level, count); - } - void increaseFetchCount(int level) { fetchCountPerLevel.increment(level, 1); } - void increaseDeferredFetchCount(int level) { - deferredFetchCountPerLevel.increment(level, 1); - } - void increaseExpectedStrategyCalls(int level, int count) { expectedStrategyCallsPerLevel.increment(level, count); } - void increaseExpectedDeferredStrategyCalls(int level, int count) { - expectedDeferredStrategyCallsPerLevel.increment(level, count); - hasDeferredCalls.set(true); - } - void increaseHappenedStrategyCalls(int level) { happenedStrategyCallsPerLevel.increment(level, 1); } @@ -80,55 +57,33 @@ void increaseHappenedOnFieldValueCalls(int level) { happenedOnFieldValueCallsPerLevel.increment(level, 1); } - void increaseHappenedOnDeferredFieldValueCalls(int level) { - happenedOnDeferredFieldValueCallsPerLevel.increment(level, 1); - hasDeferredCalls.set(true); - } - boolean allStrategyCallsHappened(int level) { - return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); + return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); } boolean allOnFieldCallsHappened(int level) { - return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); + return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); } boolean allFetchesHappened(int level) { return fetchCountPerLevel.get(level) == expectedFetchCountPerLevel.get(level); } - private boolean hasDeferredCalls() { - return hasDeferredCalls.get(); - } - - private boolean hasDeferredCalls(int level) { - return expectedDeferredStrategyCallsPerLevel.get(level) > 0 || happenedOnDeferredFieldValueCallsPerLevel.get(level) > 0 || - deferredFetchCountPerLevel.get(level) > 0 || expectedDeferredFetchCountPerLevel.get(level) > 0; - } - @Override public String toString() { - return "{" + - "efc=" + expectedFetchCountPerLevel + - ", fc=" + fetchCountPerLevel + - ", esc=" + expectedStrategyCallsPerLevel + - ", hsc=" + happenedStrategyCallsPerLevel + - ", hofvc=" + happenedOnFieldValueCallsPerLevel + - ", de=" + expectedDeferredStrategyCallsPerLevel + - ", dh=" + happenedOnDeferredFieldValueCallsPerLevel + - ", dl" + dispatchedLevels + - ", edc=" + expectedDeferredFetchCountPerLevel + - ", dc=" + deferredFetchCountPerLevel + + return "CallStack{" + + "expectedFetchCountPerLevel=" + expectedFetchCountPerLevel + + ", fetchCountPerLevel=" + fetchCountPerLevel + + ", expectedStrategyCallsPerLevel=" + expectedStrategyCallsPerLevel + + ", happenedStrategyCallsPerLevel=" + happenedStrategyCallsPerLevel + + ", happenedOnFieldValueCallsPerLevel=" + happenedOnFieldValueCallsPerLevel + + ", dispatchedLevels" + dispatchedLevels + '}'; } - public boolean dispatchIfNotDispatchedBefore(int level, String origin) { + public boolean dispatchIfNotDispatchedBefore(int level) { if (dispatchedLevels.contains(level)) { - if(this.hasDeferredCalls(level) || this.hasDeferredCalls(level - 1)) { - System.out.println("df: " + level + " already dispatched."); - return true; - } Assert.assertShouldNeverHappen("level " + level + " already dispatched"); return false; } @@ -143,51 +98,24 @@ public PerLevelDataLoaderDispatchStrategy(ExecutionContext executionContext) { } @Override - public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - - boolean dispatchNeeded = callStack.lock.callLocked(() -> { - callStack.increaseHappenedOnDeferredFieldValueCalls(curLevel); - - int expectedStrategyCalls = getCountForList(Collections.singletonList(fieldValueInfo)); - callStack.increaseExpectedDeferredStrategyCalls(curLevel + 1, expectedStrategyCalls); - -// callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); - - System.out.println( - "df: " + curLevel + " :: " + - callStack + " :: " + - parameters.getPath() + " :: " - ); - -// return false; - return dispatchIfNeeded(curLevel + 1, "df"); - } - ); - if (dispatchNeeded) { - dispatch(curLevel); - } - -// onFieldValuesInfoDispatchIfNeeded(Collections.singletonList(fieldValueInfo), curLevel, parameters, "df"); + public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { + throw new UnsupportedOperationException("Data Loaders cannot be used to resolve deferred fields"); } @Override public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - - increaseCallCounts(curLevel, parameters, "st"); + increaseCallCounts(curLevel, parameters); } @Override public void executionStrategyOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { int curLevel = parameters.getPath().getLevel() + 1; - - onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters, "ev"); + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters); } - @Override - public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getPath().getLevel() + 1; + public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters executionStrategyParameters) { + int curLevel = executionStrategyParameters.getPath().getLevel() + 1; callStack.lock.runLocked(() -> callStack.increaseHappenedOnFieldValueCalls(curLevel) ); @@ -197,21 +125,13 @@ public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrate @Override public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - - increaseCallCounts(curLevel, parameters, "ob"); + increaseCallCounts(curLevel, parameters); } @Override public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { int curLevel = parameters.getPath().getLevel() + 1; - -// System.out.println( -// "ef: " + curLevel + " :: " + -// callStack + " :: " + -// parameters.getPath() -// ); - - onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters, "of"); + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters); } @@ -224,30 +144,17 @@ public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyPa } - private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters, String origin) { - int fieldCount = parameters.getFields().size(); - - int deferredFieldCount = (int) parameters.getFields().getSubFieldsList().stream() - .filter(MergedField::isDeferred) - .count(); - + private void increaseCallCounts(int curLevel, ExecutionStrategyParameters executionStrategyParameters) { + int fieldCount = executionStrategyParameters.getFields().size(); callStack.lock.runLocked(() -> { - callStack.increaseExpectedFetchCount(curLevel, fieldCount - deferredFieldCount); - callStack.increaseExpectedDeferredFetchCount(curLevel, deferredFieldCount); + callStack.increaseExpectedFetchCount(curLevel, fieldCount); callStack.increaseHappenedStrategyCalls(curLevel); }); - - - System.out.println( - origin + ": " + curLevel + " :: " + - callStack + " :: " + - parameters.getPath() - ); } - private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, ExecutionStrategyParameters parameters, String origin) { + private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, ExecutionStrategyParameters parameters) { boolean dispatchNeeded = callStack.lock.callLocked(() -> - handleOnFieldValuesInfo(fieldValueInfoList, curLevel, origin, parameters) + handleOnFieldValuesInfo(fieldValueInfoList, curLevel) ); if (dispatchNeeded) { dispatch(curLevel); @@ -255,28 +162,13 @@ private void onFieldValuesInfoDispatchIfNeeded(List fieldValueIn } // - // thread safety: called with callStack.lock - // - private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel, String origin, ExecutionStrategyParameters parameters) { - boolean isDeferred = Optional.ofNullable(parameters.getField()).map(MergedField::isDeferred).orElse(false); - +// thread safety: called with callStack.lock +// + private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel) { + callStack.increaseHappenedOnFieldValueCalls(curLevel); int expectedStrategyCalls = getCountForList(fieldValueInfos); - if(isDeferred) { - System.out.println("deffZ"); - callStack.increaseExpectedDeferredStrategyCalls(10+curLevel + 1, expectedStrategyCalls); - callStack.increaseHappenedOnDeferredFieldValueCalls(10+curLevel); - } else { - callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); - callStack.increaseHappenedOnFieldValueCalls(curLevel); - } - - System.out.println( - origin + ": " + curLevel + " :: " + - callStack + " :: " + - parameters.getPath() + " :: " - ); - - return dispatchIfNeeded(curLevel + 1, origin); + callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + return dispatchIfNeeded(curLevel + 1); } private int getCountForList(List fieldValueInfos) { @@ -294,27 +186,14 @@ private int getCountForList(List fieldValueInfos) { @Override public void fieldFetched(ExecutionContext executionContext, - ExecutionStrategyParameters parameters, + ExecutionStrategyParameters executionStrategyParameters, DataFetcher dataFetcher, Object fetchedValue) { - int level = parameters.getPath().getLevel(); - boolean isDeferred = parameters.getField().isDeferred(); + int level = executionStrategyParameters.getPath().getLevel(); boolean dispatchNeeded = callStack.lock.callLocked(() -> { - if(isDeferred) { - callStack.increaseDeferredFetchCount(level); - } else { - callStack.increaseFetchCount(level); - } - return dispatchIfNeeded(level, "ff"); + callStack.increaseFetchCount(level); + return dispatchIfNeeded(level); }); - - System.out.println( - "ff: " + level + " :: " + - callStack + " :: " + - parameters.getPath() + " :: " + - fetchedValue - ); - if (dispatchNeeded) { dispatch(level); } @@ -325,31 +204,25 @@ public void fieldFetched(ExecutionContext executionContext, // // thread safety : called with callStack.lock // - private boolean dispatchIfNeeded(int level, String origin) { - boolean ready = levelReady(level, origin); + private boolean dispatchIfNeeded(int level) { + boolean ready = levelReady(level); if (ready) { - System.out.println(level + " dispatched :: " + origin); - return callStack.dispatchIfNotDispatchedBefore(level, origin); + return callStack.dispatchIfNotDispatchedBefore(level); } return false; } // - // thread safety: called with callStack.lock - // - private boolean levelReady(int level, String origin) { +// thread safety: called with callStack.lock +// + private boolean levelReady(int level) { if (level == 1) { // level 1 is special: there is only one strategy call and that's it return callStack.allFetchesHappened(1); } + if (levelReady(level - 1) && callStack.allOnFieldCallsHappened(level - 1) + && callStack.allStrategyCallsHappened(level) && callStack.allFetchesHappened(level)) { - - boolean lr = levelReady(level - 1, "-1"); - boolean aofch = callStack.allOnFieldCallsHappened(level - 1); - boolean atch = callStack.allStrategyCallsHappened(level); - boolean afh = callStack.allFetchesHappened(level); - - if (lr && aofch && atch && afh) { return true; } return false; diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java new file mode 100644 index 0000000000..37a9816993 --- /dev/null +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java @@ -0,0 +1,276 @@ +package graphql.execution.instrumentation.dataloader; + +import graphql.Assert; +import graphql.Internal; +import graphql.execution.DataLoaderDispatchStrategy; +import graphql.execution.ExecutionContext; +import graphql.execution.ExecutionStrategyParameters; +import graphql.execution.FieldValueInfo; +import graphql.execution.MergedField; +import graphql.schema.DataFetcher; +import graphql.util.LockKit; +import org.dataloader.DataLoaderRegistry; + +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +@Internal +public class PerLevelDataLoaderDispatchStrategyWithDefer implements DataLoaderDispatchStrategy { + + private final CallStack callStack; + private final ExecutionContext executionContext; + + + 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 happenedOnFieldValueCallsPerLevel = new LevelMap(); + + private final LevelMap expectedDeferredStrategyCallsPerLevel = new LevelMap(); + + private final Set dispatchedLevels = new LinkedHashSet<>(); + private final Set levelsWithDeferredFields = new LinkedHashSet<>(); + + public CallStack() { + expectedStrategyCallsPerLevel.set(1, 1); + } + + void increaseExpectedFetchCount(int level, int count) { + expectedFetchCountPerLevel.increment(level, count); + } + + void levelHasDeferredFields(int level) { + levelsWithDeferredFields.add(level); + } + + void increaseFetchCount(int level) { + fetchCountPerLevel.increment(level, 1); + } + + void increaseExpectedStrategyCalls(int level, int count) { + expectedStrategyCallsPerLevel.increment(level, count); + } + + void increaseExpectedDeferredStrategyCalls(int level, int count) { + expectedDeferredStrategyCallsPerLevel.increment(level, count); + levelsWithDeferredFields.add(level); + } + + void increaseHappenedStrategyCalls(int level) { + happenedStrategyCallsPerLevel.increment(level, 1); + } + + void increaseHappenedOnFieldValueCalls(int level) { + happenedOnFieldValueCallsPerLevel.increment(level, 1); + } + + boolean allStrategyCallsHappened(int level) { + return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); + } + + boolean allOnFieldCallsHappened(int level) { + return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); + } + + boolean allFetchesHappened(int level) { + return fetchCountPerLevel.get(level) == expectedFetchCountPerLevel.get(level); + } + + @Override + public String toString() { + return "CallStack{" + + "expectedFetchCountPerLevel=" + expectedFetchCountPerLevel + + ", fetchCountPerLevel=" + fetchCountPerLevel + + ", expectedStrategyCallsPerLevel=" + expectedStrategyCallsPerLevel + + ", happenedStrategyCallsPerLevel=" + happenedStrategyCallsPerLevel + + ", happenedOnFieldValueCallsPerLevel=" + happenedOnFieldValueCallsPerLevel + + ", dispatchedLevels" + dispatchedLevels + + '}'; + } + + + public boolean dispatchIfNotDispatchedBefore(int level) { + if (dispatchedLevels.contains(level)) { + // Levels that have deferred fields can be dispatched multiple times. + // For one, we don't want to wait until deferred fields are resolved to dispatch the initial response. + // Also, multiple defer blocks in the same level should not have to wait for each other. + if (this.levelsWithDeferredFields.contains(level) || this.levelsWithDeferredFields.contains(level - 1)) { + return true; + } + Assert.assertShouldNeverHappen("level " + level + " already dispatched"); + return false; + } + dispatchedLevels.add(level); + return true; + } + } + + public PerLevelDataLoaderDispatchStrategyWithDefer(ExecutionContext executionContext) { + this.callStack = new CallStack(); + this.executionContext = executionContext; + } + + @Override + public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; + + onFieldValuesInfoDispatchIfNeeded(Collections.singletonList(fieldValueInfo), curLevel, true); + } + + @Override + public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; + increaseCallCounts(curLevel, parameters); + } + + @Override + public void executionStrategyOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getPath().getLevel() + 1; + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, false); + } + + @Override + public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getPath().getLevel() + 1; + callStack.lock.runLocked(() -> + callStack.increaseHappenedOnFieldValueCalls(curLevel) + ); + } + + + @Override + public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; + increaseCallCounts(curLevel, parameters); + } + + @Override + public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getPath().getLevel() + 1; + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, false); + } + + + @Override + public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getPath().getLevel() + 1; + callStack.lock.runLocked(() -> + callStack.increaseHappenedOnFieldValueCalls(curLevel) + ); + } + + + private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters) { + int fieldCount = parameters.getFields().size(); + + int deferredFieldCount = (int) parameters.getFields().getSubFieldsList().stream() + .filter(MergedField::isDeferred) + .count(); + + callStack.lock.runLocked(() -> { + // Deferred fields should be accounted in the expected fetch count, otherwise they might block the dispatch. + callStack.increaseExpectedFetchCount(curLevel, fieldCount - deferredFieldCount); + callStack.levelHasDeferredFields(curLevel); + callStack.increaseHappenedStrategyCalls(curLevel); + }); + } + + private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, boolean isDeferred) { + boolean dispatchNeeded = callStack.lock.callLocked(() -> + handleOnFieldValuesInfo(fieldValueInfoList, curLevel, isDeferred) + ); + if (dispatchNeeded) { + dispatch(); + } + } + + // + // thread safety: called with callStack.lock + // + private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel, boolean isDeferred) { + int expectedStrategyCalls = getCountForList(fieldValueInfos); + if (isDeferred) { + // Expected strategy/object calls that are deferred are tracked separately, to avoid blocking the dispatching + // of non-deferred fields. + callStack.increaseExpectedDeferredStrategyCalls(curLevel + 1, expectedStrategyCalls); + } else { + callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + callStack.increaseHappenedOnFieldValueCalls(curLevel); + } + + return dispatchIfNeeded(curLevel + 1); + } + + private int getCountForList(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()); + } + } + return result; + } + + + @Override + public void fieldFetched(ExecutionContext executionContext, + ExecutionStrategyParameters parameters, + DataFetcher dataFetcher, + Object fetchedValue) { + int level = parameters.getPath().getLevel(); + boolean isDeferred = parameters.getField().isDeferred(); + boolean dispatchNeeded = callStack.lock.callLocked(() -> { + if (!isDeferred) { + callStack.increaseFetchCount(level); + } + return dispatchIfNeeded(level); + }); + if (dispatchNeeded) { + dispatch(); + } + + } + + + // + // thread safety : called with callStack.lock + // + private boolean dispatchIfNeeded(int level) { + boolean ready = levelReady(level); + if (ready) { + return callStack.dispatchIfNotDispatchedBefore(level); + } + return false; + } + + // + // thread safety: called with callStack.lock + // + private boolean levelReady(int level) { + if (level == 1) { + // level 1 is special: there is only one strategy call and that's it + return callStack.allFetchesHappened(1); + } + if (levelReady(level - 1) && callStack.allOnFieldCallsHappened(level - 1) + && callStack.allStrategyCallsHappened(level) && callStack.allFetchesHappened(level)) { + + return true; + } + return false; + } + + void dispatch() { + DataLoaderRegistry dataLoaderRegistry = executionContext.getDataLoaderRegistry(); + dataLoaderRegistry.dispatchAll(); + } + +} + diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java index 53a957e04f..7f20938b3b 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java @@ -51,11 +51,11 @@ public void useAsyncBatchLoading(boolean flag) { } - public DataFetcher>> shopsDataFetcher = environment -> - supplyAsyncWithSleep(() -> new ArrayList<>(shops.values()), new Random().nextInt(100)); + public DataFetcher>> shopsDataFetcher = + environment -> supplyAsyncWithSleep(() -> new ArrayList<>(shops.values())); public DataFetcher>> expensiveShopsDataFetcher = environment -> - supplyAsyncWithSleep(() -> new ArrayList<>(expensiveShops.values()), new Random().nextInt(100)); + supplyAsyncWithSleep(() -> new ArrayList<>(expensiveShops.values())); // Departments private static Map departments = new LinkedHashMap<>(); @@ -142,23 +142,22 @@ private static List> getProductsForDepartments(List de private CompletableFuture maybeAsyncWithSleep(Supplier> supplier) { if (useAsyncBatchLoading.get()) { - return supplyAsyncWithSleep(supplier, 100) + return supplyAsyncWithSleep(supplier) .thenCompose(cf -> cf); } else { return supplier.get(); } } - private static CompletableFuture supplyAsyncWithSleep(Supplier supplier, long time) { - Supplier sleepSome = sleepSome(supplier, time); + private static CompletableFuture supplyAsyncWithSleep(Supplier supplier) { + Supplier sleepSome = sleepSome(supplier); return CompletableFuture.supplyAsync(sleepSome); } - private static Supplier sleepSome(Supplier supplier, long time) { + private static Supplier sleepSome(Supplier supplier) { return () -> { try { -// Thread.sleep(new Random().nextInt(100)); - Thread.sleep(time); + Thread.sleep(new Random().nextInt(100)); return supplier.get(); } catch (Exception e) { throw new RuntimeException(e); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy index 83058b8c28..5467c87220 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceWithChainedInstrumentationTest.groovy @@ -2,16 +2,14 @@ package graphql.execution.instrumentation.dataloader import graphql.ExecutionInput import graphql.GraphQL -import graphql.incremental.IncrementalExecutionResult import org.dataloader.DataLoaderRegistry import spock.lang.Ignore import spock.lang.Specification import static graphql.ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.assertIncrementalExpensiveData import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedExpensiveData import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpectedData -import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getIncrementalResults +import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getExpensiveQuery import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.getQuery class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification { @@ -33,7 +31,7 @@ class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification when: ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(query) + .query(getQuery()) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -57,7 +55,7 @@ class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification when: ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(expensiveQuery) + .query(getExpensiveQuery(false)) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -105,7 +103,7 @@ class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification batchCompareDataFetchers.useAsyncBatchLoading(true) ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(expensiveQuery) + .query(getExpensiveQuery(false)) .dataLoaderRegistry(dataLoaderRegistry) .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): incrementalSupport]) .build() @@ -121,89 +119,4 @@ class DataLoaderPerformanceWithChainedInstrumentationTest extends Specification incrementalSupport << [true, false] } - def "chainedInstrumentation: data loader will not work with deferred queries"() { - when: - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(deferredQuery) - .dataLoaderRegistry(dataLoaderRegistry) - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .build() - - graphQL.execute(executionInput) - - then: - def exception = thrown(UnsupportedOperationException) - exception.message == "Data Loaders cannot be used to resolve deferred fields" - } - - def "chainedInstrumentation: data loader will work with deferred queries"() { - - when: - - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(deferredQuery) - .dataLoaderRegistry(dataLoaderRegistry) - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .build() - - IncrementalExecutionResult result = graphQL.execute(executionInput) - - then: - result.toSpecification() == expectedInitialDeferredData - - when: - def incrementalResults = getIncrementalResults(result) - - then: - - println(incrementalResults) - // Ordering is non-deterministic, so we assert on the things we know are going to be true. - - incrementalResults.size() == 3 - // only the last payload has "hasNext=true" - incrementalResults[0].hasNext == true - incrementalResults[1].hasNext == true - incrementalResults[2].hasNext == false - - // every payload has only 1 incremental item, and the data is the same for all of them - incrementalResults.every { it.incremental.size() == 1 } - incrementalResults.every { it.incremental[0].data == [wordCount: 45999] } - - // path is different for every payload - incrementalResults.any { it.incremental[0].path == ["posts", 0] } - incrementalResults.any { it.incremental[0].path == ["posts", 1] } - incrementalResults.any { it.incremental[0].path == ["posts", 2] } - - // With deferred results, we don't achieve the same efficiency. - batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 - batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 3 - } - - - @Ignore("Resolution of deferred fields via Data loaders is not yet supported") - def "chainedInstrumentation: data loader will work with deferred queries on multiple levels deep"() { - when: - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .query(expensiveDeferredQuery) - .dataLoaderRegistry(dataLoaderRegistry) - .build() - - IncrementalExecutionResult result = graphQL.execute(executionInput) - - then: - result.toSpecification() == expectedInitialDeferredData - - when: - def incrementalResults = getIncrementalResults(result) - - - then: - assertIncrementalExpensiveData(incrementalResults) - - // With deferred results, we don't achieve the same efficiency. - // The final number of loader calls is non-deterministic, so we can't assert an exact number. - batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() >= 3 - batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() >= 3 - } } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index e73c321ad0..4cc78307c4 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -47,20 +47,6 @@ class DeferWithDataLoaderTest extends Specification { def "query with single deferred field"() { given: def query = getQuery(true, false) -// def defer = true -// def query = """ -// query { -// shops { -// name -// ... @defer(if: $defer) { -// departments { -// name -// } -// } -// } -// } -// -// """ def expectedInitialData = [ data : [ @@ -97,6 +83,82 @@ class DeferWithDataLoaderTest extends Specification { then: combined.errors == null combined.data == expectedData + + // With deferred results, we don't achieve the same efficiency. +// batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 +// batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 3 + } + + def "multiple fields on same defer block"() { + given: + def defer = true + def query = """ + query { + shops { + id + ... @defer(if: $defer) { + name + departments { + name + } + } + } + } + + """ + + def expectedInitialData = [ + data : [ + shops: [ + [id: "shop-1"], + [id: "shop-2"], + [id: "shop-3"], + ] + ], + hasNext: true + ] + + when: + ExecutionInput executionInput = ExecutionInput.newExecutionInput() + .query(query) + .dataLoaderRegistry(dataLoaderRegistry) + .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) + .build() + + IncrementalExecutionResult result = graphQL.execute(executionInput) + + then: + result.toSpecification() == expectedInitialData + + when: + def incrementalResults = getIncrementalResults(result) + + then: + + assertIncrementalResults(incrementalResults, [["shops", 0], ["shops", 1], ["shops", 2]]) + + when: + def combined = combineExecutionResults(result.toSpecification(), incrementalResults) + then: + combined.errors == null + combined.data == [ + shops: [ + [id : "shop-1", name: "Shop 1", + departments: [[name: "Department 1"], + [name: "Department 2"], + [name: "Department 3"] + ]], + [id : "shop-2", name: "Shop 2", + departments: [[name: "Department 4"], + [name: "Department 5"], + [name: "Department 6"] + ]], + [id : "shop-3", name: "Shop 3", + departments: [[name: "Department 7"], + [name: "Department 8"], + [name: "Department 9"]] + ]] + ] } def "query with nested deferred fields"() { From fc2cfd04f90476fdbe93b92a9f6672066880ada4 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Thu, 20 Jun 2024 16:27:51 +1000 Subject: [PATCH 06/21] tidy up --- .../java/graphql/execution/MergedField.java | 4 ++-- .../instrumentation/dataloader/LevelMap.java | 17 ++++++++--------- .../dataloader/DeferWithDataLoaderTest.groovy | 4 ---- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index f70a46fad3..d80b3e8583 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -140,8 +140,8 @@ public List getDeferredExecutions() { } /** - * TODO Javadoc - * @return + * Returns true if this field is part of a deferred execution + * @return true if this field is part of a deferred execution */ @ExperimentalApi public boolean isDeferred() { diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java b/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java index 1df527d3d6..ddf46f643b 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/LevelMap.java @@ -11,7 +11,7 @@ public class LevelMap { // A reasonable default that guarantees no additional allocations for most use cases. - private static final int DEFAULT_INITIAL_SIZE = 8; + private static final int DEFAULT_INITIAL_SIZE = 16; // this array is mutable in both size and contents. private int[] countsByLevel; @@ -54,14 +54,13 @@ private void maybeResize(int level) { @Override public String toString() { - return Arrays.toString(this.countsByLevel); -// StringBuilder result = new StringBuilder(); -// result.append("IntMap["); -// for (int i = 0; i < countsByLevel.length; i++) { -// result.append("level=").append(i).append(",count=").append(countsByLevel[i]).append(" "); -// } -// result.append("]"); -// return result.toString(); + StringBuilder result = new StringBuilder(); + result.append("IntMap["); + for (int i = 0; i < countsByLevel.length; i++) { + result.append("level=").append(i).append(",count=").append(countsByLevel[i]).append(" "); + } + result.append("]"); + return result.toString(); } public String toString(int level) { diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index 4cc78307c4..bcf8a96b57 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -83,10 +83,6 @@ class DeferWithDataLoaderTest extends Specification { then: combined.errors == null combined.data == expectedData - - // With deferred results, we don't achieve the same efficiency. -// batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 -// batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 3 } def "multiple fields on same defer block"() { From ea8a89f9389902af4b74afc50f12939460966107 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Fri, 21 Jun 2024 09:11:36 +1000 Subject: [PATCH 07/21] Change defer callback method name --- .../execution/DataLoaderDispatchStrategy.java | 3 +- .../incremental/DeferredExecutionSupport.java | 2 +- .../PerLevelDataLoaderDispatchStrategy.java | 2 +- ...elDataLoaderDispatchStrategyWithDefer.java | 41 +++++++++---------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java index 18397d1227..9e21b94bfa 100644 --- a/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/DataLoaderDispatchStrategy.java @@ -4,7 +4,6 @@ import graphql.schema.DataFetcher; import java.util.List; -import java.util.concurrent.CompletableFuture; @Internal public interface DataLoaderDispatchStrategy { @@ -50,7 +49,7 @@ default DataFetcher modifyDataFetcher(DataFetcher dataFetcher) { return dataFetcher; } - default void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { + default void executeDeferredOnFieldValueInfo(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { } } diff --git a/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java b/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java index 2c752d10cd..3b8e7efe8a 100644 --- a/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java +++ b/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java @@ -162,7 +162,7 @@ private Supplier executionResultCF = fieldValueResult .thenCompose(fvi -> { - executionContext.getDataLoaderDispatcherStrategy().deferredField(fvi, executionStrategyParameters); + executionContext.getDataLoaderDispatcherStrategy().executeDeferredOnFieldValueInfo(fvi, executionStrategyParameters); return fvi .getFieldValueFuture() diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java index 4402fa1576..60bd36e6f9 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java @@ -98,7 +98,7 @@ public PerLevelDataLoaderDispatchStrategy(ExecutionContext executionContext) { } @Override - public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { + public void executeDeferredOnFieldValueInfo(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { throw new UnsupportedOperationException("Data Loaders cannot be used to resolve deferred fields"); } diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java index 37a9816993..cb215d1595 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java @@ -117,7 +117,7 @@ public PerLevelDataLoaderDispatchStrategyWithDefer(ExecutionContext executionCon } @Override - public void deferredField(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters parameters) { + public void executeDeferredOnFieldValueInfo(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters parameters) { int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; onFieldValuesInfoDispatchIfNeeded(Collections.singletonList(fieldValueInfo), curLevel, true); @@ -165,6 +165,25 @@ public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyPa ); } + @Override + public void fieldFetched(ExecutionContext executionContext, + ExecutionStrategyParameters parameters, + DataFetcher dataFetcher, + Object fetchedValue) { + int level = parameters.getPath().getLevel(); + boolean isDeferred = parameters.getField().isDeferred(); + boolean dispatchNeeded = callStack.lock.callLocked(() -> { + if (!isDeferred) { + callStack.increaseFetchCount(level); + } + return dispatchIfNeeded(level); + }); + if (dispatchNeeded) { + dispatch(); + } + + } + private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters) { int fieldCount = parameters.getFields().size(); @@ -220,26 +239,6 @@ private int getCountForList(List fieldValueInfos) { } - @Override - public void fieldFetched(ExecutionContext executionContext, - ExecutionStrategyParameters parameters, - DataFetcher dataFetcher, - Object fetchedValue) { - int level = parameters.getPath().getLevel(); - boolean isDeferred = parameters.getField().isDeferred(); - boolean dispatchNeeded = callStack.lock.callLocked(() -> { - if (!isDeferred) { - callStack.increaseFetchCount(level); - } - return dispatchIfNeeded(level); - }); - if (dispatchNeeded) { - dispatch(); - } - - } - - // // thread safety : called with callStack.lock // From 37980a0a6f6214ebeac6c9342456a303df2a2d6f Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Mon, 24 Jun 2024 12:41:54 +1000 Subject: [PATCH 08/21] WIP: Notes after pairing with Andi --- ...PerLevelDataLoaderDispatchStrategyWithDefer.java | 13 +++++++++++++ .../dataloader/DeferWithDataLoaderTest.groovy | 6 ++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java index cb215d1595..537a21870f 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java @@ -18,18 +18,31 @@ @Internal public class PerLevelDataLoaderDispatchStrategyWithDefer implements DataLoaderDispatchStrategy { + // ITERATIONS: + // 1 - all defer fields should be ignored. + // 2 - new call stacks for every defer block. private final CallStack callStack; private final ExecutionContext executionContext; + // data fetchers state: 1) not called 2) called but not returned 3) called and resolve + + // TODO: add test for only a scalar being deferred private static class CallStack { private final LockKit.ReentrantLock lock = new LockKit.ReentrantLock(); + + // expected data fetchers method invocations private final LevelMap expectedFetchCountPerLevel = new LevelMap(); + // actual data fetchers that were invoked and returned private final LevelMap fetchCountPerLevel = new LevelMap(); + + // object stuff private final LevelMap expectedStrategyCallsPerLevel = new LevelMap(); private final LevelMap happenedStrategyCallsPerLevel = new LevelMap(); + + // data fetchers methods have returned (returned an actual value, which we can inspect - it is list, non-null, object, etc....) private final LevelMap happenedOnFieldValueCallsPerLevel = new LevelMap(); private final LevelMap expectedDeferredStrategyCallsPerLevel = new LevelMap(); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index bcf8a96b57..b8fdb13e87 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -217,9 +217,11 @@ class DeferWithDataLoaderTest extends Specification { name } } - ... @defer(if: true) { + ... @defer { expensiveShops { - name + departments { + name + } } } } From 23976e0a0ec0731ca2d57afddf1a378d6b6ffd61 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Mon, 29 Jul 2024 10:39:13 +1000 Subject: [PATCH 09/21] Add test for dataloader on field that is not directly under @defer --- .../dataloader/BatchCompare.java | 4 ++ .../dataloader/BatchCompareDataFetchers.java | 16 ++++- .../dataloader/DeferWithDataLoaderTest.groovy | 68 ++++++++++++++++++- .../dataloader/models/Suburb.java | 24 +++++++ .../resources/storesanddepartments.graphqls | 7 ++ 5 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java index 4882406716..7e22236a95 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java @@ -23,6 +23,7 @@ GraphQLSchema buildDataLoaderSchema(BatchCompareDataFetchers batchCompareDataFet .type(TypeRuntimeWiring.newTypeWiring("Query") .dataFetcher("shops", batchCompareDataFetchers.shopsDataFetcher) .dataFetcher("expensiveShops", batchCompareDataFetchers.expensiveShopsDataFetcher) + .dataFetcher("suburb", batchCompareDataFetchers.suburbDataFetcher) ) .type(TypeRuntimeWiring.newTypeWiring("Shop") .dataFetcher("departments", batchCompareDataFetchers.departmentsForShopDataLoaderDataFetcher) @@ -32,6 +33,9 @@ GraphQLSchema buildDataLoaderSchema(BatchCompareDataFetchers batchCompareDataFet .dataFetcher("products", batchCompareDataFetchers.productsForDepartmentDataLoaderDataFetcher) .dataFetcher("expensiveProducts", batchCompareDataFetchers.productsForDepartmentDataLoaderDataFetcher) ) + .type(TypeRuntimeWiring.newTypeWiring("Suburb") + .dataFetcher("shops", batchCompareDataFetchers.shopsForSuburbDataFetcher) + ) .build(); return new SchemaGenerator().makeExecutableSchema(typeDefinitionRegistry, runtimeWiring); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java index 7f20938b3b..7e4fc0ff19 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java @@ -3,6 +3,7 @@ import graphql.execution.instrumentation.dataloader.models.Department; import graphql.execution.instrumentation.dataloader.models.Product; import graphql.execution.instrumentation.dataloader.models.Shop; +import graphql.execution.instrumentation.dataloader.models.Suburb; import graphql.schema.DataFetcher; import org.dataloader.BatchLoader; import org.dataloader.DataLoader; @@ -10,6 +11,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -36,9 +38,11 @@ public void useAsyncBatchLoading(boolean flag) { useAsyncBatchLoading.set(flag); } - // Shops + private static final Map shops = new LinkedHashMap<>(); private static final Map expensiveShops = new LinkedHashMap<>(); + private static final Map suburbs = new LinkedHashMap<>(); + static { shops.put("shop-1", new Shop("shop-1", "Shop 1", Arrays.asList("department-1", "department-2", "department-3"))); @@ -48,6 +52,8 @@ public void useAsyncBatchLoading(boolean flag) { expensiveShops.put("exshop-1", new Shop("exshop-1", "ExShop 1", Arrays.asList("department-1", "department-2", "department-3"))); expensiveShops.put("exshop-2", new Shop("exshop-2", "ExShop 2", Arrays.asList("department-4", "department-5", "department-6"))); expensiveShops.put("exshop-3", new Shop("exshop-3", "ExShop 3", Arrays.asList("department-7", "department-8", "department-9"))); + + suburbs.put("suburb-1", new Suburb("suburb-1", "Suburb 1")); } @@ -57,6 +63,12 @@ public void useAsyncBatchLoading(boolean flag) { public DataFetcher>> expensiveShopsDataFetcher = environment -> supplyAsyncWithSleep(() -> new ArrayList<>(expensiveShops.values())); + public DataFetcher> suburbDataFetcher = environment -> + supplyAsyncWithSleep(() -> suburbs.values().stream() + .filter(suburb -> suburb.getId().equals(environment.getArgument("id"))) + .findFirst() + .orElse(null)); + // Departments private static Map departments = new LinkedHashMap<>(); @@ -140,6 +152,8 @@ private static List> getProductsForDepartments(List de return productsForDepartmentDataLoader.load(department.getId()); }; + public DataFetcher> shopsForSuburbDataFetcher = environment -> shops.values(); + private CompletableFuture maybeAsyncWithSleep(Supplier> supplier) { if (useAsyncBatchLoading.get()) { return supplyAsyncWithSleep(supplier) diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index b8fdb13e87..618e20e665 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -43,6 +43,70 @@ class DeferWithDataLoaderTest extends Specification { } } + def "Field with data loader is not directly under @defer block"() { + given: + def query = """ + query { + shops { + name + } + ... @defer(if: true) { + suburb(id: "suburb-1") { + name + shops { + name + } + } + } + } + """ + def expectedInitialData = [ + data : [ + shops: [ + [name: "Shop 1"], + [name: "Shop 2"], + [name: "Shop 3"], + ] + ], + hasNext: true + ] + + when: + ExecutionInput executionInput = ExecutionInput.newExecutionInput() + .query(query) + .dataLoaderRegistry(dataLoaderRegistry) + .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) + .build() + + IncrementalExecutionResult result = graphQL.execute(executionInput) + + then: + result.toSpecification() == expectedInitialData + + when: + def incrementalResults = getIncrementalResults(result) + + def expectedIncrementalResults = [[ + hasNext: false, + incremental: [[ + path: [], + data: [ + suburb: [ + name : "Suburb 1", + shops: [ + [name: "Shop 1"], + [name: "Shop 2"], + [name: "Shop 3"] + ] + ] + ] + + ]] + ]] + + then: + incrementalResults == expectedIncrementalResults + } def "query with single deferred field"() { given: @@ -219,9 +283,7 @@ class DeferWithDataLoaderTest extends Specification { } ... @defer { expensiveShops { - departments { - name - } + name } } } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java new file mode 100644 index 0000000000..0776c49fa2 --- /dev/null +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java @@ -0,0 +1,24 @@ +package graphql.execution.instrumentation.dataloader.models; + +import org.jetbrains.annotations.NotNull; + +public class Suburb { + private final String id; + private final String name; + + public Suburb(@NotNull String id, @NotNull String name) { + this.id = id; + this.name = name; + } + + @NotNull + public String getId() { + return id; + } + + @NotNull + public String getName() { + return name; + } + +} diff --git a/src/test/resources/storesanddepartments.graphqls b/src/test/resources/storesanddepartments.graphqls index 57c83e3ab7..a604d3eaf1 100644 --- a/src/test/resources/storesanddepartments.graphqls +++ b/src/test/resources/storesanddepartments.graphqls @@ -6,6 +6,13 @@ schema { type Query { shops(howMany : Int = 5): [Shop] expensiveShops(howMany : Int = 5, howLong : Int = 0): [Shop] + suburb(id: ID!): Suburb +} + +type Suburb { + id: ID! + name: String + shops: [Shop] } type Shop { From eff937dd0e60f0d608d657c537818050d5562aed Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Mon, 29 Jul 2024 16:01:00 +1000 Subject: [PATCH 10/21] New approach: always dispatch once we have entered deferred execution --- .../java/graphql/execution/Execution.java | 4 +- ...spatchStrategyWithDeferAlwaysDispatch.java | 274 ++++++++++++++++++ ...oaderDispatchStrategyWithDeferLegacy.java} | 6 +- .../dataloader/BatchCompare.java | 4 - .../dataloader/BatchCompareDataFetchers.java | 13 - .../DataLoaderPerformanceData.groovy | 36 ++- .../dataloader/DeferWithDataLoaderTest.groovy | 98 ++----- .../resources/storesanddepartments.graphqls | 7 - 8 files changed, 323 insertions(+), 119 deletions(-) create mode 100644 src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java rename src/main/java/graphql/execution/instrumentation/dataloader/{PerLevelDataLoaderDispatchStrategyWithDefer.java => PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java} (97%) diff --git a/src/main/java/graphql/execution/Execution.java b/src/main/java/graphql/execution/Execution.java index 4055268362..cf805a84fb 100644 --- a/src/main/java/graphql/execution/Execution.java +++ b/src/main/java/graphql/execution/Execution.java @@ -14,7 +14,7 @@ import graphql.execution.instrumentation.InstrumentationState; import graphql.execution.instrumentation.dataloader.FallbackDataLoaderDispatchStrategy; import graphql.execution.instrumentation.dataloader.PerLevelDataLoaderDispatchStrategy; -import graphql.execution.instrumentation.dataloader.PerLevelDataLoaderDispatchStrategyWithDefer; +import graphql.execution.instrumentation.dataloader.PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch; import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters; import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters; import graphql.extensions.ExtensionsBuilder; @@ -235,7 +235,7 @@ private DataLoaderDispatchStrategy createDataLoaderDispatchStrategy(ExecutionCon // Dedicated strategy for defer support, for safety purposes. return deferEnabled ? - new PerLevelDataLoaderDispatchStrategyWithDefer(executionContext) : + new PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch(executionContext) : new PerLevelDataLoaderDispatchStrategy(executionContext); } else { return new FallbackDataLoaderDispatchStrategy(executionContext); diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java new file mode 100644 index 0000000000..7563cf71aa --- /dev/null +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java @@ -0,0 +1,274 @@ +package graphql.execution.instrumentation.dataloader; + +import graphql.Assert; +import graphql.Internal; +import graphql.execution.DataLoaderDispatchStrategy; +import graphql.execution.ExecutionContext; +import graphql.execution.ExecutionStrategyParameters; +import graphql.execution.FieldValueInfo; +import graphql.schema.DataFetcher; +import graphql.util.LockKit; +import org.dataloader.DataLoaderRegistry; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * The execution of a query can be divided into 2 phases: first, the non-deferred fields are executed and only once + * they are completely resolved, we start to execute the deferred fields. + * The behavior of this Data Loader strategy is quite different during those 2 phases. During the execution of the + * deferred fields the Data Loader will not attempt to dispatch in a optimal way. It will essentially dispatch for + * every field fetched, which is quite ineffective. + * This is the first iteration of the Data Loader strategy with support for @defer, and it will be improved in the + * future. + */ +@Internal +public class PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch implements DataLoaderDispatchStrategy { + + private final CallStack callStack; + private final ExecutionContext executionContext; + + /** + * This flag is used to determine if we are in a deferred context or not. + * The value of this flag is set to true as soon as we identified that a deferred field is being executed, and then + * the flag stays on that state for the remainder of the execution. + */ + private final AtomicBoolean enteredDeferredContext = new AtomicBoolean(false); + + + 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 happenedOnFieldValueCallsPerLevel = new LevelMap(); + + private final Set dispatchedLevels = new LinkedHashSet<>(); + + public CallStack() { + expectedStrategyCallsPerLevel.set(1, 1); + } + + void increaseExpectedFetchCount(int level, int count) { + expectedFetchCountPerLevel.increment(level, count); + } + + void increaseFetchCount(int level) { + fetchCountPerLevel.increment(level, 1); + } + + void increaseExpectedStrategyCalls(int level, int count) { + expectedStrategyCallsPerLevel.increment(level, count); + } + + void increaseHappenedStrategyCalls(int level) { + happenedStrategyCallsPerLevel.increment(level, 1); + } + + void increaseHappenedOnFieldValueCalls(int level) { + happenedOnFieldValueCallsPerLevel.increment(level, 1); + } + + boolean allStrategyCallsHappened(int level) { + return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); + } + + boolean allOnFieldCallsHappened(int level) { + return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level); + } + + boolean allFetchesHappened(int level) { + return fetchCountPerLevel.get(level) == expectedFetchCountPerLevel.get(level); + } + + @Override + public String toString() { + return "CallStack{" + + "expectedFetchCountPerLevel=" + expectedFetchCountPerLevel + + ", fetchCountPerLevel=" + fetchCountPerLevel + + ", expectedStrategyCallsPerLevel=" + expectedStrategyCallsPerLevel + + ", happenedStrategyCallsPerLevel=" + happenedStrategyCallsPerLevel + + ", happenedOnFieldValueCallsPerLevel=" + happenedOnFieldValueCallsPerLevel + + ", dispatchedLevels" + dispatchedLevels + + '}'; + } + + + public boolean dispatchIfNotDispatchedBefore(int level) { + if (dispatchedLevels.contains(level)) { + Assert.assertShouldNeverHappen("level " + level + " already dispatched"); + return false; + } + dispatchedLevels.add(level); + return true; + } + } + + public PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch(ExecutionContext executionContext) { + this.callStack = new CallStack(); + this.executionContext = executionContext; + } + + @Override + public void executeDeferredOnFieldValueInfo(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { + this.enteredDeferredContext.set(true); + } + + @Override + public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + if (this.enteredDeferredContext.get()) { + return; + } + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; + increaseCallCounts(curLevel, parameters); + } + + @Override + public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + if (this.enteredDeferredContext.get()) { + return; + } + int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; + increaseCallCounts(curLevel, parameters); + } + + @Override + public void executionStrategyOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { + if (this.enteredDeferredContext.get()) { + this.dispatch(); + } + int curLevel = parameters.getPath().getLevel() + 1; + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters); + } + + @Override + public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters executionStrategyParameters) { + int curLevel = executionStrategyParameters.getPath().getLevel() + 1; + callStack.lock.runLocked(() -> + callStack.increaseHappenedOnFieldValueCalls(curLevel) + ); + } + + @Override + public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { + if (this.enteredDeferredContext.get()) { + this.dispatch(); + } + int curLevel = parameters.getPath().getLevel() + 1; + onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, parameters); + } + + + @Override + public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { + int curLevel = parameters.getPath().getLevel() + 1; + callStack.lock.runLocked(() -> + callStack.increaseHappenedOnFieldValueCalls(curLevel) + ); + } + + @Override + public void fieldFetched(ExecutionContext executionContext, + ExecutionStrategyParameters parameters, + DataFetcher dataFetcher, + Object fetchedValue) { + + final boolean dispatchNeeded; + + if (parameters.getField().isDeferred() || this.enteredDeferredContext.get()) { + this.enteredDeferredContext.compareAndSet(false, 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 nonDeferredFieldCount = (int) parameters.getFields().getSubFieldsList().stream() + .filter(field -> !field.isDeferred()) + .count(); + + callStack.lock.runLocked(() -> { + callStack.increaseExpectedFetchCount(curLevel, nonDeferredFieldCount); + callStack.increaseHappenedStrategyCalls(curLevel); + }); + } + + private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, ExecutionStrategyParameters parameters) { + boolean dispatchNeeded = callStack.lock.callLocked(() -> + handleOnFieldValuesInfo(fieldValueInfoList, curLevel) + ); + if (dispatchNeeded) { + dispatch(); + } + } + + // + // thread safety: called with callStack.lock + // + private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel) { + callStack.increaseHappenedOnFieldValueCalls(curLevel); + int expectedStrategyCalls = getCountForList(fieldValueInfos); + callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); + return dispatchIfNeeded(curLevel + 1); + } + + private int getCountForList(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()); + } + } + return result; + } + + // + // thread safety : called with callStack.lock + // + private boolean dispatchIfNeeded(int level) { + boolean ready = levelReady(level); + if (ready) { + return callStack.dispatchIfNotDispatchedBefore(level); + } + return false; + } + + // + // thread safety: called with callStack.lock + // + private boolean levelReady(int level) { + if (level == 1) { + // level 1 is special: there is only one strategy call and that's it + return callStack.allFetchesHappened(1); + } + if (levelReady(level - 1) && callStack.allOnFieldCallsHappened(level - 1) + && callStack.allStrategyCallsHappened(level) && callStack.allFetchesHappened(level)) { + + return true; + } + return false; + } + + void dispatch() { + DataLoaderRegistry dataLoaderRegistry = executionContext.getDataLoaderRegistry(); + dataLoaderRegistry.dispatchAll(); + } + +} + diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java similarity index 97% rename from src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java rename to src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java index 537a21870f..d58b8b51bb 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDefer.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java @@ -17,9 +17,11 @@ import java.util.Set; @Internal -public class PerLevelDataLoaderDispatchStrategyWithDefer implements DataLoaderDispatchStrategy { +public class PerLevelDataLoaderDispatchStrategyWithDeferLegacy implements DataLoaderDispatchStrategy { // ITERATIONS: // 1 - all defer fields should be ignored. + // - when we identify that a field is deferred, we should just dispatch. This will result in multiple dispatches, which is not optimal. + // 2 - new call stacks for every defer block. private final CallStack callStack; @@ -124,7 +126,7 @@ public boolean dispatchIfNotDispatchedBefore(int level) { } } - public PerLevelDataLoaderDispatchStrategyWithDefer(ExecutionContext executionContext) { + public PerLevelDataLoaderDispatchStrategyWithDeferLegacy(ExecutionContext executionContext) { this.callStack = new CallStack(); this.executionContext = executionContext; } diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java index 7e22236a95..4882406716 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompare.java @@ -23,7 +23,6 @@ GraphQLSchema buildDataLoaderSchema(BatchCompareDataFetchers batchCompareDataFet .type(TypeRuntimeWiring.newTypeWiring("Query") .dataFetcher("shops", batchCompareDataFetchers.shopsDataFetcher) .dataFetcher("expensiveShops", batchCompareDataFetchers.expensiveShopsDataFetcher) - .dataFetcher("suburb", batchCompareDataFetchers.suburbDataFetcher) ) .type(TypeRuntimeWiring.newTypeWiring("Shop") .dataFetcher("departments", batchCompareDataFetchers.departmentsForShopDataLoaderDataFetcher) @@ -33,9 +32,6 @@ GraphQLSchema buildDataLoaderSchema(BatchCompareDataFetchers batchCompareDataFet .dataFetcher("products", batchCompareDataFetchers.productsForDepartmentDataLoaderDataFetcher) .dataFetcher("expensiveProducts", batchCompareDataFetchers.productsForDepartmentDataLoaderDataFetcher) ) - .type(TypeRuntimeWiring.newTypeWiring("Suburb") - .dataFetcher("shops", batchCompareDataFetchers.shopsForSuburbDataFetcher) - ) .build(); return new SchemaGenerator().makeExecutableSchema(typeDefinitionRegistry, runtimeWiring); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java index 7e4fc0ff19..d0dacd5964 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/BatchCompareDataFetchers.java @@ -3,7 +3,6 @@ import graphql.execution.instrumentation.dataloader.models.Department; import graphql.execution.instrumentation.dataloader.models.Product; import graphql.execution.instrumentation.dataloader.models.Shop; -import graphql.execution.instrumentation.dataloader.models.Suburb; import graphql.schema.DataFetcher; import org.dataloader.BatchLoader; import org.dataloader.DataLoader; @@ -41,8 +40,6 @@ public void useAsyncBatchLoading(boolean flag) { private static final Map shops = new LinkedHashMap<>(); private static final Map expensiveShops = new LinkedHashMap<>(); - private static final Map suburbs = new LinkedHashMap<>(); - static { shops.put("shop-1", new Shop("shop-1", "Shop 1", Arrays.asList("department-1", "department-2", "department-3"))); @@ -52,8 +49,6 @@ public void useAsyncBatchLoading(boolean flag) { expensiveShops.put("exshop-1", new Shop("exshop-1", "ExShop 1", Arrays.asList("department-1", "department-2", "department-3"))); expensiveShops.put("exshop-2", new Shop("exshop-2", "ExShop 2", Arrays.asList("department-4", "department-5", "department-6"))); expensiveShops.put("exshop-3", new Shop("exshop-3", "ExShop 3", Arrays.asList("department-7", "department-8", "department-9"))); - - suburbs.put("suburb-1", new Suburb("suburb-1", "Suburb 1")); } @@ -63,12 +58,6 @@ public void useAsyncBatchLoading(boolean flag) { public DataFetcher>> expensiveShopsDataFetcher = environment -> supplyAsyncWithSleep(() -> new ArrayList<>(expensiveShops.values())); - public DataFetcher> suburbDataFetcher = environment -> - supplyAsyncWithSleep(() -> suburbs.values().stream() - .filter(suburb -> suburb.getId().equals(environment.getArgument("id"))) - .findFirst() - .orElse(null)); - // Departments private static Map departments = new LinkedHashMap<>(); @@ -152,8 +141,6 @@ private static List> getProductsForDepartments(List de return productsForDepartmentDataLoader.load(department.getId()); }; - public DataFetcher> shopsForSuburbDataFetcher = environment -> shops.values(); - private CompletableFuture maybeAsyncWithSleep(Supplier> supplier) { if (useAsyncBatchLoading.get()) { return supplyAsyncWithSleep(supplier) diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy index 54a46b69ac..2df1aba3ef 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy @@ -202,35 +202,33 @@ class DataLoaderPerformanceData { } } } - ... @defer(if: false) { - expensiveShops { + expensiveShops { + name + departments { name - departments { + products { + name + } + ... @defer(if: $deferredEnabled) { + expensiveProducts { + name + } + } + } + ... @defer(if: $deferredEnabled) { + expensiveDepartments { name products { name } - ... @defer(if: $deferredEnabled) { + ... @defer(if: $deferredEnabled) { expensiveProducts { name } } } - ... @defer(if: $deferredEnabled) { - expensiveDepartments { - name - products { - name - } - ... @defer(if: $deferredEnabled) { - expensiveProducts { - name - } - } - } - } - } - } + } + } } """ diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index 618e20e665..5268f4d094 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -7,6 +7,8 @@ import graphql.incremental.IncrementalExecutionResult import org.dataloader.DataLoaderRegistry import spock.lang.Specification +import java.util.stream.Collectors + import static graphql.ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.combineExecutionResults import static graphql.execution.instrumentation.dataloader.DataLoaderPerformanceData.expectedData @@ -30,6 +32,10 @@ class DeferWithDataLoaderTest extends Specification { graphQL = dataLoaderPerformanceData.setupGraphQL() } + /** + * @param results a list of the incremental results from the execution + * @param expectedPaths a list of the expected paths in the incremental results. The order of the elements in the list is not important. + */ private static void assertIncrementalResults(List> results, List> expectedPaths) { assert results.size() == expectedPaths.size(), "Expected ${expectedPaths.size()} results, got ${results.size()}" @@ -43,71 +49,6 @@ class DeferWithDataLoaderTest extends Specification { } } - def "Field with data loader is not directly under @defer block"() { - given: - def query = """ - query { - shops { - name - } - ... @defer(if: true) { - suburb(id: "suburb-1") { - name - shops { - name - } - } - } - } - """ - def expectedInitialData = [ - data : [ - shops: [ - [name: "Shop 1"], - [name: "Shop 2"], - [name: "Shop 3"], - ] - ], - hasNext: true - ] - - when: - ExecutionInput executionInput = ExecutionInput.newExecutionInput() - .query(query) - .dataLoaderRegistry(dataLoaderRegistry) - .graphQLContext([(ENABLE_INCREMENTAL_SUPPORT): true]) - .build() - - IncrementalExecutionResult result = graphQL.execute(executionInput) - - then: - result.toSpecification() == expectedInitialData - - when: - def incrementalResults = getIncrementalResults(result) - - def expectedIncrementalResults = [[ - hasNext: false, - incremental: [[ - path: [], - data: [ - suburb: [ - name : "Suburb 1", - shops: [ - [name: "Shop 1"], - [name: "Shop 2"], - [name: "Shop 3"] - ] - ] - ] - - ]] - ]] - - then: - incrementalResults == expectedIncrementalResults - } - def "query with single deferred field"() { given: def query = getQuery(true, false) @@ -147,16 +88,18 @@ class DeferWithDataLoaderTest extends Specification { then: combined.errors == null combined.data == expectedData + + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 9 } def "multiple fields on same defer block"() { given: - def defer = true def query = """ query { shops { id - ... @defer(if: $defer) { + ... @defer { name departments { name @@ -219,6 +162,8 @@ class DeferWithDataLoaderTest extends Specification { [name: "Department 9"]] ]] ] + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 0 } def "query with nested deferred fields"() { @@ -270,6 +215,9 @@ class DeferWithDataLoaderTest extends Specification { then: combined.errors == null combined.data == expectedData + + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 3 + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 9 } def "query with top-level deferred field"() { @@ -330,6 +278,9 @@ class DeferWithDataLoaderTest extends Specification { [departments: [[name: "Department 4"], [name: "Department 5"], [name: "Department 6"]]], [departments: [[name: "Department 7"], [name: "Department 8"], [name: "Department 9"]]]], expensiveShops: [[name: "ExShop 1"], [name: "ExShop 2"], [name: "ExShop 3"]]] + + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 1 + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 0 } def "query with multiple deferred fields"() { @@ -367,16 +318,15 @@ class DeferWithDataLoaderTest extends Specification { def incrementalResults = getIncrementalResults(result) then: - // Ordering is non-deterministic, so we assert on the things we know are going to be true. assertIncrementalResults(incrementalResults, [ ["expensiveShops", 0], ["expensiveShops", 1], ["expensiveShops", 2], ["shops", 0], ["shops", 1], ["shops", 2], - ["shops", 2, "departments", 0], ["shops", 1, "departments", 2],["shops", 1, "departments", 1], ["shops", 1, "departments", 0],["shops", 0, "departments", 2], ["shops", 0, "departments", 1], ["shops", 0, "departments", 0],["shops", 2, "departments", 2],["shops", 2, "departments", 1], - ["shops", 1, "expensiveDepartments", 2], ["shops", 1, "expensiveDepartments", 1], ["shops", 1, "expensiveDepartments", 0], ["shops", 0, "expensiveDepartments", 2], ["shops", 0, "expensiveDepartments", 1], ["shops", 0, "expensiveDepartments", 0], ["shops", 2, "expensiveDepartments", 1], ["shops", 2, "expensiveDepartments", 2], - ["expensiveShops", 2, "expensiveDepartments", 2], ["expensiveShops", 2, "expensiveDepartments", 1], ["expensiveShops", 2, "expensiveDepartments", 0], ["expensiveShops", 2, "departments", 2], ["expensiveShops", 1, "expensiveDepartments", 2], ["expensiveShops", 1, "expensiveDepartments", 1], ["expensiveShops", 1, "expensiveDepartments", 0], ["expensiveShops", 0, "expensiveDepartments", 2], ["expensiveShops", 0, "expensiveDepartments", 1], ["expensiveShops", 0, "expensiveDepartments", 0], - ["expensiveShops", 2, "departments", 1], ["expensiveShops", 2, "departments", 0], ["expensiveShops", 1, "departments", 2], ["expensiveShops", 1, "departments", 1], ["expensiveShops", 1, "departments", 0], ["expensiveShops", 0, "departments", 2], ["expensiveShops", 0, "departments", 1], ["expensiveShops", 0, "departments", 0], ["shops", 2, "expensiveDepartments", 0]] + ["shops", 0, "departments", 0], ["shops", 0, "departments", 1],["shops", 0, "departments", 2], ["shops", 1, "departments", 0],["shops", 1, "departments", 1], ["shops", 1, "departments", 2], ["shops", 2, "departments", 0],["shops", 2, "departments", 1],["shops", 2, "departments", 2], + ["shops", 0, "expensiveDepartments", 0], ["shops", 0, "expensiveDepartments", 1], ["shops", 0, "expensiveDepartments", 2], ["shops", 1, "expensiveDepartments", 0], ["shops", 1, "expensiveDepartments", 1], ["shops", 1, "expensiveDepartments", 2], ["shops", 2, "expensiveDepartments", 0], ["shops", 2, "expensiveDepartments", 1],["shops", 2, "expensiveDepartments", 2], + ["expensiveShops", 0, "expensiveDepartments", 0], ["expensiveShops", 0, "expensiveDepartments", 1], ["expensiveShops", 0, "expensiveDepartments", 2], ["expensiveShops", 1, "expensiveDepartments", 0], ["expensiveShops", 1, "expensiveDepartments", 1], ["expensiveShops", 1, "expensiveDepartments", 2], ["expensiveShops", 2, "expensiveDepartments", 0], ["expensiveShops", 2, "expensiveDepartments", 1],["expensiveShops", 2, "expensiveDepartments", 2], + ["expensiveShops", 0, "departments", 0], ["expensiveShops", 0, "departments", 1], ["expensiveShops", 0, "departments", 2], ["expensiveShops", 1, "departments", 0], ["expensiveShops", 1, "departments", 1], ["expensiveShops", 1, "departments", 2], ["expensiveShops", 2, "departments", 0], ["expensiveShops", 2, "departments", 1],["expensiveShops", 2, "departments", 2]] ) when: @@ -384,6 +334,10 @@ class DeferWithDataLoaderTest extends Specification { then: combined.errors == null combined.data == expectedExpensiveData + + // TODO: Why the load counters are only 1? + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 1 + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() == 1 } } diff --git a/src/test/resources/storesanddepartments.graphqls b/src/test/resources/storesanddepartments.graphqls index a604d3eaf1..57c83e3ab7 100644 --- a/src/test/resources/storesanddepartments.graphqls +++ b/src/test/resources/storesanddepartments.graphqls @@ -6,13 +6,6 @@ schema { type Query { shops(howMany : Int = 5): [Shop] expensiveShops(howMany : Int = 5, howLong : Int = 0): [Shop] - suburb(id: ID!): Suburb -} - -type Suburb { - id: ID! - name: String - shops: [Shop] } type Shop { From 92ef8993457776527faf02c14a6f141492627e51 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Thu, 1 Aug 2024 14:11:10 +1000 Subject: [PATCH 11/21] Remove old implementation --- ...LoaderDispatchStrategyWithDeferLegacy.java | 290 ------------------ 1 file changed, 290 deletions(-) delete mode 100644 src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java deleted file mode 100644 index d58b8b51bb..0000000000 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferLegacy.java +++ /dev/null @@ -1,290 +0,0 @@ -package graphql.execution.instrumentation.dataloader; - -import graphql.Assert; -import graphql.Internal; -import graphql.execution.DataLoaderDispatchStrategy; -import graphql.execution.ExecutionContext; -import graphql.execution.ExecutionStrategyParameters; -import graphql.execution.FieldValueInfo; -import graphql.execution.MergedField; -import graphql.schema.DataFetcher; -import graphql.util.LockKit; -import org.dataloader.DataLoaderRegistry; - -import java.util.Collections; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Set; - -@Internal -public class PerLevelDataLoaderDispatchStrategyWithDeferLegacy implements DataLoaderDispatchStrategy { - // ITERATIONS: - // 1 - all defer fields should be ignored. - // - when we identify that a field is deferred, we should just dispatch. This will result in multiple dispatches, which is not optimal. - - // 2 - new call stacks for every defer block. - - private final CallStack callStack; - private final ExecutionContext executionContext; - - // data fetchers state: 1) not called 2) called but not returned 3) called and resolve - - // TODO: add test for only a scalar being deferred - - private static class CallStack { - - private final LockKit.ReentrantLock lock = new LockKit.ReentrantLock(); - - // expected data fetchers method invocations - private final LevelMap expectedFetchCountPerLevel = new LevelMap(); - // actual data fetchers that were invoked and returned - private final LevelMap fetchCountPerLevel = new LevelMap(); - - // object stuff - private final LevelMap expectedStrategyCallsPerLevel = new LevelMap(); - private final LevelMap happenedStrategyCallsPerLevel = new LevelMap(); - - // data fetchers methods have returned (returned an actual value, which we can inspect - it is list, non-null, object, etc....) - private final LevelMap happenedOnFieldValueCallsPerLevel = new LevelMap(); - - private final LevelMap expectedDeferredStrategyCallsPerLevel = new LevelMap(); - - private final Set dispatchedLevels = new LinkedHashSet<>(); - private final Set levelsWithDeferredFields = new LinkedHashSet<>(); - - public CallStack() { - expectedStrategyCallsPerLevel.set(1, 1); - } - - void increaseExpectedFetchCount(int level, int count) { - expectedFetchCountPerLevel.increment(level, count); - } - - void levelHasDeferredFields(int level) { - levelsWithDeferredFields.add(level); - } - - void increaseFetchCount(int level) { - fetchCountPerLevel.increment(level, 1); - } - - void increaseExpectedStrategyCalls(int level, int count) { - expectedStrategyCallsPerLevel.increment(level, count); - } - - void increaseExpectedDeferredStrategyCalls(int level, int count) { - expectedDeferredStrategyCallsPerLevel.increment(level, count); - levelsWithDeferredFields.add(level); - } - - void increaseHappenedStrategyCalls(int level) { - happenedStrategyCallsPerLevel.increment(level, 1); - } - - void increaseHappenedOnFieldValueCalls(int level) { - happenedOnFieldValueCallsPerLevel.increment(level, 1); - } - - boolean allStrategyCallsHappened(int level) { - return happenedStrategyCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); - } - - boolean allOnFieldCallsHappened(int level) { - return happenedOnFieldValueCallsPerLevel.get(level) == expectedStrategyCallsPerLevel.get(level) + expectedDeferredStrategyCallsPerLevel.get(level); - } - - boolean allFetchesHappened(int level) { - return fetchCountPerLevel.get(level) == expectedFetchCountPerLevel.get(level); - } - - @Override - public String toString() { - return "CallStack{" + - "expectedFetchCountPerLevel=" + expectedFetchCountPerLevel + - ", fetchCountPerLevel=" + fetchCountPerLevel + - ", expectedStrategyCallsPerLevel=" + expectedStrategyCallsPerLevel + - ", happenedStrategyCallsPerLevel=" + happenedStrategyCallsPerLevel + - ", happenedOnFieldValueCallsPerLevel=" + happenedOnFieldValueCallsPerLevel + - ", dispatchedLevels" + dispatchedLevels + - '}'; - } - - - public boolean dispatchIfNotDispatchedBefore(int level) { - if (dispatchedLevels.contains(level)) { - // Levels that have deferred fields can be dispatched multiple times. - // For one, we don't want to wait until deferred fields are resolved to dispatch the initial response. - // Also, multiple defer blocks in the same level should not have to wait for each other. - if (this.levelsWithDeferredFields.contains(level) || this.levelsWithDeferredFields.contains(level - 1)) { - return true; - } - Assert.assertShouldNeverHappen("level " + level + " already dispatched"); - return false; - } - dispatchedLevels.add(level); - return true; - } - } - - public PerLevelDataLoaderDispatchStrategyWithDeferLegacy(ExecutionContext executionContext) { - this.callStack = new CallStack(); - this.executionContext = executionContext; - } - - @Override - public void executeDeferredOnFieldValueInfo(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - - onFieldValuesInfoDispatchIfNeeded(Collections.singletonList(fieldValueInfo), curLevel, true); - } - - @Override - public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - increaseCallCounts(curLevel, parameters); - } - - @Override - public void executionStrategyOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getPath().getLevel() + 1; - onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, false); - } - - @Override - public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getPath().getLevel() + 1; - callStack.lock.runLocked(() -> - callStack.increaseHappenedOnFieldValueCalls(curLevel) - ); - } - - - @Override - public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; - increaseCallCounts(curLevel, parameters); - } - - @Override - public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getPath().getLevel() + 1; - onFieldValuesInfoDispatchIfNeeded(fieldValueInfoList, curLevel, false); - } - - - @Override - public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) { - int curLevel = parameters.getPath().getLevel() + 1; - callStack.lock.runLocked(() -> - callStack.increaseHappenedOnFieldValueCalls(curLevel) - ); - } - - @Override - public void fieldFetched(ExecutionContext executionContext, - ExecutionStrategyParameters parameters, - DataFetcher dataFetcher, - Object fetchedValue) { - int level = parameters.getPath().getLevel(); - boolean isDeferred = parameters.getField().isDeferred(); - boolean dispatchNeeded = callStack.lock.callLocked(() -> { - if (!isDeferred) { - callStack.increaseFetchCount(level); - } - return dispatchIfNeeded(level); - }); - if (dispatchNeeded) { - dispatch(); - } - - } - - - private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters) { - int fieldCount = parameters.getFields().size(); - - int deferredFieldCount = (int) parameters.getFields().getSubFieldsList().stream() - .filter(MergedField::isDeferred) - .count(); - - callStack.lock.runLocked(() -> { - // Deferred fields should be accounted in the expected fetch count, otherwise they might block the dispatch. - callStack.increaseExpectedFetchCount(curLevel, fieldCount - deferredFieldCount); - callStack.levelHasDeferredFields(curLevel); - callStack.increaseHappenedStrategyCalls(curLevel); - }); - } - - private void onFieldValuesInfoDispatchIfNeeded(List fieldValueInfoList, int curLevel, boolean isDeferred) { - boolean dispatchNeeded = callStack.lock.callLocked(() -> - handleOnFieldValuesInfo(fieldValueInfoList, curLevel, isDeferred) - ); - if (dispatchNeeded) { - dispatch(); - } - } - - // - // thread safety: called with callStack.lock - // - private boolean handleOnFieldValuesInfo(List fieldValueInfos, int curLevel, boolean isDeferred) { - int expectedStrategyCalls = getCountForList(fieldValueInfos); - if (isDeferred) { - // Expected strategy/object calls that are deferred are tracked separately, to avoid blocking the dispatching - // of non-deferred fields. - callStack.increaseExpectedDeferredStrategyCalls(curLevel + 1, expectedStrategyCalls); - } else { - callStack.increaseExpectedStrategyCalls(curLevel + 1, expectedStrategyCalls); - callStack.increaseHappenedOnFieldValueCalls(curLevel); - } - - return dispatchIfNeeded(curLevel + 1); - } - - private int getCountForList(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()); - } - } - return result; - } - - - // - // thread safety : called with callStack.lock - // - private boolean dispatchIfNeeded(int level) { - boolean ready = levelReady(level); - if (ready) { - return callStack.dispatchIfNotDispatchedBefore(level); - } - return false; - } - - // - // thread safety: called with callStack.lock - // - private boolean levelReady(int level) { - if (level == 1) { - // level 1 is special: there is only one strategy call and that's it - return callStack.allFetchesHappened(1); - } - if (levelReady(level - 1) && callStack.allOnFieldCallsHappened(level - 1) - && callStack.allStrategyCallsHappened(level) && callStack.allFetchesHappened(level)) { - - return true; - } - return false; - } - - void dispatch() { - DataLoaderRegistry dataLoaderRegistry = executionContext.getDataLoaderRegistry(); - dataLoaderRegistry.dispatchAll(); - } - -} - From 1f94d7e8a5a1ef6ba44ae3dc1914a5f53781b160 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Thu, 1 Aug 2024 14:19:47 +1000 Subject: [PATCH 12/21] Removed unused data class --- .../dataloader/models/Suburb.java | 24 ------------------- 1 file changed, 24 deletions(-) delete mode 100644 src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java b/src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java deleted file mode 100644 index 0776c49fa2..0000000000 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/models/Suburb.java +++ /dev/null @@ -1,24 +0,0 @@ -package graphql.execution.instrumentation.dataloader.models; - -import org.jetbrains.annotations.NotNull; - -public class Suburb { - private final String id; - private final String name; - - public Suburb(@NotNull String id, @NotNull String name) { - this.id = id; - this.name = name; - } - - @NotNull - public String getId() { - return id; - } - - @NotNull - public String getName() { - return name; - } - -} From 9ea3fa78e0cc7317e214254e71eaf9a38a350833 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Aug 2024 16:47:02 +0000 Subject: [PATCH 13/21] Bump google-github-actions/auth from 2.1.3 to 2.1.4 Bumps [google-github-actions/auth](https://github.com/google-github-actions/auth) from 2.1.3 to 2.1.4. - [Release notes](https://github.com/google-github-actions/auth/releases) - [Changelog](https://github.com/google-github-actions/auth/blob/main/CHANGELOG.md) - [Commits](https://github.com/google-github-actions/auth/compare/v2.1.3...v2.1.4) --- updated-dependencies: - dependency-name: google-github-actions/auth dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/invoke_test_runner.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/invoke_test_runner.yml b/.github/workflows/invoke_test_runner.yml index 61be8d20da..9a3ec7e102 100644 --- a/.github/workflows/invoke_test_runner.yml +++ b/.github/workflows/invoke_test_runner.yml @@ -50,7 +50,7 @@ jobs: - id: 'auth' name: 'Authenticate to Google Cloud' - uses: google-github-actions/auth@v2.1.3 + uses: google-github-actions/auth@v2.1.4 with: credentials_json: ${{ secrets.GOOGLE_APPLICATION_CREDENTIALS }} From af3a193f40c81e302c9d22f1262447635a107648 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 19 Aug 2024 16:51:52 +0000 Subject: [PATCH 14/21] Bump io.projectreactor:reactor-core from 3.6.8 to 3.6.9 Bumps [io.projectreactor:reactor-core](https://github.com/reactor/reactor-core) from 3.6.8 to 3.6.9. - [Release notes](https://github.com/reactor/reactor-core/releases) - [Commits](https://github.com/reactor/reactor-core/compare/v3.6.8...v3.6.9) --- updated-dependencies: - dependency-name: io.projectreactor:reactor-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 07a6456b03..07509bb943 100644 --- a/build.gradle +++ b/build.gradle @@ -117,7 +117,7 @@ dependencies { testImplementation 'org.reactivestreams:reactive-streams-tck:' + reactiveStreamsVersion testImplementation "io.reactivex.rxjava2:rxjava:2.2.21" - testImplementation "io.projectreactor:reactor-core:3.6.8" + testImplementation "io.projectreactor:reactor-core:3.6.9" testImplementation 'org.testng:testng:7.10.2' // use for reactive streams test inheritance From 38c808d0b583a4461b7cf3c4ce67280efb19edbc Mon Sep 17 00:00:00 2001 From: James Bellenger Date: Fri, 23 Aug 2024 10:20:33 -0700 Subject: [PATCH 15/21] . --- .../incremental/IncrementalExecutionResultImpl.java | 8 ++++++++ .../incremental/IncrementalExecutionResultTest.groovy | 11 +++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/incremental/IncrementalExecutionResultImpl.java b/src/main/java/graphql/incremental/IncrementalExecutionResultImpl.java index 77b05c6fe5..704d1b67cc 100644 --- a/src/main/java/graphql/incremental/IncrementalExecutionResultImpl.java +++ b/src/main/java/graphql/incremental/IncrementalExecutionResultImpl.java @@ -10,6 +10,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.function.Consumer; import java.util.stream.Collectors; @ExperimentalApi @@ -52,6 +53,13 @@ public static Builder fromExecutionResult(ExecutionResult executionResult) { return new Builder().from(executionResult); } + @Override + public IncrementalExecutionResult transform(Consumer> builderConsumer) { + var builder = fromExecutionResult(this); + builderConsumer.accept(builder); + return builder.build(); + } + @Override public Map toSpecification() { Map map = new LinkedHashMap<>(super.toSpecification()); diff --git a/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy b/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy index 9bf954cb63..d4661740b9 100644 --- a/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy +++ b/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy @@ -1,9 +1,7 @@ package graphql.incremental import graphql.execution.ResultPath -import groovy.json.JsonOutput import io.reactivex.Flowable -import org.reactivestreams.Publisher import spock.lang.Specification import static graphql.incremental.DeferPayload.newDeferredItem @@ -120,4 +118,13 @@ class IncrementalExecutionResultTest extends Specification { newIncrementalExecutionResult.hasNext() == incrementalExecutionResult.hasNext() newIncrementalExecutionResult.toSpecification() == incrementalExecutionResult.toSpecification() } + + def "transform returns IncrementalExecutionResult"() { + when: + def initial = newIncrementalExecutionResult().build() + + then: + def transformed = initial.transform { } + transformed instanceof IncrementalExecutionResult + } } From 00774a9a5cd14a4ea515ea72c23e3a98b0efe6b0 Mon Sep 17 00:00:00 2001 From: James Bellenger Date: Fri, 23 Aug 2024 12:52:47 -0700 Subject: [PATCH 16/21] tweaks --- .../incremental/IncrementalExecutionResultTest.groovy | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy b/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy index d4661740b9..b40bfd8712 100644 --- a/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy +++ b/src/test/groovy/graphql/incremental/IncrementalExecutionResultTest.groovy @@ -121,10 +121,15 @@ class IncrementalExecutionResultTest extends Specification { def "transform returns IncrementalExecutionResult"() { when: - def initial = newIncrementalExecutionResult().build() + def initial = newIncrementalExecutionResult().hasNext(true).build() then: - def transformed = initial.transform { } + def transformed = initial.transform { b -> + b.addExtension("ext-key", "ext-value") + b.hasNext(false) + } transformed instanceof IncrementalExecutionResult + transformed.extensions == ["ext-key": "ext-value"] + transformed.hasNext == false } } From c56d632e9bcd5c835b2611ad477d215a8b7c4128 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 26 Aug 2024 16:28:44 +0000 Subject: [PATCH 17/21] Bump google-github-actions/auth from 2.1.4 to 2.1.5 Bumps [google-github-actions/auth](https://github.com/google-github-actions/auth) from 2.1.4 to 2.1.5. - [Release notes](https://github.com/google-github-actions/auth/releases) - [Changelog](https://github.com/google-github-actions/auth/blob/main/CHANGELOG.md) - [Commits](https://github.com/google-github-actions/auth/compare/v2.1.4...v2.1.5) --- updated-dependencies: - dependency-name: google-github-actions/auth dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- .github/workflows/invoke_test_runner.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/invoke_test_runner.yml b/.github/workflows/invoke_test_runner.yml index 9a3ec7e102..09f5cdb243 100644 --- a/.github/workflows/invoke_test_runner.yml +++ b/.github/workflows/invoke_test_runner.yml @@ -50,7 +50,7 @@ jobs: - id: 'auth' name: 'Authenticate to Google Cloud' - uses: google-github-actions/auth@v2.1.4 + uses: google-github-actions/auth@v2.1.5 with: credentials_json: ${{ secrets.GOOGLE_APPLICATION_CREDENTIALS }} From 3fc8444ad2881588025a15678a57f87186e129e1 Mon Sep 17 00:00:00 2001 From: Felipe Reis Date: Tue, 27 Aug 2024 15:29:44 +1000 Subject: [PATCH 18/21] Small adjustments after pairing with Andi --- ...ispatchStrategyWithDeferAlwaysDispatch.java | 18 +++++++++--------- .../DataLoaderPerformanceData.groovy | 10 +++++++++- .../dataloader/DeferWithDataLoaderTest.groovy | 3 --- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java index 7563cf71aa..e81ef3948c 100644 --- a/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java +++ b/src/main/java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java @@ -31,11 +31,11 @@ public class PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch implement private final ExecutionContext executionContext; /** - * This flag is used to determine if we are in a deferred context or not. + * This flag is used to determine if we have started the deferred execution. * The value of this flag is set to true as soon as we identified that a deferred field is being executed, and then * the flag stays on that state for the remainder of the execution. */ - private final AtomicBoolean enteredDeferredContext = new AtomicBoolean(false); + private final AtomicBoolean startedDeferredExecution = new AtomicBoolean(false); private static class CallStack { @@ -115,12 +115,12 @@ public PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch(ExecutionContex @Override public void executeDeferredOnFieldValueInfo(FieldValueInfo fieldValueInfo, ExecutionStrategyParameters executionStrategyParameters) { - this.enteredDeferredContext.set(true); + this.startedDeferredExecution.set(true); } @Override public void executionStrategy(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { - if (this.enteredDeferredContext.get()) { + if (this.startedDeferredExecution.get()) { return; } int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; @@ -129,7 +129,7 @@ public void executionStrategy(ExecutionContext executionContext, ExecutionStrate @Override public void executeObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { - if (this.enteredDeferredContext.get()) { + if (this.startedDeferredExecution.get()) { return; } int curLevel = parameters.getExecutionStepInfo().getPath().getLevel() + 1; @@ -138,7 +138,7 @@ public void executeObject(ExecutionContext executionContext, ExecutionStrategyPa @Override public void executionStrategyOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { - if (this.enteredDeferredContext.get()) { + if (this.startedDeferredExecution.get()) { this.dispatch(); } int curLevel = parameters.getPath().getLevel() + 1; @@ -155,7 +155,7 @@ public void executionStrategyOnFieldValuesException(Throwable t, ExecutionStrate @Override public void executeObjectOnFieldValuesInfo(List fieldValueInfoList, ExecutionStrategyParameters parameters) { - if (this.enteredDeferredContext.get()) { + if (this.startedDeferredExecution.get()) { this.dispatch(); } int curLevel = parameters.getPath().getLevel() + 1; @@ -179,8 +179,8 @@ public void fieldFetched(ExecutionContext executionContext, final boolean dispatchNeeded; - if (parameters.getField().isDeferred() || this.enteredDeferredContext.get()) { - this.enteredDeferredContext.compareAndSet(false, true); + if (parameters.getField().isDeferred() || this.startedDeferredExecution.get()) { + this.startedDeferredExecution.set(true); dispatchNeeded = true; } else { int level = parameters.getPath().getLevel(); diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy index 2df1aba3ef..7366ded562 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderPerformanceData.groovy @@ -249,7 +249,14 @@ class DataLoaderPerformanceData { .collect { it.toSpecification() } } - + /** + * Combines the initial result with the incremental results into a single result that has the same shape as a + * "normal" execution result. + * + * @param initialResult the data from the initial execution + * @param incrementalResults the data from the incremental executions + * @return a single result that combines the initial and incremental results + */ static Map combineExecutionResults(Map initialResult, List> incrementalResults) { Map combinedResult = deepClone(initialResult, Map.class) @@ -275,6 +282,7 @@ class DataLoaderPerformanceData { } } + // Remove the "hasNext" to make the result look like a normal execution result combinedResult.remove("hasNext") combinedResult diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy index 5268f4d094..6da9489c76 100644 --- a/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/dataloader/DeferWithDataLoaderTest.groovy @@ -190,9 +190,6 @@ class DeferWithDataLoaderTest extends Specification { ExecutionResult result = graphQL.execute(executionInput) - println "dept for shops dl: " + batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() - println "prod for depts dl: " + batchCompareDataFetchers.productsForDepartmentsBatchLoaderCounter.get() - then: result.toSpecification() == expectedInitialData From 14ed86abb5c8132b21be2a91b4fd05bcebb6ed95 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 2 Sep 2024 16:40:10 +0000 Subject: [PATCH 19/21] Bump net.bytebuddy:byte-buddy from 1.14.18 to 1.15.1 Bumps [net.bytebuddy:byte-buddy](https://github.com/raphw/byte-buddy) from 1.14.18 to 1.15.1. - [Release notes](https://github.com/raphw/byte-buddy/releases) - [Changelog](https://github.com/raphw/byte-buddy/blob/master/release-notes.md) - [Commits](https://github.com/raphw/byte-buddy/compare/byte-buddy-1.14.18...byte-buddy-1.15.1) --- updated-dependencies: - dependency-name: net.bytebuddy:byte-buddy dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- agent/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/build.gradle b/agent/build.gradle index 1dfeba3f27..7d220cfcdd 100644 --- a/agent/build.gradle +++ b/agent/build.gradle @@ -6,7 +6,7 @@ plugins { } dependencies { - implementation("net.bytebuddy:byte-buddy:1.14.18") + implementation("net.bytebuddy:byte-buddy:1.15.1") // graphql-java itself implementation(rootProject) } From c444f031358e34ce8547b391d6bb01a812752088 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 2 Sep 2024 16:40:19 +0000 Subject: [PATCH 20/21] Bump net.bytebuddy:byte-buddy-agent from 1.14.18 to 1.15.1 Bumps [net.bytebuddy:byte-buddy-agent](https://github.com/raphw/byte-buddy) from 1.14.18 to 1.15.1. - [Release notes](https://github.com/raphw/byte-buddy/releases) - [Changelog](https://github.com/raphw/byte-buddy/blob/master/release-notes.md) - [Commits](https://github.com/raphw/byte-buddy/compare/byte-buddy-1.14.18...byte-buddy-1.15.1) --- updated-dependencies: - dependency-name: net.bytebuddy:byte-buddy-agent dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- agent-test/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent-test/build.gradle b/agent-test/build.gradle index d447dc42fe..bc70a660b8 100644 --- a/agent-test/build.gradle +++ b/agent-test/build.gradle @@ -4,7 +4,7 @@ plugins { dependencies { implementation(rootProject) - implementation("net.bytebuddy:byte-buddy-agent:1.14.18") + implementation("net.bytebuddy:byte-buddy-agent:1.15.1") testImplementation 'org.junit.jupiter:junit-jupiter:5.10.3' testRuntimeOnly 'org.junit.platform:junit-platform-launcher' From 159285c76133fffa826d2f2f5f08db253374bd43 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 3 Sep 2024 03:19:40 +0000 Subject: [PATCH 21/21] Bump org.junit.jupiter:junit-jupiter from 5.10.3 to 5.11.0 Bumps [org.junit.jupiter:junit-jupiter](https://github.com/junit-team/junit5) from 5.10.3 to 5.11.0. - [Release notes](https://github.com/junit-team/junit5/releases) - [Commits](https://github.com/junit-team/junit5/compare/r5.10.3...r5.11.0) --- updated-dependencies: - dependency-name: org.junit.jupiter:junit-jupiter dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- agent-test/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent-test/build.gradle b/agent-test/build.gradle index bc70a660b8..3a5911b824 100644 --- a/agent-test/build.gradle +++ b/agent-test/build.gradle @@ -6,7 +6,7 @@ dependencies { implementation(rootProject) implementation("net.bytebuddy:byte-buddy-agent:1.15.1") - testImplementation 'org.junit.jupiter:junit-jupiter:5.10.3' + testImplementation 'org.junit.jupiter:junit-jupiter:5.11.0' testRuntimeOnly 'org.junit.platform:junit-platform-launcher' testImplementation("org.assertj:assertj-core:3.26.3")