From b6f553ffa679ca3a3ee2fd1a8356e4991d082707 Mon Sep 17 00:00:00 2001 From: Franklin Wang Date: Thu, 21 Dec 2023 10:07:56 +1300 Subject: [PATCH] Refactor source input to be an object --- .../NadelNewBatchHydrationInputBuilder.kt | 18 +- .../hydration/batch/NadelNewBatchHydrator.kt | 197 ++++++++++-------- .../nadel/hooks/NadelExecutionHooks.kt | 2 +- ...hydration-instruction-hook-returns-null.kt | 4 +- ...-conditional-hydration-in-abstract-type.kt | 4 +- ...-source-ids-going-to-different-services.kt | 4 +- ...ation-instructions-use-different-inputs.kt | 2 +- 7 files changed, 129 insertions(+), 102 deletions(-) diff --git a/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrationInputBuilder.kt b/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrationInputBuilder.kt index ec9ae17ea..007dffc8b 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrationInputBuilder.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrationInputBuilder.kt @@ -19,19 +19,19 @@ import graphql.schema.GraphQLTypeUtil * [NadelBatchHydrationFieldInstruction.batchSize] was exceeded etc. */ internal data class NadelHydrationArgumentsBatch( - val sourceIds: List, + val sourceInputs: List, val arguments: Map, ) /** * An [NormalizedInputValue] for one query to a service. * - * i.e. this object represents one batch of the [sourceIds] values that we send down. + * i.e. this object represents one batch of the [sourceInputs] values that we send down. * * An intermediary object to store info while we pass data around functions. */ private data class BatchedArgumentValue( - val sourceIds: List, + val sourceInputs: List, val argumentDef: NadelHydrationActorInputDef, val argumentValue: NormalizedInputValue, ) @@ -50,23 +50,23 @@ internal object NadelNewBatchHydrationInputBuilder { userContext: Any?, instruction: NadelBatchHydrationFieldInstruction, hydrationField: ExecutableNormalizedField, - sourceIds: List, + sourceInputs: List, ): List { val nonBatchArgs = getNonBatchInputValues(instruction, hydrationField) - val batchArgs = getBatchArgumentValue(instruction, sourceIds, hooks, userContext) + val batchArgs = getBatchArgumentValue(instruction, sourceInputs, hooks, userContext) return batchArgs .map { batchedArgument -> NadelHydrationArgumentsBatch( arguments = nonBatchArgs + (batchedArgument.argumentDef to batchedArgument.argumentValue), - sourceIds = batchedArgument.sourceIds, + sourceInputs = batchedArgument.sourceInputs, ) } } private fun getBatchArgumentValue( instruction: NadelBatchHydrationFieldInstruction, - sourceIds: List, + sourceInputs: List, hooks: NadelExecutionHooks, userContext: Any?, ): List { @@ -76,7 +76,7 @@ internal object NadelNewBatchHydrationInputBuilder { val actorBatchArgDef = instruction.actorFieldDef.getArgument(batchInputDef.name) val partitionArgumentList = hooks.partitionBatchHydrationArgumentList( - argumentValues = sourceIds.map { it.value }, + argumentValues = sourceInputs.map { it.value }, instruction = instruction, userContext = userContext, ) @@ -92,7 +92,7 @@ internal object NadelNewBatchHydrationInputBuilder { ) BatchedArgumentValue( - sourceIds = chunk + sourceInputs = chunk .map { JsonNode(it) }, diff --git a/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrator.kt b/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrator.kt index ac9066b3f..e76528cfc 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrator.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelNewBatchHydrator.kt @@ -22,8 +22,6 @@ import graphql.nadel.engine.transform.hydration.batch.indexing.NadelBatchHydrati import graphql.nadel.engine.transform.result.NadelResultInstruction import graphql.nadel.engine.transform.result.json.JsonNode import graphql.nadel.engine.transform.result.json.JsonNodeExtractor -import graphql.nadel.engine.util.PairList -import graphql.nadel.engine.util.filterPairSecondNotNull import graphql.nadel.engine.util.flatten import graphql.nadel.engine.util.getField import graphql.nadel.engine.util.isList @@ -43,13 +41,13 @@ import kotlinx.coroutines.coroutineScope * Some things to consider * * 1. There can be repeated @hydrated instruction, so we need to choose one. - * 2. There can be multiple source IDs to hydrate per result object e.g. one issue may have multiple contributors - * 3. Each source ID can resolve to a different @hydrated instruction + * 2. There can be multiple source inputs to hydrate per result object e.g. one issue may have multiple contributors + * 3. Each source input can resolve to a different @hydrated instruction * * So what do we need * - * 1. Source object -> Source IDs - * 2. Source ID -> @hydrated instruction + * 1. Source object -> Source inputs + * 2. Source input -> @hydrated instruction * 3. Resolved objects need to be indexed * * So let's consider a hydration @@ -87,9 +85,9 @@ import kotlinx.coroutines.coroutineScope * * We need to create * - * 1. Source object -> Source IDs [getSourceObjectsMetadata] - * 2. Source IDs to -> Instruction [getInstructionParingForSourceIds] - * 3. Resolved objects need to be indexed [indexResults] + * 1. Source object -> Source inputs [getSourceObjectsMetadata] + * 2. Source input -> instruction [getSourceInputs] + * 3. Resolved objects need to be indexed [getIndexedResultsByInstruction] * * e.g. of [SourceObjectMetadata] * @@ -100,9 +98,17 @@ import kotlinx.coroutines.coroutineScope * "key": "GQLGW-6" * "linkIds": ["user/2", "comment/4"] * } - * "sourceIds": [ - * {"user/2": "@hydrated(field: userById)"} - * {"comment/4": "@hydrated(field: commentById)"} + * "sourceInputs": [ + * { + * sourceInputNode: "user/2" + * instruction: "@hydrated(field: userById)" + * indexKey: "user/2" + * } + * { + * sourceInputNode: "comment/4" + * instruction: "@hydrated(field: commentById)" + * indexKey: "comment/4" + * } * ] * } * { @@ -110,8 +116,12 @@ import kotlinx.coroutines.coroutineScope * "key": "ZELDA-12" * "linkIds": ["user/128"] * } - * "sourceIds": [ - * {"user/122": "@hydrated(field: userById)"} + * "sourceInputs": [ + * { + * sourceInputNode: "user/122" + * instruction: "@hydrated(field: userById)" + * indexKey: "comment/4" + * } * ] * } * ] @@ -136,9 +146,9 @@ import kotlinx.coroutines.coroutineScope * ```kotlin * for (sourceObjectMetadata in sourceObjectsMetadata) { * val values = sourceObjectMetadata - * .sourceIdsPairedWithInstructions - * .map { (sourceId, instruction) -> - * index[instruction][sourceId] + * .sourceInputs + * .map { sourceInput -> + * index[sourceInput.instruction][sourceInput.indexKey] * } * } * ``` @@ -151,9 +161,23 @@ internal class NadelNewBatchHydrator( */ private data class SourceObjectMetadata( val sourceObject: JsonNode, - val sourceIdsPairedWithInstructions: PairList?, + val sourceInputs: List?, ) + private sealed class SourceInput { + abstract val sourceInputNode: JsonNode + + data class NotQueryable( + override val sourceInputNode: JsonNode, + ) : SourceInput() + + data class Queryable( + override val sourceInputNode: JsonNode, + val instruction: NadelBatchHydrationFieldInstruction, + val indexKey: NadelBatchHydrationIndexKey, + ) : SourceInput() + } + /** * todo: add validation that repeated directives must use the same $source object unless there is only one input */ @@ -178,16 +202,16 @@ internal class NadelNewBatchHydrator( context(NadelBatchHydratorContext) suspend fun hydrate(sourceObjects: List): List { - // Gets source IDs, instructions info etc. + // Gets source inputs, instructions info etc. val sourceObjectsMetadata = getSourceObjectsMetadata(sourceObjects) - val sourceIdsByInstruction = getSourceIdsByInstruction(sourceObjectsMetadata) + val sourceInputsByInstruction = groupSourceInputsByInstruction(sourceObjectsMetadata) - val resultsByInstruction = sourceIdsByInstruction - .mapValues { (instruction, sourceIds) -> + val resultsByInstruction = sourceInputsByInstruction + .mapValues { (instruction, sourceInputs) -> executeQueries( executionBlueprint = executionBlueprint, instruction = instruction, - sourceIds = sourceIds, + sourceInputs = sourceInputs, ) } @@ -212,13 +236,13 @@ internal class NadelNewBatchHydrator( indexedResultsByInstruction: Map>, ): List { return sourceObjectsMetadata - .map { (sourceObject, sourceIdsPairedWithInstruction) -> + .map { (sourceObject, sourceInputsPairedWithInstruction) -> NadelResultInstruction.Set( subject = sourceObject, field = sourceField, newValue = getHydrationValueForSourceObject( indexedResultsByInstruction, - sourceIdsPairedWithInstruction, + sourceInputsPairedWithInstruction, ), ) } @@ -227,53 +251,48 @@ internal class NadelNewBatchHydrator( context(NadelBatchHydratorContext) private fun getHydrationValueForSourceObject( indexedResultsByInstruction: Map>, - sourceIdsPairedWithInstruction: PairList?, + sourceInputsPairedWithInstruction: List?, ): JsonNode { - fun extractNode( - sourceId: JsonNode, - instruction: NadelBatchHydrationFieldInstruction?, - ): JsonNode { - return if (instruction == null) { - JsonNode.Null - } else { - val key = getIndexer(instruction).getSourceKey(sourceId) - // todo: could this ever be null say if response failed? - indexedResultsByInstruction[instruction]!![key] ?: JsonNode.Null + fun extractNode(sourceInput: SourceInput): JsonNode { + return when (sourceInput) { + is SourceInput.NotQueryable -> JsonNode.Null + is SourceInput.Queryable -> indexedResultsByInstruction[sourceInput.instruction]!![sourceInput.indexKey] + ?: JsonNode.Null } } return if (isIndexHydration) { - if (sourceIdsPairedWithInstruction == null) { + if (sourceInputsPairedWithInstruction == null) { JsonNode.Null } else if (isSourceFieldListOutput) { if (isSourceInputFieldListOutput) { JsonNode( - sourceIdsPairedWithInstruction - .map { (sourceId, instruction) -> - extractNode(sourceId, instruction).value + sourceInputsPairedWithInstruction + .map { sourceInput -> + extractNode(sourceInput).value }, ) } else { - val (sourceId, instruction) = sourceIdsPairedWithInstruction.single() - extractNode(sourceId, instruction) + val sourceInput = sourceInputsPairedWithInstruction.single() + extractNode(sourceInput) } } else { - val (sourceId, instruction) = sourceIdsPairedWithInstruction.single() - extractNode(sourceId, instruction) + val sourceInput = sourceInputsPairedWithInstruction.single() + extractNode(sourceInput) } } else { - if (sourceIdsPairedWithInstruction == null) { + if (sourceInputsPairedWithInstruction == null) { JsonNode.Null } else if (isSourceFieldListOutput) { JsonNode( - sourceIdsPairedWithInstruction - .map { (sourceId, instruction) -> - extractNode(sourceId, instruction).value + sourceInputsPairedWithInstruction + .map { sourceInput -> + extractNode(sourceInput).value }, ) } else { - val (sourceId, instruction) = sourceIdsPairedWithInstruction.single() - extractNode(sourceId, instruction) + val sourceInput = sourceInputsPairedWithInstruction.single() + extractNode(sourceInput) } } } @@ -303,15 +322,18 @@ internal class NadelNewBatchHydrator( private suspend fun executeQueries( executionBlueprint: NadelOverallExecutionBlueprint, instruction: NadelBatchHydrationFieldInstruction, - sourceIds: List, + sourceInputs: List, ): List { - val uniqueSourceIds = sourceIds + val uniqueSourceInputs = sourceInputs .asSequence() // We don't want to query for null values, we always map those to null .filter { - it.value != null + it.sourceInputNode.value != null + } + .map { + it.sourceInputNode } - .toSet() + .toCollection(LinkedHashSet()) .toList() val argBatches = NadelNewBatchHydrationInputBuilder.getInputValueBatches( @@ -319,7 +341,7 @@ internal class NadelNewBatchHydrator( userContext = executionContext.userContext, instruction = instruction, hydrationField = sourceField, - sourceIds = uniqueSourceIds, + sourceInputs = uniqueSourceInputs, ) val queries = NadelHydrationFieldsBuilder @@ -362,7 +384,7 @@ internal class NadelNewBatchHydrator( error("Each argument batch must correspond to one query") } .map { (result, argBatch) -> - NadelResolvedObjectBatch(argBatch.sourceIds, result) + NadelResolvedObjectBatch(argBatch.sourceInputs, result) } .toList() } @@ -384,24 +406,24 @@ internal class NadelNewBatchHydrator( if (instructions.isEmpty()) { null } else { - val sourceIdsPairedWithInstructions = getInstructionParingForSourceIds( + val sourceInputs = getSourceInputs( sourceObject = sourceObject, instructions = instructions, ) SourceObjectMetadata( sourceObject, - sourceIdsPairedWithInstructions, + sourceInputs, ) } } } context(NadelBatchHydratorContext) - private fun getInstructionParingForSourceIds( + private fun getSourceInputs( sourceObject: JsonNode, instructions: List, - ): PairList? { + ): List? { val coords = makeFieldCoordinates( typeName = sourceField.objectTypeNames.first(), fieldName = sourceField.name, @@ -417,15 +439,23 @@ internal class NadelNewBatchHydrator( } .singleOfType() - getSourceInputs(sourceObject, fieldSource, aliasHelper, includeNulls = isIndexHydration) - ?.map { sourceId -> + getSourceInputNodes(sourceObject, fieldSource, aliasHelper, includeNulls = isIndexHydration) + ?.map { sourceInput -> val instruction = executionContext.hooks.getHydrationInstruction( instructions = instructions, - sourceId = sourceId, + sourceInput = sourceInput, userContext = executionContext.userContext, ) - sourceId to instruction + if (instruction == null) { + SourceInput.NotQueryable(sourceInput) + } else { + SourceInput.Queryable( + sourceInputNode = sourceInput, + instruction = instruction, + indexKey = getIndexer(instruction).getSourceKey(sourceInput), + ) + } } } else { // todo: determine what to do here in the longer term, this hook should probably be replaced @@ -447,43 +477,40 @@ internal class NadelNewBatchHydrator( } .singleOfType() - getSourceInputs(sourceObject, fieldSource, aliasHelper, includeNulls = isIndexHydration) - ?.map { sourceId -> - sourceId to instruction + getSourceInputNodes(sourceObject, fieldSource, aliasHelper, includeNulls = isIndexHydration) + ?.map { sourceInput -> + SourceInput.Queryable( + sourceInputNode = sourceInput, + instruction = instruction, + indexKey = getIndexer(instruction).getSourceKey(sourceInput), + ) } } } } /** - * Creates a giant inverted Map of [SourceObjectMetadata.sourceIdsPairedWithInstructions] - * where the instruction is the key and the value is the source ID. + * Groups the [SourceInput] by instruction so that we can gather all the source + * IDs together for a given query. */ - private fun getSourceIdsByInstruction( + private fun groupSourceInputsByInstruction( sourceObjects: List, - ): Map> { + ): Map> { return sourceObjects .asSequence() .flatMap { - it.sourceIdsPairedWithInstructions ?: emptyList() + it.sourceInputs ?: emptyList() + } + .filterIsInstance() + .groupBy { sourceInput -> + sourceInput.instruction } - // Removes Pair values where instruction is null - .filterPairSecondNotNull() - .groupBy( - // Pair - keySelector = { (_, instruction) -> - instruction - }, - valueTransform = { (sourceId, _) -> - sourceId - }, - ) } /** - * Gets the source inputs for [sourceObject] + * Gets the [JsonNode] source inputs for [sourceObject] */ - private fun getSourceInputs( + private fun getSourceInputNodes( sourceObject: JsonNode, valueSource: ValueSource.FieldResultValue, aliasHelper: NadelAliasHelper, diff --git a/lib/src/main/java/graphql/nadel/hooks/NadelExecutionHooks.kt b/lib/src/main/java/graphql/nadel/hooks/NadelExecutionHooks.kt index efdd2a5f6..92e794fbb 100644 --- a/lib/src/main/java/graphql/nadel/hooks/NadelExecutionHooks.kt +++ b/lib/src/main/java/graphql/nadel/hooks/NadelExecutionHooks.kt @@ -53,7 +53,7 @@ interface NadelExecutionHooks { fun getHydrationInstruction( instructions: List, - sourceId: JsonNode, + sourceInput: JsonNode, userContext: Any?, ): T? { return instructions.single() diff --git a/test/src/test/kotlin/graphql/nadel/tests/hooks/batch-hydration-instruction-hook-returns-null.kt b/test/src/test/kotlin/graphql/nadel/tests/hooks/batch-hydration-instruction-hook-returns-null.kt index ecabccf52..054916b09 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/hooks/batch-hydration-instruction-hook-returns-null.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/hooks/batch-hydration-instruction-hook-returns-null.kt @@ -25,10 +25,10 @@ class `new-batch-hydration-instruction-hook-returns-null` : EngineTestHook { override fun getHydrationInstruction( instructions: List, - sourceId: JsonNode, + sourceInput: JsonNode, userContext: Any?, ): T? { - return if (sourceId.value.toString().contains("NULL", ignoreCase = true)) { + return if (sourceInput.value.toString().contains("NULL", ignoreCase = true)) { null } else { instructions.single() diff --git a/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-conditional-hydration-in-abstract-type.kt b/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-conditional-hydration-in-abstract-type.kt index e4e34a9a4..683df25ab 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-conditional-hydration-in-abstract-type.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-conditional-hydration-in-abstract-type.kt @@ -21,10 +21,10 @@ class `new-batching-conditional-hydration-in-abstract-type` : EngineTestHook { object : NadelExecutionHooks { override fun getHydrationInstruction( instructions: List, - sourceId: JsonNode, + sourceInput: JsonNode, userContext: Any?, ): T { - val type = (sourceId.value as String).substringBefore("/") + val type = (sourceInput.value as String).substringBefore("/") return instructions .first { diff --git a/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-multiple-source-ids-going-to-different-services.kt b/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-multiple-source-ids-going-to-different-services.kt index f460ccb07..62f1ce367 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-multiple-source-ids-going-to-different-services.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/hooks/new-batching-multiple-source-ids-going-to-different-services.kt @@ -21,10 +21,10 @@ class `new-batching-multiple-source-ids-going-to-different-services` : EngineTes object : NadelExecutionHooks { override fun getHydrationInstruction( instructions: List, - sourceId: JsonNode, + sourceInput: JsonNode, userContext: Any?, ): T { - val type = (sourceId.value as String).substringBefore("/") + val type = (sourceInput.value as String).substringBefore("/") return instructions .first { diff --git a/test/src/test/kotlin/graphql/nadel/tests/hooks/new-polymorphic-hydration-instructions-use-different-inputs.kt b/test/src/test/kotlin/graphql/nadel/tests/hooks/new-polymorphic-hydration-instructions-use-different-inputs.kt index 7f0ef312a..e0ec1e194 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/hooks/new-polymorphic-hydration-instructions-use-different-inputs.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/hooks/new-polymorphic-hydration-instructions-use-different-inputs.kt @@ -32,7 +32,7 @@ class `new-polymorphic-hydration-instructions-use-different-inputs` : EngineTest override fun getHydrationInstruction( instructions: List, - sourceId: JsonNode, + sourceInput: JsonNode, userContext: Any?, ): T? { throw UnsupportedOperationException()