From 1e2f9f56d8461d7a380b71ca52f5042e555425dc Mon Sep 17 00:00:00 2001 From: Franklin Wang Date: Thu, 19 Dec 2024 16:00:37 +1100 Subject: [PATCH] Performance improvements --- .../nadel/engine/NadelExecutionContext.kt | 4 - .../NadelIncrementalResultAccumulator.kt | 4 +- .../engine/NadelIncrementalResultSupport.kt | 12 +- .../blueprint/NadelExecutionBlueprint.kt | 74 ++++---- .../engine/plan/NadelExecutionPlanFactory.kt | 63 ++++--- .../transform/NadelDeepRenameTransform.kt | 3 +- .../engine/transform/NadelRenameTransform.kt | 3 +- .../NadelServiceTypeFilterTransform.kt | 48 ++--- .../hydration/NadelHydrationTransform.kt | 3 +- .../batch/NadelBatchHydrationTransform.kt | 3 +- .../partition/NadelPartitionTransform.kt | 3 +- .../transform/query/NadelQueryTransformer.kt | 58 +++--- .../graphql/nadel/engine/util/GraphQLUtil.kt | 33 +++- .../java/graphql/nadel/util/NadelFieldMap.kt | 172 ++++++++++++++++++ .../java/graphql/nadel/util/TraversalUtil.kt | 35 ++++ .../nadel/validation/NadelSchemaValidation.kt | 12 +- .../nadel/validation/NadelTypeValidation.kt | 10 +- .../NadelIncrementalResultSupportTest.kt | 24 +-- .../graphql/nadel/util/TraversalUtilKtTest.kt | 84 +++++++++ 19 files changed, 478 insertions(+), 170 deletions(-) create mode 100644 lib/src/main/java/graphql/nadel/util/NadelFieldMap.kt create mode 100644 lib/src/main/java/graphql/nadel/util/TraversalUtil.kt create mode 100644 lib/src/test/kotlin/graphql/nadel/util/TraversalUtilKtTest.kt diff --git a/lib/src/main/java/graphql/nadel/engine/NadelExecutionContext.kt b/lib/src/main/java/graphql/nadel/engine/NadelExecutionContext.kt index ac41cd2f4..b7c15e668 100644 --- a/lib/src/main/java/graphql/nadel/engine/NadelExecutionContext.kt +++ b/lib/src/main/java/graphql/nadel/engine/NadelExecutionContext.kt @@ -28,10 +28,6 @@ data class NadelExecutionContext internal constructor( ) { private val serviceContexts = ConcurrentHashMap>() - @Deprecated("Use incrementalSupport instead", ReplaceWith("incrementalResultSupport")) - internal val deferSupport: NadelIncrementalResultSupport - get() = incrementalResultSupport - val userContext: Any? get() { return executionInput.context diff --git a/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultAccumulator.kt b/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultAccumulator.kt index 5eec863ac..44306f099 100644 --- a/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultAccumulator.kt +++ b/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultAccumulator.kt @@ -189,7 +189,7 @@ class NadelIncrementalResultAccumulator( /** * Similar to [java.io.File.walkTopDown] but for ENFs. */ -private fun ExecutableNormalizedOperation.walkTopDown(): Sequence { +internal fun ExecutableNormalizedOperation.walkTopDown(): Sequence { return topLevelFields .asSequence() .flatMap { @@ -200,7 +200,7 @@ private fun ExecutableNormalizedOperation.walkTopDown(): Sequence { +internal fun ExecutableNormalizedField.walkTopDown(): Sequence { return sequenceOf(this) + children.asSequence() .flatMap { it.walkTopDown() diff --git a/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultSupport.kt b/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultSupport.kt index af6f28ffa..232f76e82 100644 --- a/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultSupport.kt +++ b/lib/src/main/java/graphql/nadel/engine/NadelIncrementalResultSupport.kt @@ -23,16 +23,20 @@ import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger class NadelIncrementalResultSupport internal constructor( - private val accumulator: NadelIncrementalResultAccumulator, + lazyAccumulator: Lazy, private val delayedResultsChannel: Channel = makeDefaultChannel(), ) { + private val accumulator by lazyAccumulator + internal constructor( operation: ExecutableNormalizedOperation, delayedResultsChannel: Channel = makeDefaultChannel(), ) : this( - accumulator = NadelIncrementalResultAccumulator( - operation = operation, - ), + lazyAccumulator = lazy { + NadelIncrementalResultAccumulator( + operation = operation, + ) + }, delayedResultsChannel = delayedResultsChannel, ) diff --git a/lib/src/main/java/graphql/nadel/engine/blueprint/NadelExecutionBlueprint.kt b/lib/src/main/java/graphql/nadel/engine/blueprint/NadelExecutionBlueprint.kt index 59b78b820..86a6683a6 100644 --- a/lib/src/main/java/graphql/nadel/engine/blueprint/NadelExecutionBlueprint.kt +++ b/lib/src/main/java/graphql/nadel/engine/blueprint/NadelExecutionBlueprint.kt @@ -5,8 +5,8 @@ import graphql.nadel.ServiceExecutionHydrationDetails import graphql.nadel.engine.transform.GraphQLObjectTypeName import graphql.nadel.engine.util.emptyOrSingle import graphql.nadel.engine.util.makeFieldCoordinates -import graphql.nadel.engine.util.mapFrom import graphql.nadel.engine.util.strictAssociateBy +import graphql.nadel.util.NadelFieldMap import graphql.normalized.ExecutableNormalizedField import graphql.schema.FieldCoordinates import graphql.schema.GraphQLSchema @@ -16,7 +16,7 @@ import graphql.schema.GraphQLSchema */ data class NadelOverallExecutionBlueprint( val engineSchema: GraphQLSchema, - val fieldInstructions: Map>, + val fieldInstructions: NadelFieldMap>, private val underlyingTypeNamesByService: Map>, private val overallTypeNamesByService: Map>, private val underlyingBlueprints: Map, @@ -72,6 +72,38 @@ data class NadelOverallExecutionBlueprint( return coordinatesToService[fieldCoordinates] } + inline fun getTypeNameToInstructionMap( + field: ExecutableNormalizedField, + ): Map { + val map: MutableMap = mutableMapOf() + + field.objectTypeNames.forEach { objectTypeName -> + val instruction = fieldInstructions.get(objectTypeName, field.name) + ?.asSequence() + ?.filterIsInstance() + ?.emptyOrSingle() ?: return@forEach + map[objectTypeName] = instruction + } + + return map + } + + inline fun getTypeNameToInstructionsMap( + field: ExecutableNormalizedField, + ): Map> { + val map: MutableMap> = mutableMapOf() + + field.objectTypeNames.forEach { objectTypeName -> + val instructions = fieldInstructions.get(objectTypeName, field.name) + ?.filterIsInstance() + if (instructions?.isNotEmpty() == true) { + map[objectTypeName] = instructions + } + } + + return map + } + inline fun getInstructionInsideVirtualType( hydrationDetails: ServiceExecutionHydrationDetails?, backingField: ExecutableNormalizedField, @@ -158,41 +190,3 @@ data class NadelTypeRenameInstructions internal constructor( } } } - -/** - * todo: why doesn't this belong inside [NadelOverallExecutionBlueprint] - */ -inline fun Map>.getTypeNameToInstructionMap( - field: ExecutableNormalizedField, -): Map { - return mapFrom( - field.objectTypeNames - .mapNotNull { objectTypeName -> - val coordinates = makeFieldCoordinates(objectTypeName, field.name) - val instruction = this[coordinates] - ?.filterIsInstance() - ?.emptyOrSingle() ?: return@mapNotNull null - objectTypeName to instruction - }, - ) -} - -/** - * todo: why doesn't this belong inside [NadelOverallExecutionBlueprint] - */ -inline fun Map>.getTypeNameToInstructionsMap( - field: ExecutableNormalizedField, -): Map> { - return mapFrom( - field.objectTypeNames - .mapNotNull { objectTypeName -> - val coordinates = makeFieldCoordinates(objectTypeName, field.name) - val instructions = (this[coordinates] ?: return@mapNotNull null) - .filterIsInstance() - when { - instructions.isEmpty() -> return@mapNotNull null - else -> objectTypeName to instructions - } - }, - ) -} diff --git a/lib/src/main/java/graphql/nadel/engine/plan/NadelExecutionPlanFactory.kt b/lib/src/main/java/graphql/nadel/engine/plan/NadelExecutionPlanFactory.kt index 4787383e9..1cc993649 100644 --- a/lib/src/main/java/graphql/nadel/engine/plan/NadelExecutionPlanFactory.kt +++ b/lib/src/main/java/graphql/nadel/engine/plan/NadelExecutionPlanFactory.kt @@ -22,6 +22,7 @@ import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParame import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.RootStep.ExecutionPlanning import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.RootStep.QueryTransforming import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.RootStep.ResultTransforming +import graphql.nadel.util.dfs import graphql.normalized.ExecutableNormalizedField internal class NadelExecutionPlanFactory( @@ -60,33 +61,34 @@ internal class NadelExecutionPlanFactory( rootField: ExecutableNormalizedField, serviceHydrationDetails: ServiceExecutionHydrationDetails?, ): NadelExecutionPlan { - val executionSteps = mutableListOf() + val executionSteps: MutableMap>> = + mutableMapOf() executionContext.timer.batch { timer -> traverseQuery(rootField) { field -> - transformsWithTimingStepInfo.forEach { transformWithTimingInfo -> + val steps = transformsWithTimingStepInfo.mapNotNull { transformWithTimingInfo -> val transform = transformWithTimingInfo.transform // This is a patch to prevent errors // Ideally this should not happen but the proper fix requires more refactoring // See NadelSkipIncludeTransform.isApplicable for more details if (isSkipIncludeSpecialField(field) && ((transform as NadelTransform<*>) !is NadelSkipIncludeTransform)) { - return@forEach - } - - val state = timer.time(step = transformWithTimingInfo.executionPlanTimingStep) { - transform.isApplicable( - executionContext, - serviceExecutionContext, - executionBlueprint, - services, - service, - field, - serviceHydrationDetails, - ) - } + null + } else { + val state = timer.time(step = transformWithTimingInfo.executionPlanTimingStep) { + transform.isApplicable( + executionContext, + serviceExecutionContext, + executionBlueprint, + services, + service, + field, + serviceHydrationDetails, + ) + } - if (state != null) { - executionSteps.add( + if (state == null) { + null + } else { NadelExecutionPlan.Step( service = service, field = field, @@ -94,26 +96,29 @@ internal class NadelExecutionPlanFactory( queryTransformTimingStep = transformWithTimingInfo.queryTransformTimingStep, resultTransformTimingStep = transformWithTimingInfo.resultTransformTimingStep, state = state, - ), - ) + ) + } } } + + if (steps.isNotEmpty()) { + executionSteps[field] = steps + } } } - return NadelExecutionPlan( - executionSteps.groupBy { it.field }, - ) + return NadelExecutionPlan(executionSteps) } - private suspend fun traverseQuery( + private inline fun traverseQuery( root: ExecutableNormalizedField, - consumer: suspend (ExecutableNormalizedField) -> Unit, + consumer: (ExecutableNormalizedField) -> Unit, ) { - consumer(root) - root.children.forEach { - traverseQuery(it, consumer) - } + dfs( + root = root, + getChildren = ExecutableNormalizedField::getChildren, + consumer = consumer, + ) } companion object { diff --git a/lib/src/main/java/graphql/nadel/engine/transform/NadelDeepRenameTransform.kt b/lib/src/main/java/graphql/nadel/engine/transform/NadelDeepRenameTransform.kt index e9e68f02b..6ceca72b0 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/NadelDeepRenameTransform.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/NadelDeepRenameTransform.kt @@ -7,7 +7,6 @@ import graphql.nadel.engine.NadelExecutionContext import graphql.nadel.engine.NadelServiceExecutionContext import graphql.nadel.engine.blueprint.NadelDeepRenameFieldInstruction import graphql.nadel.engine.blueprint.NadelOverallExecutionBlueprint -import graphql.nadel.engine.blueprint.getTypeNameToInstructionMap import graphql.nadel.engine.transform.artificial.NadelAliasHelper import graphql.nadel.engine.transform.query.NFUtil import graphql.nadel.engine.transform.query.NadelQueryPath @@ -91,7 +90,7 @@ internal class NadelDeepRenameTransform : NadelTransform(overallField) if (deepRenameInstructions.isEmpty()) { return null diff --git a/lib/src/main/java/graphql/nadel/engine/transform/NadelRenameTransform.kt b/lib/src/main/java/graphql/nadel/engine/transform/NadelRenameTransform.kt index 48fbf2840..0a30539f2 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/NadelRenameTransform.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/NadelRenameTransform.kt @@ -7,7 +7,6 @@ import graphql.nadel.engine.NadelExecutionContext import graphql.nadel.engine.NadelServiceExecutionContext import graphql.nadel.engine.blueprint.NadelOverallExecutionBlueprint import graphql.nadel.engine.blueprint.NadelRenameFieldInstruction -import graphql.nadel.engine.blueprint.getTypeNameToInstructionMap import graphql.nadel.engine.transform.NadelRenameTransform.State import graphql.nadel.engine.transform.artificial.NadelAliasHelper import graphql.nadel.engine.transform.query.NFUtil.createField @@ -46,7 +45,7 @@ internal class NadelRenameTransform : NadelTransform { overallField: ExecutableNormalizedField, hydrationDetails: ServiceExecutionHydrationDetails?, ): State? { - val renameInstructions = executionBlueprint.fieldInstructions + val renameInstructions = executionBlueprint .getTypeNameToInstructionMap(overallField) if (renameInstructions.isEmpty()) { return null diff --git a/lib/src/main/java/graphql/nadel/engine/transform/NadelServiceTypeFilterTransform.kt b/lib/src/main/java/graphql/nadel/engine/transform/NadelServiceTypeFilterTransform.kt index 7b259d84a..b5e441089 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/NadelServiceTypeFilterTransform.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/NadelServiceTypeFilterTransform.kt @@ -90,22 +90,29 @@ class NadelServiceTypeFilterTransform : NadelTransform { val typeNamesOwnedByService = executionBlueprint.getOverAllTypeNamesForService(service) - val fieldObjectTypeNamesOwnedByService = overallField.objectTypeNames - .filter { objectTypeName -> - isTypeOwnedByService( - objectTypeName, - executionContext, - service, - executionBlueprint, - ) + // Add underlying type names as well to handle combination of hydration and renames. + // Transforms are applied to hydration fields as well, and those fields always reference + // elements from the underlying schema + val underlyingTypeNamesOwnedByService = executionBlueprint.getUnderlyingTypeNamesForService(service) + + // Assume for most cases there aren't foreign types so there is no point filtering to a new List + val noForeignTypes = overallField.objectTypeNames + .all { objectTypeName -> + objectTypeName in typeNamesOwnedByService + || objectTypeName in underlyingTypeNamesOwnedByService + || (executionContext.hints.sharedTypeRenames(service) && executionBlueprint.getUnderlyingTypeName(objectTypeName) in underlyingTypeNamesOwnedByService) } - // All types are owned by service - // Note: one list is a subset of the other, so if size is same, contents are too - if (fieldObjectTypeNamesOwnedByService.size == overallField.objectTypeNames.size) { + if (noForeignTypes) { return null } + val fieldObjectTypeNamesOwnedByService = overallField.objectTypeNames.filter { objectTypeName -> + objectTypeName in typeNamesOwnedByService + || objectTypeName in underlyingTypeNamesOwnedByService + || (executionContext.hints.sharedTypeRenames(service) && executionBlueprint.getUnderlyingTypeName(objectTypeName) in underlyingTypeNamesOwnedByService) + } + return State( aliasHelper = NadelAliasHelper.forField( tag = "type_filter", @@ -117,25 +124,6 @@ class NadelServiceTypeFilterTransform : NadelTransform { ) } - private fun isTypeOwnedByService( - objectTypeName: String, - executionContext: NadelExecutionContext, - service: Service, - executionBlueprint: NadelOverallExecutionBlueprint, - ): Boolean { - val typeNamesOwnedByService = executionBlueprint.getOverAllTypeNamesForService(service) - // Add underlying type names as well to handle combination of hydration and renames. - // Transforms are applied to hydration fields as well, and those fields always reference - // elements from the underlying schema - val underlyingTypeNamesOwnedByService = executionBlueprint.getUnderlyingTypeNamesForService(service) - - // it is MUCH quicker to compare membership in 2 sets rather than - // concat 1 giant set and then check - return objectTypeName in typeNamesOwnedByService - || objectTypeName in underlyingTypeNamesOwnedByService - || (executionContext.hints.sharedTypeRenames(service) && executionBlueprint.getUnderlyingTypeName(objectTypeName) in underlyingTypeNamesOwnedByService) - } - override suspend fun transformField( executionContext: NadelExecutionContext, serviceExecutionContext: NadelServiceExecutionContext, diff --git a/lib/src/main/java/graphql/nadel/engine/transform/hydration/NadelHydrationTransform.kt b/lib/src/main/java/graphql/nadel/engine/transform/hydration/NadelHydrationTransform.kt index 5aaaefcd1..54449f441 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/hydration/NadelHydrationTransform.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/hydration/NadelHydrationTransform.kt @@ -11,7 +11,6 @@ import graphql.nadel.engine.NadelServiceExecutionContext import graphql.nadel.engine.blueprint.NadelGenericHydrationInstruction import graphql.nadel.engine.blueprint.NadelHydrationFieldInstruction import graphql.nadel.engine.blueprint.NadelOverallExecutionBlueprint -import graphql.nadel.engine.blueprint.getTypeNameToInstructionsMap import graphql.nadel.engine.blueprint.hydration.NadelHydrationStrategy import graphql.nadel.engine.transform.GraphQLObjectTypeName import graphql.nadel.engine.transform.NadelTransform @@ -77,7 +76,7 @@ internal class NadelHydrationTransform( overallField: ExecutableNormalizedField, hydrationDetails: ServiceExecutionHydrationDetails?, ): State? { - val instructionsByObjectTypeName = executionBlueprint.fieldInstructions + val instructionsByObjectTypeName = executionBlueprint .getTypeNameToInstructionsMap(overallField) .ifEmpty { if (executionContext.hints.virtualTypeSupport(service)) { diff --git a/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelBatchHydrationTransform.kt b/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelBatchHydrationTransform.kt index 57a151964..314f71ad6 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelBatchHydrationTransform.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelBatchHydrationTransform.kt @@ -8,7 +8,6 @@ import graphql.nadel.engine.NadelExecutionContext import graphql.nadel.engine.NadelServiceExecutionContext import graphql.nadel.engine.blueprint.NadelBatchHydrationFieldInstruction import graphql.nadel.engine.blueprint.NadelOverallExecutionBlueprint -import graphql.nadel.engine.blueprint.getTypeNameToInstructionsMap import graphql.nadel.engine.transform.GraphQLObjectTypeName import graphql.nadel.engine.transform.NadelTransform import graphql.nadel.engine.transform.NadelTransformFieldResult @@ -47,7 +46,7 @@ internal class NadelBatchHydrationTransform( overallField: ExecutableNormalizedField, hydrationDetails: ServiceExecutionHydrationDetails?, ): State? { - val instructionsByObjectTypeName = executionBlueprint.fieldInstructions + val instructionsByObjectTypeName = executionBlueprint .getTypeNameToInstructionsMap(overallField) .ifEmpty { if (executionContext.hints.virtualTypeSupport(service)) { diff --git a/lib/src/main/java/graphql/nadel/engine/transform/partition/NadelPartitionTransform.kt b/lib/src/main/java/graphql/nadel/engine/transform/partition/NadelPartitionTransform.kt index 352f5a70e..40ab21633 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/partition/NadelPartitionTransform.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/partition/NadelPartitionTransform.kt @@ -8,7 +8,6 @@ import graphql.nadel.engine.NadelExecutionContext import graphql.nadel.engine.NadelServiceExecutionContext import graphql.nadel.engine.blueprint.NadelOverallExecutionBlueprint import graphql.nadel.engine.blueprint.NadelPartitionInstruction -import graphql.nadel.engine.blueprint.getTypeNameToInstructionMap import graphql.nadel.engine.transform.NadelTransform import graphql.nadel.engine.transform.NadelTransformFieldResult import graphql.nadel.engine.transform.partition.NadelPartitionMutationPayloadMerger.isMutationPayloadLike @@ -54,7 +53,7 @@ internal class NadelPartitionTransform( return null } - val partitionInstructions = executionBlueprint.fieldInstructions + val partitionInstructions = executionBlueprint .getTypeNameToInstructionMap(overallField) // We can't partition a field that has multiple partition instructions in different types. But, since diff --git a/lib/src/main/java/graphql/nadel/engine/transform/query/NadelQueryTransformer.kt b/lib/src/main/java/graphql/nadel/engine/transform/query/NadelQueryTransformer.kt index 1abeeba51..74b886b2f 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/query/NadelQueryTransformer.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/query/NadelQueryTransformer.kt @@ -57,7 +57,7 @@ class NadelQueryTransformer private constructor( private data class TransformContext( val artificialFields: MutableList = mutableListOf(), - val overallToUnderlyingFields: MutableMap> = mutableMapOf(), + val overallToUnderlyingFields: MutableMap> = mutableMapOf(), ) data class TransformResult( @@ -72,10 +72,6 @@ class NadelQueryTransformer private constructor( val overallToUnderlyingFields: Map>, ) - fun markArtificial(field: ExecutableNormalizedField) { - transformContext.artificialFields.add(field) - } - /** * Helper for calling [transform] for all the given [fields]. */ @@ -124,11 +120,20 @@ class NadelQueryTransformer private constructor( transformContext.artificialFields.addAll(artificialFields) // Track overall -> underlying fields - transformContext.overallToUnderlyingFields.compute(field) { _, oldValue -> - (oldValue ?: emptyList()) + newField + artificialFields - } + transformContext.overallToUnderlyingFields + .computeIfAbsent(field) { + mutableListOf() + } + .also { + it.addAll(newField) + it.addAll(artificialFields) + } - return artificialFields + newField + return if (artificialFields.isEmpty()) { + newField + } else { + newField + artificialFields + } } /** @@ -142,9 +147,13 @@ class NadelQueryTransformer private constructor( .build() .also { newField -> // Track overall -> underlying fields - transformContext.overallToUnderlyingFields.compute(field) { _, oldValue -> - (oldValue ?: emptyList()) + newField - } + transformContext.overallToUnderlyingFields + .computeIfAbsent(field) { + mutableListOf() + } + .also { + it.add(newField) + } } } @@ -152,8 +161,9 @@ class NadelQueryTransformer private constructor( field: ExecutableNormalizedField, transformationSteps: List>, ): NadelTransformFieldResult { - var fieldFromPreviousTransform: ExecutableNormalizedField = field - var aggregatedTransformResult: NadelTransformFieldResult? = null + var newField: ExecutableNormalizedField = field + val artificialFields = mutableListOf() + for (transformStep in transformationSteps) { val transformResultForStep = timer.time(transformStep.queryTransformTimingStep) { transformStep.transform.transformField( @@ -162,21 +172,19 @@ class NadelQueryTransformer private constructor( this, executionBlueprint, service, - fieldFromPreviousTransform, + newField, transformStep.state, ) } - aggregatedTransformResult = if (aggregatedTransformResult == null) { - transformResultForStep - } else { - NadelTransformFieldResult( - newField = transformResultForStep.newField, - artificialFields = aggregatedTransformResult.artificialFields + transformResultForStep.artificialFields, - ) - } - fieldFromPreviousTransform = transformResultForStep.newField ?: break + artificialFields.addAll(transformResultForStep.artificialFields) + newField = transformResultForStep.newField + ?: return NadelTransformFieldResult(null, artificialFields) } - return aggregatedTransformResult!! + + return NadelTransformFieldResult( + newField = newField, + artificialFields = artificialFields, + ) } private fun getUnderlyingTypeNames(objectTypeNames: Collection): List { diff --git a/lib/src/main/java/graphql/nadel/engine/util/GraphQLUtil.kt b/lib/src/main/java/graphql/nadel/engine/util/GraphQLUtil.kt index 8d35bfaff..981cfedb7 100644 --- a/lib/src/main/java/graphql/nadel/engine/util/GraphQLUtil.kt +++ b/lib/src/main/java/graphql/nadel/engine/util/GraphQLUtil.kt @@ -70,6 +70,7 @@ import graphql.schema.GraphQLUnmodifiedType import graphql.schema.idl.TypeUtil import kotlinx.coroutines.future.asDeferred import org.intellij.lang.annotations.Language +import java.lang.invoke.MethodHandles internal typealias AnyAstValue = Value<*> internal typealias AnyAstNode = Node<*> @@ -191,10 +192,17 @@ private fun GraphQLFieldsContainer.getFieldContainerFor( } } +private val ExecutableNormalizedFieldConstructor = ExecutableNormalizedField.Builder::class.java + .getDeclaredConstructor(ExecutableNormalizedField::class.java) + .also { + it.trySetAccessible() + } + .let { + MethodHandles.lookup().unreflectConstructor(it) + } + fun ExecutableNormalizedField.toBuilder(): ExecutableNormalizedField.Builder { - var builder: ExecutableNormalizedField.Builder? = null - transform { builder = it } - return builder!! + return ExecutableNormalizedFieldConstructor.invokeExact(this) as ExecutableNormalizedField.Builder } fun GraphQLCodeRegistry.toBuilder(): GraphQLCodeRegistry.Builder { @@ -204,15 +212,20 @@ fun GraphQLCodeRegistry.toBuilder(): GraphQLCodeRegistry.Builder { } fun GraphQLSchema.toBuilder(): GraphQLSchema.Builder { - var builder: GraphQLSchema.Builder? = null - transform { builder = it } - return builder!! + return GraphQLSchema.newSchema(this) } +private val GraphQLSchemaBuilderWithoutTypesConstructor = GraphQLSchema.BuilderWithoutTypes::class.java + .getDeclaredConstructor(GraphQLSchema::class.java) + .also { + it.trySetAccessible() + } + .let { + MethodHandles.lookup().unreflectConstructor(it) + } + fun GraphQLSchema.toBuilderWithoutTypes(): GraphQLSchema.BuilderWithoutTypes { - var builder: GraphQLSchema.BuilderWithoutTypes? = null - transformWithoutTypes { builder = it } - return builder!! + return GraphQLSchemaBuilderWithoutTypesConstructor.invokeExact(this) as GraphQLSchema.BuilderWithoutTypes } fun ExecutableNormalizedField.copyWithChildren(children: List): ExecutableNormalizedField { @@ -630,7 +643,7 @@ fun compileToDocument( } } -internal fun DelayedIncrementalPartialResult.copy( +fun DelayedIncrementalPartialResult.copy( incremental: List? = this.incremental, extensions: Map? = this.extensions, hasNext: Boolean = this.hasNext(), diff --git a/lib/src/main/java/graphql/nadel/util/NadelFieldMap.kt b/lib/src/main/java/graphql/nadel/util/NadelFieldMap.kt new file mode 100644 index 000000000..d684b1457 --- /dev/null +++ b/lib/src/main/java/graphql/nadel/util/NadelFieldMap.kt @@ -0,0 +1,172 @@ +package graphql.nadel.util + +import graphql.schema.FieldCoordinates + +/** + * Stores values that belong to a particular field. + * + * Refer to one of the constructor methods [groupBy] or [from] + */ +class NadelFieldMap private constructor( + private val map: Map, +) { + operator fun get(fieldCoordinates: FieldCoordinates): T? { + return get(fieldCoordinates.typeName, fieldCoordinates.fieldName) + } + + fun get(objectTypeName: String, fieldName: String): T? { + return map[hashCode(objectTypeName, fieldName)] + } + + val size: Int = map.size + + fun isEmpty(): Boolean { + return map.isEmpty() + } + + fun isNotEmpty(): Boolean { + return map.isNotEmpty() + } + + companion object { + private val emptyMap = NadelFieldMap(kotlin.collections.emptyMap()) + + fun emptyMap(): NadelFieldMap { + @Suppress("UNCHECKED_CAST") + return emptyMap as NadelFieldMap + } + + internal inline fun from( + values: Iterable, + getCoordinates: (T) -> FieldCoordinates, + ): NadelFieldMap { + return from( + values = values, + getCoordinates = getCoordinates, + getValue = { + it + }, + ) + } + + @JvmName("fromFieldCoordinates") + internal inline fun from( + values: Iterable, + getCoordinates: (E) -> FieldCoordinates, + getValue: (E) -> T, + ): NadelFieldMap { + return from( + values = values, + getTypeName = { + getCoordinates(it).typeName + }, + getFieldName = { + getCoordinates(it).fieldName + }, + getValue = getValue, + ) + } + + internal inline fun from( + values: Iterable, + getTypeName: (T) -> String, + getFieldName: (T) -> String, + ): NadelFieldMap { + return from( + values = values, + getTypeName = getTypeName, + getFieldName = getFieldName, + getValue = { + it + }, + ) + } + + internal inline fun from( + values: Iterable, + getTypeName: (E) -> String, + getFieldName: (E) -> String, + getValue: (E) -> T, + ): NadelFieldMap { + val map: MutableMap = mutableMapOf() + values.forEach { value -> + val typeName = getTypeName(value) + val fieldName = getFieldName(value) + map[hashCode(typeName, fieldName)] = getValue(value) + } + return NadelFieldMap(map) + } + + internal inline fun groupBy( + values: List, + getCoordinates: (T) -> FieldCoordinates, + ): NadelFieldMap> { + return groupBy( + values = values, + getCoordinates = getCoordinates, + getValue = { + it + }, + ) + } + + @JvmName("groupByFieldCoordinates") + internal inline fun groupBy( + values: List, + getCoordinates: (E) -> FieldCoordinates, + getValue: (E) -> T, + ): NadelFieldMap> { + return groupBy( + values = values, + getTypeName = { + getCoordinates(it).typeName + }, + getFieldName = { + getCoordinates(it).fieldName + }, + getValue = getValue, + ) + } + + internal inline fun groupBy( + values: List, + getTypeName: (T) -> String, + getFieldName: (T) -> String, + ): NadelFieldMap> { + return groupBy( + values = values, + getTypeName = getTypeName, + getFieldName = getFieldName, + getValue = { + it + }, + ) + } + + internal inline fun groupBy( + values: List, + getTypeName: (E) -> String, + getFieldName: (E) -> String, + getValue: (E) -> T, + ): NadelFieldMap> { + val map: MutableMap> = mutableMapOf() + values.forEach { value -> + val typeName = getTypeName(value) + val fieldName = getFieldName(value) + map + .computeIfAbsent(hashCode(typeName, fieldName)) { + mutableListOf() + } + .add(getValue(value)) + } + return NadelFieldMap(map) + } + + private fun hashCode(typeName: String, fieldName: String): Int { + var result = 1 + result = 31 * result + typeName.hashCode() + result = 31 * result + fieldName.hashCode() + return result + } + } +} diff --git a/lib/src/main/java/graphql/nadel/util/TraversalUtil.kt b/lib/src/main/java/graphql/nadel/util/TraversalUtil.kt new file mode 100644 index 000000000..5084c957a --- /dev/null +++ b/lib/src/main/java/graphql/nadel/util/TraversalUtil.kt @@ -0,0 +1,35 @@ +package graphql.nadel.util + +internal data class NadelDfsNode( + val parent: NadelDfsNode? = null, + val children: List, + var index: Int = children.lastIndex, +) + +internal inline fun dfs( + root: T, + getChildren: (T) -> List, + consumer: (T) -> Unit, +) { + var cursor: NadelDfsNode? = NadelDfsNode( + children = listOf(root), + ) + + while (cursor != null) { + val element = cursor.children[cursor.index--] + consumer(element) + + // Pop + if (cursor.index < 0) { + cursor = cursor.parent + } + + val children = getChildren(element) + if (children.isNotEmpty()) { + cursor = NadelDfsNode( + parent = cursor, + children = children, + ) + } + } +} diff --git a/lib/src/main/java/graphql/nadel/validation/NadelSchemaValidation.kt b/lib/src/main/java/graphql/nadel/validation/NadelSchemaValidation.kt index bfee507f6..ea2acf8ce 100644 --- a/lib/src/main/java/graphql/nadel/validation/NadelSchemaValidation.kt +++ b/lib/src/main/java/graphql/nadel/validation/NadelSchemaValidation.kt @@ -14,6 +14,7 @@ import graphql.nadel.engine.util.strictAssociateBy import graphql.nadel.engine.util.toMapStrictly import graphql.nadel.engine.util.unwrapAll import graphql.nadel.schema.NadelDirectives +import graphql.nadel.util.NadelFieldMap import graphql.schema.FieldCoordinates import graphql.schema.GraphQLFieldsContainer import graphql.schema.GraphQLObjectType @@ -144,8 +145,15 @@ class NadelSchemaValidation internal constructor( return NadelOverallExecutionBlueprint( engineSchema = schemas.engineSchema, - fieldInstructions = fieldInstructions - .groupBy(keySelector = { it.fieldInstruction.location }, valueTransform = { it.fieldInstruction }), + fieldInstructions = NadelFieldMap.groupBy( + values = fieldInstructions, + getCoordinates = { + it.fieldInstruction.location + }, + getValue = { + it.fieldInstruction + }, + ), underlyingTypeNamesByService = typenamesForService.associate { it.service to it.underlyingTypeNames }, overallTypeNamesByService = typenamesForService.associate { it.service to it.overallTypeNames }, underlyingBlueprints = schemas.services.associate { service -> // Blank map to ensure that all services are represented diff --git a/lib/src/main/java/graphql/nadel/validation/NadelTypeValidation.kt b/lib/src/main/java/graphql/nadel/validation/NadelTypeValidation.kt index d2f4ac963..c2fd48c58 100644 --- a/lib/src/main/java/graphql/nadel/validation/NadelTypeValidation.kt +++ b/lib/src/main/java/graphql/nadel/validation/NadelTypeValidation.kt @@ -13,6 +13,8 @@ import graphql.nadel.validation.util.NadelBuiltInTypes.allNadelBuiltInTypeNames import graphql.nadel.validation.util.NadelReferencedType import graphql.nadel.validation.util.NadelSchemaUtil.getUnderlyingType import graphql.nadel.validation.util.getReferencedTypeNames +import graphql.schema.GraphQLFieldDefinition +import graphql.schema.GraphQLFieldsContainer import graphql.schema.GraphQLNamedType import graphql.schema.GraphQLUnionType @@ -31,7 +33,7 @@ internal class NadelTypeValidation( ): NadelSchemaValidationResult { val (serviceTypes, serviceTypeErrors) = getServiceTypes(service) - val serviceTypeMetadata = getReachableTypeMetadata(serviceTypes, service) + val reachableTypeMetadata = getReachableTypeMetadata(serviceTypes, service) val typeValidationResult = serviceTypes .map { @@ -42,10 +44,14 @@ internal class NadelTypeValidation( return results( serviceTypeErrors, typeValidationResult, - serviceTypeMetadata, + reachableTypeMetadata, ) } + fun GraphQLFieldsContainer.hasField(field: GraphQLFieldDefinition): Boolean { + return getFieldDefinition(field.name) != null + } + private fun getReachableTypeMetadata( serviceTypes: List, service: Service, diff --git a/lib/src/test/kotlin/graphql/nadel/engine/NadelIncrementalResultSupportTest.kt b/lib/src/test/kotlin/graphql/nadel/engine/NadelIncrementalResultSupportTest.kt index 2029c58f6..d620e972b 100644 --- a/lib/src/test/kotlin/graphql/nadel/engine/NadelIncrementalResultSupportTest.kt +++ b/lib/src/test/kotlin/graphql/nadel/engine/NadelIncrementalResultSupportTest.kt @@ -43,7 +43,7 @@ class NadelIncrementalResultSupportTest { @Test fun `channel closes once initial result comes in and there are no pending defer jobs`() { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) assertFalse(channel.isClosedForSend) assertFalse(channel.isClosedForReceive) @@ -60,7 +60,7 @@ class NadelIncrementalResultSupportTest { fun `after last job the hasNext is false`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) // Use locks to continue the deferred jobs when we release the lock val firstLock = Mutex(true) val secondLock = Mutex(true) @@ -142,7 +142,7 @@ class NadelIncrementalResultSupportTest { fun `does not send anything before onInitialResultComplete is invoked`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) val lock = CompletableDeferred() every { @@ -197,7 +197,7 @@ class NadelIncrementalResultSupportTest { @Test fun `hasNext is true if last job launches more jobs`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) val firstLock = CompletableDeferred() val secondLock = CompletableDeferred() @@ -269,7 +269,7 @@ class NadelIncrementalResultSupportTest { fun `hasNext is true if there is another job still running`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) val firstLock = CompletableDeferred() val secondLock = CompletableDeferred() @@ -346,7 +346,7 @@ class NadelIncrementalResultSupportTest { fun `emits nothing if accumulator returns null`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) every { accumulator.accumulate(any()) @@ -391,7 +391,7 @@ class NadelIncrementalResultSupportTest { fun `forwards responses from multiple Flows`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) every { accumulator.accumulate(any()) @@ -482,7 +482,7 @@ class NadelIncrementalResultSupportTest { runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) val failureMutex = Mutex(true) @@ -548,7 +548,7 @@ class NadelIncrementalResultSupportTest { fun `handles empty Flow`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) // When subject.defer(emptyFlow()) @@ -564,7 +564,7 @@ class NadelIncrementalResultSupportTest { fun `Flow can launch more defer jobs`() = runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) val childLock = Mutex(locked = true) every { @@ -638,7 +638,7 @@ class NadelIncrementalResultSupportTest { runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) every { accumulator.accumulate(any()) @@ -692,7 +692,7 @@ class NadelIncrementalResultSupportTest { runTest { val channel = Channel(UNLIMITED) - val subject = NadelIncrementalResultSupport(accumulator, channel) + val subject = NadelIncrementalResultSupport(lazy { accumulator }, channel) val lock = CompletableDeferred() assertFalse(channel.isClosedForSend) diff --git a/lib/src/test/kotlin/graphql/nadel/util/TraversalUtilKtTest.kt b/lib/src/test/kotlin/graphql/nadel/util/TraversalUtilKtTest.kt new file mode 100644 index 000000000..1a432d423 --- /dev/null +++ b/lib/src/test/kotlin/graphql/nadel/util/TraversalUtilKtTest.kt @@ -0,0 +1,84 @@ +package graphql.nadel.util + +import kotlin.test.Test +import kotlin.test.assertTrue + +class TraversalUtilKtTest { + @Test + fun `dfs traverses`() { + data class Node( + val label: String, + val children: List, + ) + + val root = Node( + label = "A", + children = listOf( + Node( + label = "B", + children = listOf( + Node( + label = "D", + children = listOf( + + ), + ), + Node( + label = "E", + children = listOf( + Node( + label = "F", + children = listOf( + ), + ), + ), + ), + ), + ), + Node( + label = "C", + children = listOf( + + ), + ), + ), + ) + + val visited = mutableListOf() + + dfs( + root, + getChildren = Node::children, + consumer = { + visited += it.label + } + ) + + assertTrue(visited == listOf("A", "C", "B", "E", "F", "D")) + } + + @Test + fun `dfs empty children`() { + data class Node( + val label: String, + val children: List, + ) + + val root = Node( + label = "A", + children = listOf(), + ) + + val visited = mutableListOf() + + dfs( + root, + getChildren = Node::children, + consumer = { + visited += it.label + } + ) + + assertTrue(visited == listOf("A")) + } +}