From 9eabddeadef7bc32e062d1ef2e27774f398159c9 Mon Sep 17 00:00:00 2001 From: Samuel Vazquez Date: Thu, 11 Jul 2024 14:58:53 -0700 Subject: [PATCH] feat(batching): v6 check if execution was exhausted when there are errors (#2011) ### :pencil: Description https://github.com/ExpediaGroup/graphql-kotlin/pull/2009 Co-authored-by: Samuel Vazquez --- ...erSyncExecutionExhaustedInstrumentation.kt | 2 +- ...ctSyncExecutionExhaustedInstrumentation.kt | 9 ++- ...utionExhaustedInstrumentationParameters.kt | 6 +- .../state/SyncExecutionExhaustedState.kt | 19 +++++- .../fixture/AstronautGraphQL.kt | 44 +++++++++---- ...oaderLevelDispatchedInstrumentationTest.kt | 10 +-- ...ncExecutionExhaustedInstrumentationTest.kt | 66 ++++++++++++++----- 7 files changed, 113 insertions(+), 43 deletions(-) diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentation.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentation.kt index 209b61bcbb..358c7abed7 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentation.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentation.kt @@ -40,7 +40,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentation : AbstractSyncExecutionExh parameters: SyncExecutionExhaustedInstrumentationParameters ): OnSyncExecutionExhaustedCallback = { _: List -> parameters - .executionContext.executionInput + .executionInput .dataLoaderRegistry .dispatchAll() } diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/AbstractSyncExecutionExhaustedInstrumentation.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/AbstractSyncExecutionExhaustedInstrumentation.kt index 8d06df8f85..b6e07f401d 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/AbstractSyncExecutionExhaustedInstrumentation.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/AbstractSyncExecutionExhaustedInstrumentation.kt @@ -59,7 +59,12 @@ abstract class AbstractSyncExecutionExhaustedInstrumentation : SimplePerformantI ): InstrumentationContext? = parameters.graphQLContext ?.get(SyncExecutionExhaustedState::class) - ?.beginExecution(parameters) + ?.beginExecution( + parameters, + this.getOnSyncExecutionExhaustedCallback( + SyncExecutionExhaustedInstrumentationParameters(parameters.executionInput) + ) + ) override fun beginExecutionStrategy( parameters: InstrumentationExecutionStrategyParameters, @@ -78,7 +83,7 @@ abstract class AbstractSyncExecutionExhaustedInstrumentation : SimplePerformantI ?.beginFieldFetch( parameters, this.getOnSyncExecutionExhaustedCallback( - SyncExecutionExhaustedInstrumentationParameters(parameters.executionContext) + SyncExecutionExhaustedInstrumentationParameters(parameters.executionContext.executionInput) ) ) } diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/SyncExecutionExhaustedInstrumentationParameters.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/SyncExecutionExhaustedInstrumentationParameters.kt index 1e955de7ab..38d8f8c58d 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/SyncExecutionExhaustedInstrumentationParameters.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/SyncExecutionExhaustedInstrumentationParameters.kt @@ -1,5 +1,5 @@ /* - * Copyright 2022 Expedia, Inc + * Copyright 2024 Expedia, Inc * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,11 +17,11 @@ package com.expediagroup.graphql.dataloader.instrumentation.syncexhaustion.execution import com.expediagroup.graphql.dataloader.instrumentation.syncexhaustion.DataLoaderSyncExecutionExhaustedInstrumentation -import graphql.execution.ExecutionContext +import graphql.ExecutionInput /** * Hold information that will be provided to an instance of [DataLoaderSyncExecutionExhaustedInstrumentation] */ data class SyncExecutionExhaustedInstrumentationParameters( - val executionContext: ExecutionContext + val executionInput: ExecutionInput ) diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/state/SyncExecutionExhaustedState.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/state/SyncExecutionExhaustedState.kt index c8cb8eb599..b5fbc4bd2b 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/state/SyncExecutionExhaustedState.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/state/SyncExecutionExhaustedState.kt @@ -67,15 +67,30 @@ class SyncExecutionExhaustedState( * @return a non null [InstrumentationContext] object */ fun beginExecution( - parameters: InstrumentationExecutionParameters + parameters: InstrumentationExecutionParameters, + onSyncExecutionExhausted: OnSyncExecutionExhaustedCallback ): InstrumentationContext { executions.computeIfAbsent(parameters.executionInput.executionId) { ExecutionBatchState() } return object : SimpleInstrumentationContext() { + /** + * Remove an [ExecutionBatchState] from the state in case operation does not qualify for starting an execution, + * for example: + * - parsing, validation errors + * - persisted query errors + * - an exception during execution was thrown + */ override fun onCompleted(result: ExecutionResult?, t: Throwable?) { if ((result != null && result.errors.size > 0) || t != null) { - removeExecution(parameters.executionInput.executionId) + if (executions.containsKey(parameters.executionInput.executionId)) { + executions.remove(parameters.executionInput.executionId) + totalExecutions.set(totalExecutions.get() - 1) + val allSyncExecutionsExhausted = allSyncExecutionsExhausted() + if (allSyncExecutionsExhausted) { + onSyncExecutionExhausted(executions.keys().toList()) + } + } } } } diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/fixture/AstronautGraphQL.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/fixture/AstronautGraphQL.kt index 2663e5160a..50866b0925 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/fixture/AstronautGraphQL.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/fixture/AstronautGraphQL.kt @@ -1,5 +1,5 @@ /* - * Copyright 2022 Expedia, Inc + * Copyright 2024 Expedia, Inc * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -197,11 +197,22 @@ object AstronautGraphQL { ) ) - fun execute( + fun executeOperations( graphQL: GraphQL, queries: List, dataLoaderInstrumentationStrategy: DataLoaderInstrumentationStrategy - ): Pair, KotlinDataLoaderRegistry> { + ): Pair>, KotlinDataLoaderRegistry> = + execute( + graphQL, + queries.map { query -> ExecutionInput.newExecutionInput(query).build() }, + dataLoaderInstrumentationStrategy + ) + + fun execute( + graphQL: GraphQL, + executionInputs: List, + dataLoaderInstrumentationStrategy: DataLoaderInstrumentationStrategy + ): Pair>, KotlinDataLoaderRegistry> { val kotlinDataLoaderRegistry = spyk( KotlinDataLoaderRegistryFactory( AstronautDataLoader(), @@ -214,26 +225,33 @@ object AstronautGraphQL { when (dataLoaderInstrumentationStrategy) { DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION -> SyncExecutionExhaustedState::class to SyncExecutionExhaustedState( - queries.size, + executionInputs.size, kotlinDataLoaderRegistry ) DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED -> ExecutionLevelDispatchedState::class to ExecutionLevelDispatchedState( - queries.size + executionInputs.size ) } ) val results = runBlocking { - queries.map { query -> + executionInputs.map { executionInput -> async { - graphQL.executeAsync( - ExecutionInput - .newExecutionInput(query) - .dataLoaderRegistry(kotlinDataLoaderRegistry) - .graphQLContext(graphQLContext) - .build() - ).await() + try { + Result.success( + graphQL.executeAsync( + executionInput.transform { builder -> + builder + .dataLoaderRegistry(kotlinDataLoaderRegistry) + .graphQLContext(graphQLContext) + .build() + } + ).await() + ) + } catch (e: Exception) { + Result.failure(e) + } } }.awaitAll() } diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/level/DataLoaderLevelDispatchedInstrumentationTest.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/level/DataLoaderLevelDispatchedInstrumentationTest.kt index 728e81d540..943b0b5899 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/level/DataLoaderLevelDispatchedInstrumentationTest.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/level/DataLoaderLevelDispatchedInstrumentationTest.kt @@ -48,7 +48,7 @@ class DataLoaderLevelDispatchedInstrumentationTest { "{ mission(id: 4) { designation } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED @@ -79,7 +79,7 @@ class DataLoaderLevelDispatchedInstrumentationTest { "{ nasa { mission(id: 4) { id designation } } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED @@ -114,7 +114,7 @@ class DataLoaderLevelDispatchedInstrumentationTest { "{ mission(id: 4) { designation } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED @@ -149,7 +149,7 @@ class DataLoaderLevelDispatchedInstrumentationTest { """mutation { createAstronaut(name: "spaceMan") { id name } }""" ) - val (results, _) = AstronautGraphQL.execute( + val (results, _) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED @@ -170,7 +170,7 @@ class DataLoaderLevelDispatchedInstrumentationTest { "{ mission(id: 4) { designation } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED diff --git a/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentationTest.kt b/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentationTest.kt index f4388a275a..5f5a8b74fd 100644 --- a/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentationTest.kt +++ b/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentationTest.kt @@ -19,6 +19,7 @@ package com.expediagroup.graphql.dataloader.instrumentation.syncexhaustion import com.expediagroup.graphql.dataloader.instrumentation.fixture.DataLoaderInstrumentationStrategy import com.expediagroup.graphql.dataloader.instrumentation.fixture.AstronautGraphQL import com.expediagroup.graphql.dataloader.instrumentation.fixture.ProductGraphQL +import graphql.ExecutionInput import io.mockk.Called import io.mockk.clearAllMocks import io.mockk.spyk @@ -56,7 +57,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { "{ mission(id: 4) { designation } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -87,7 +88,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { "{ nasa { mission(id: 4) { id designation } } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -122,7 +123,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { "{ mission(id: 4) { designation } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -166,7 +167,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -204,7 +205,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -255,7 +256,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -301,7 +302,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -342,7 +343,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -382,7 +383,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -391,7 +392,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { assertEquals(3, results.size) results.forEach { result -> - assertTrue(result.errors.isEmpty()) + assertTrue(result.getOrThrow().errors.isEmpty()) } val missionStatistics = kotlinDataLoaderRegistry.dataLoadersMap["MissionDataLoader"]?.statistics @@ -424,7 +425,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent() ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -432,7 +433,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { assertEquals(3, results.size) results.forEach { result -> - assertTrue(result.errors.isEmpty()) + assertTrue(result.getOrThrow().errors.isEmpty()) } val astronautStatistics = kotlinDataLoaderRegistry.dataLoadersMap["AstronautDataLoader"]?.statistics @@ -472,7 +473,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """.trimIndent(), ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -484,7 +485,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { assertEquals(2, results.size) results.forEach { result -> - assertTrue(result.errors.isEmpty()) + assertTrue(result.getOrThrow().errors.isEmpty()) } assertEquals(1, astronautStatistics?.batchInvokeCount) @@ -568,7 +569,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { """mutation { createAstronaut(name: "spaceMan") { id name } }""" ) - val (results, _) = AstronautGraphQL.execute( + val (results, _) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.LEVEL_DISPATCHED @@ -581,7 +582,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { } @Test - fun `Instrumentation should not account for invalid operations`() { + fun `Instrumentation should not consider executions with invalid operations`() { val queries = listOf( "invalid query{ astronaut(id: 1) {", "{ astronaut(id: 2) { id name } }", @@ -589,7 +590,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { "{ mission(id: 4) { designation } }" ) - val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations( graphQL, queries, DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION @@ -610,4 +611,35 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest { kotlinDataLoaderRegistry.dispatchAll() } } + + @Test + fun `Instrumentation should not consider executions that thrown exceptions`() { + val executions = listOf( + ExecutionInput.newExecutionInput("query test1 { astronaut(id: 1) { id name } }").operationName("test1").build(), + ExecutionInput.newExecutionInput("query test2 { astronaut(id: 2) { id name } }").operationName("test2").build(), + ExecutionInput.newExecutionInput("query test3 { mission(id: 3) { id designation } }").operationName("test3").build(), + ExecutionInput.newExecutionInput("query test4 { mission(id: 4) { designation } }").operationName("OPERATION_NOT_IN_DOCUMENT").build() + ) + + val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute( + graphQL, + executions, + DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION + ) + + assertEquals(4, results.size) + + val astronautStatistics = kotlinDataLoaderRegistry.dataLoadersMap["AstronautDataLoader"]?.statistics + val missionStatistics = kotlinDataLoaderRegistry.dataLoadersMap["MissionDataLoader"]?.statistics + + assertEquals(1, astronautStatistics?.batchInvokeCount) + assertEquals(2, astronautStatistics?.batchLoadCount) + + assertEquals(1, missionStatistics?.batchInvokeCount) + assertEquals(1, missionStatistics?.batchLoadCount) + + verify(exactly = 2) { + kotlinDataLoaderRegistry.dispatchAll() + } + } }