Skip to content

Commit 62bcf34

Browse files
samuelAndalonSamuel Vazquez
samuelAndalon
and
Samuel Vazquez
authored
feat(batching): check if execution was exhausted when there are errors (#2009)
### 📝 Description Cover the scenario where the sync execution is exhausted after an execution exception, as the execution that thrown the exception could be the last one to execute synchronous code. --------- Co-authored-by: Samuel Vazquez <[email protected]>
1 parent cf1c9d6 commit 62bcf34

File tree

6 files changed

+106
-51
lines changed

6 files changed

+106
-51
lines changed

‎executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentation : AbstractSyncExecutionExh
4040
parameters: SyncExecutionExhaustedInstrumentationParameters
4141
): OnSyncExecutionExhaustedCallback = { _: List<ExecutionId> ->
4242
parameters
43-
.executionContext.executionInput
43+
.executionInput
4444
.dataLoaderRegistry
4545
.dispatchAll()
4646
}

‎executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/AbstractSyncExecutionExhaustedInstrumentation.kt

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,12 @@ abstract class AbstractSyncExecutionExhaustedInstrumentation : SimplePerformantI
6161
): InstrumentationContext<ExecutionResult>? =
6262
parameters.graphQLContext
6363
?.get<SyncExecutionExhaustedState>(SyncExecutionExhaustedState::class)
64-
?.beginExecution(parameters)
64+
?.beginExecution(
65+
parameters,
66+
this.getOnSyncExecutionExhaustedCallback(
67+
SyncExecutionExhaustedInstrumentationParameters(parameters.executionInput)
68+
)
69+
)
6570

6671
override fun beginExecutionStrategy(
6772
parameters: InstrumentationExecutionStrategyParameters,
@@ -92,7 +97,7 @@ abstract class AbstractSyncExecutionExhaustedInstrumentation : SimplePerformantI
9297
?.beginFieldFetching(
9398
parameters,
9499
this.getOnSyncExecutionExhaustedCallback(
95-
SyncExecutionExhaustedInstrumentationParameters(parameters.executionContext)
100+
SyncExecutionExhaustedInstrumentationParameters(parameters.executionContext.executionInput)
96101
)
97102
)
98103
}

‎executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/execution/SyncExecutionExhaustedInstrumentationParameters.kt

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2022 Expedia, Inc
2+
* Copyright 2024 Expedia, Inc
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -17,11 +17,11 @@
1717
package com.expediagroup.graphql.dataloader.instrumentation.syncexhaustion.execution
1818

1919
import com.expediagroup.graphql.dataloader.instrumentation.syncexhaustion.DataLoaderSyncExecutionExhaustedInstrumentation
20-
import graphql.execution.ExecutionContext
20+
import graphql.ExecutionInput
2121

2222
/**
2323
* Hold information that will be provided to an instance of [DataLoaderSyncExecutionExhaustedInstrumentation]
2424
*/
2525
data class SyncExecutionExhaustedInstrumentationParameters(
26-
val executionContext: ExecutionContext
26+
val executionInput: ExecutionInput
2727
)

‎executions/graphql-kotlin-dataloader-instrumentation/src/main/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/state/SyncExecutionExhaustedState.kt

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -47,36 +47,37 @@ class SyncExecutionExhaustedState(
4747
private val totalExecutions: AtomicReference<Int> = AtomicReference(totalOperations)
4848
val executions = ConcurrentHashMap<ExecutionId, ExecutionBatchState>()
4949

50-
/**
51-
* Remove an [ExecutionBatchState] from the state in case operation does not qualify for starting an execution,
52-
* for example:
53-
* - parsing, validation errors
54-
* - persisted query errors
55-
* - an exception during execution was thrown
56-
*/
57-
private fun removeExecution(executionId: ExecutionId) {
58-
if (executions.containsKey(executionId)) {
59-
executions.remove(executionId)
60-
totalExecutions.set(totalExecutions.get() - 1)
61-
}
62-
}
63-
6450
/**
6551
* Create the [ExecutionBatchState] When a specific [ExecutionInput] starts his execution
6652
*
6753
* @param parameters contains information of which [ExecutionInput] will start his execution
6854
* @return a non null [InstrumentationContext] object
6955
*/
7056
fun beginExecution(
71-
parameters: InstrumentationExecutionParameters
57+
parameters: InstrumentationExecutionParameters,
58+
onSyncExecutionExhausted: OnSyncExecutionExhaustedCallback
7259
): InstrumentationContext<ExecutionResult> {
7360
executions.computeIfAbsent(parameters.executionInput.executionId) {
7461
ExecutionBatchState()
7562
}
7663
return object : SimpleInstrumentationContext<ExecutionResult>() {
64+
/**
65+
* Remove an [ExecutionBatchState] from the state in case operation does not qualify for starting or completing execution,
66+
* for example:
67+
* - parsing, validation errors
68+
* - persisted query errors
69+
* - an exception during execution was thrown
70+
*/
7771
override fun onCompleted(result: ExecutionResult?, t: Throwable?) {
7872
if ((result != null && result.errors.size > 0) || t != null) {
79-
removeExecution(parameters.executionInput.executionId)
73+
if (executions.containsKey(parameters.executionInput.executionId)) {
74+
executions.remove(parameters.executionInput.executionId)
75+
totalExecutions.set(totalExecutions.get() - 1)
76+
val allSyncExecutionsExhausted = allSyncExecutionsExhausted()
77+
if (allSyncExecutionsExhausted) {
78+
onSyncExecutionExhausted(executions.keys().toList())
79+
}
80+
}
8081
}
8182
}
8283
}

‎executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/fixture/AstronautGraphQL.kt

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -193,11 +193,22 @@ object AstronautGraphQL {
193193
)
194194
)
195195

196-
fun execute(
196+
fun executeOperations(
197197
graphQL: GraphQL,
198198
queries: List<String>,
199199
dataLoaderInstrumentationStrategy: DataLoaderInstrumentationStrategy
200-
): Pair<List<ExecutionResult>, KotlinDataLoaderRegistry> {
200+
): Pair<List<Result<ExecutionResult>>, KotlinDataLoaderRegistry> =
201+
execute(
202+
graphQL,
203+
queries.map { query -> ExecutionInput.newExecutionInput(query).build() },
204+
dataLoaderInstrumentationStrategy
205+
)
206+
207+
fun execute(
208+
graphQL: GraphQL,
209+
executionInputs: List<ExecutionInput>,
210+
dataLoaderInstrumentationStrategy: DataLoaderInstrumentationStrategy
211+
): Pair<List<Result<ExecutionResult>>, KotlinDataLoaderRegistry> {
201212
val kotlinDataLoaderRegistry = spyk(
202213
KotlinDataLoaderRegistryFactory(
203214
AstronautDataLoader(),
@@ -210,26 +221,32 @@ object AstronautGraphQL {
210221
when (dataLoaderInstrumentationStrategy) {
211222
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION ->
212223
SyncExecutionExhaustedState::class to SyncExecutionExhaustedState(
213-
queries.size,
224+
executionInputs.size,
214225
kotlinDataLoaderRegistry
215226
)
216227
}
217228
)
218229

219230
val results = runBlocking {
220-
queries.map { query ->
231+
executionInputs.map { executionInput ->
221232
async {
222-
graphQL.executeAsync(
223-
ExecutionInput
224-
.newExecutionInput(query)
225-
.dataLoaderRegistry(kotlinDataLoaderRegistry)
226-
.graphQLContext(graphQLContext)
227-
.build()
228-
).await()
233+
try {
234+
Result.success(
235+
graphQL.executeAsync(
236+
executionInput.transform { builder ->
237+
builder
238+
.dataLoaderRegistry(kotlinDataLoaderRegistry)
239+
.graphQLContext(graphQLContext)
240+
.build()
241+
}
242+
).await()
243+
)
244+
} catch (e: Exception) {
245+
Result.failure(e)
246+
}
229247
}
230248
}.awaitAll()
231249
}
232-
233250
return Pair(results, kotlinDataLoaderRegistry)
234251
}
235252
}

‎executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentationTest.kt

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package com.expediagroup.graphql.dataloader.instrumentation.syncexhaustion
1919
import com.expediagroup.graphql.dataloader.instrumentation.fixture.DataLoaderInstrumentationStrategy
2020
import com.expediagroup.graphql.dataloader.instrumentation.fixture.AstronautGraphQL
2121
import com.expediagroup.graphql.dataloader.instrumentation.fixture.ProductGraphQL
22+
import graphql.ExecutionInput
2223
import io.mockk.clearAllMocks
2324
import io.mockk.verify
2425
import org.junit.jupiter.api.BeforeEach
@@ -54,7 +55,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
5455
"{ mission(id: 4) { designation } }"
5556
)
5657

57-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
58+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
5859
graphQL,
5960
queries,
6061
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -85,7 +86,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
8586
"{ nasa { mission(id: 4) { id designation } } }"
8687
)
8788

88-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
89+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
8990
graphQL,
9091
queries,
9192
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -120,7 +121,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
120121
"{ mission(id: 4) { designation } }"
121122
)
122123

123-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
124+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
124125
graphQL,
125126
queries,
126127
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -164,7 +165,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
164165
""".trimIndent()
165166
)
166167

167-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
168+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
168169
graphQL,
169170
queries,
170171
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -202,7 +203,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
202203
""".trimIndent()
203204
)
204205

205-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
206+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
206207
graphQL,
207208
queries,
208209
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -253,7 +254,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
253254
""".trimIndent()
254255
)
255256

256-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
257+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
257258
graphQL,
258259
queries,
259260
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -299,7 +300,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
299300
""".trimIndent()
300301
)
301302

302-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
303+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
303304
graphQL,
304305
queries,
305306
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -340,7 +341,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
340341
""".trimIndent()
341342
)
342343

343-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
344+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
344345
graphQL,
345346
queries,
346347
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -380,7 +381,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
380381
""".trimIndent()
381382
)
382383

383-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
384+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
384385
graphQL,
385386
queries,
386387
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -389,7 +390,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
389390
assertEquals(3, results.size)
390391

391392
results.forEach { result ->
392-
assertTrue(result.errors.isEmpty())
393+
assertTrue(result.getOrThrow().errors.isEmpty())
393394
}
394395

395396
val missionStatistics = kotlinDataLoaderRegistry.dataLoadersMap["MissionDataLoader"]?.statistics
@@ -422,15 +423,15 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
422423
""".trimIndent()
423424
)
424425

425-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
426+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
426427
graphQL,
427428
queries,
428429
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
429430
)
430431

431432
assertEquals(3, results.size)
432433
results.forEach { result ->
433-
assertTrue(result.errors.isEmpty())
434+
assertTrue(result.getOrThrow().errors.isEmpty())
434435
}
435436

436437
val astronautStatistics = kotlinDataLoaderRegistry.dataLoadersMap["AstronautDataLoader"]?.statistics
@@ -470,7 +471,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
470471
""".trimIndent(),
471472
)
472473

473-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
474+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
474475
graphQL,
475476
queries,
476477
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -482,7 +483,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
482483

483484
assertEquals(2, results.size)
484485
results.forEach { result ->
485-
assertTrue(result.errors.isEmpty())
486+
assertTrue(result.getOrThrow().errors.isEmpty())
486487
}
487488

488489
assertEquals(1, astronautStatistics?.batchInvokeCount)
@@ -566,7 +567,7 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
566567
"""mutation { createAstronaut(name: "spaceMan") { id name } }"""
567568
)
568569

569-
val (results, dataLoaderSyncExecutionExhaustedInstrumentation) = AstronautGraphQL.execute(
570+
val (results, dataLoaderSyncExecutionExhaustedInstrumentation) = AstronautGraphQL.executeOperations(
570571
graphQL,
571572
queries,
572573
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -579,15 +580,15 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
579580
}
580581

581582
@Test
582-
fun `Instrumentation should not account for invalid operations`() {
583+
fun `Instrumentation should not consider executions with invalid operations`() {
583584
val queries = listOf(
584585
"invalid query{ astronaut(id: 1) {",
585586
"{ astronaut(id: 2) { id name } }",
586587
"{ mission(id: 3) { id designation } }",
587588
"{ mission(id: 4) { designation } }"
588589
)
589590

590-
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
591+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.executeOperations(
591592
graphQL,
592593
queries,
593594
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
@@ -608,4 +609,35 @@ class DataLoaderSyncExecutionExhaustedInstrumentationTest {
608609
kotlinDataLoaderRegistry.dispatchAll()
609610
}
610611
}
612+
613+
@Test
614+
fun `Instrumentation should not consider executions that thrown exceptions`() {
615+
val executions = listOf(
616+
ExecutionInput.newExecutionInput("query test1 { astronaut(id: 1) { id name } }").operationName("test1").build(),
617+
ExecutionInput.newExecutionInput("query test2 { astronaut(id: 2) { id name } }").operationName("test2").build(),
618+
ExecutionInput.newExecutionInput("query test3 { mission(id: 3) { id designation } }").operationName("test3").build(),
619+
ExecutionInput.newExecutionInput("query test4 { mission(id: 4) { designation } }").operationName("OPERATION_NOT_IN_DOCUMENT").build()
620+
)
621+
622+
val (results, kotlinDataLoaderRegistry) = AstronautGraphQL.execute(
623+
graphQL,
624+
executions,
625+
DataLoaderInstrumentationStrategy.SYNC_EXHAUSTION
626+
)
627+
628+
assertEquals(4, results.size)
629+
630+
val astronautStatistics = kotlinDataLoaderRegistry.dataLoadersMap["AstronautDataLoader"]?.statistics
631+
val missionStatistics = kotlinDataLoaderRegistry.dataLoadersMap["MissionDataLoader"]?.statistics
632+
633+
assertEquals(1, astronautStatistics?.batchInvokeCount)
634+
assertEquals(2, astronautStatistics?.batchLoadCount)
635+
636+
assertEquals(1, missionStatistics?.batchInvokeCount)
637+
assertEquals(1, missionStatistics?.batchLoadCount)
638+
639+
verify(exactly = 2) {
640+
kotlinDataLoaderRegistry.dispatchAll()
641+
}
642+
}
611643
}

0 commit comments

Comments
 (0)