Remove all document variables hint

Author: temaEmelyan

lib/src/main/java/graphql/nadel/NadelExecutionHints.kt

@@ -1,6 +1,5 @@
 package graphql.nadel
 
-import graphql.nadel.hints.AllDocumentVariablesHint
 import graphql.nadel.hints.LegacyOperationNamesHint
 import graphql.nadel.hints.NadelBatchRootFieldsHint
 import graphql.nadel.hints.NadelDeferSupportHint
@@ -18,7 +17,6 @@ import graphql.nadel.hints.NewResultMergerAndNamespacedTypename
 
 data class NadelExecutionHints(
     val legacyOperationNames: LegacyOperationNamesHint,
-    val allDocumentVariablesHint: AllDocumentVariablesHint,
     val newResultMergerAndNamespacedTypename: NewResultMergerAndNamespacedTypename,
     val deferSupport: NadelDeferSupportHint,
     val sharedTypeRenames: NadelSharedTypeRenamesHint,
@@ -45,7 +43,6 @@ data class NadelExecutionHints(
 
     class Builder {
         private var legacyOperationNames = LegacyOperationNamesHint { false }
-        private var allDocumentVariablesHint = AllDocumentVariablesHint { false }
         private var newResultMergerAndNamespacedTypename = NewResultMergerAndNamespacedTypename { false }
         private var deferSupport = NadelDeferSupportHint { false }
         private var shortCircuitEmptyQuery = NadelShortCircuitEmptyQueryHint { false }
@@ -64,7 +61,6 @@ data class NadelExecutionHints(
 
         constructor(nadelExecutionHints: NadelExecutionHints) {
             legacyOperationNames = nadelExecutionHints.legacyOperationNames
-            allDocumentVariablesHint = nadelExecutionHints.allDocumentVariablesHint
             newResultMergerAndNamespacedTypename = nadelExecutionHints.newResultMergerAndNamespacedTypename
             deferSupport = nadelExecutionHints.deferSupport
             shortCircuitEmptyQuery = nadelExecutionHints.shortCircuitEmptyQuery
@@ -85,11 +81,6 @@ data class NadelExecutionHints(
             return this
         }
 
-        fun allDocumentVariablesHint(flag: AllDocumentVariablesHint): Builder {
-            allDocumentVariablesHint = flag
-            return this
-        }
-
         fun newResultMergerAndNamespacedTypename(flag: NewResultMergerAndNamespacedTypename): Builder {
             newResultMergerAndNamespacedTypename = flag
             return this
@@ -158,7 +149,6 @@ data class NadelExecutionHints(
         fun build(): NadelExecutionHints {
             return NadelExecutionHints(
                 legacyOperationNames,
-                allDocumentVariablesHint,
                 newResultMergerAndNamespacedTypename,
                 deferSupport,
                 sharedTypeRenames,

lib/src/main/java/graphql/nadel/NextgenEngine.kt

@@ -56,7 +56,6 @@ import graphql.nadel.util.OperationNameUtil
 import graphql.nadel.validation.NadelSchemaValidation
 import graphql.normalized.ExecutableNormalizedField
 import graphql.normalized.ExecutableNormalizedOperationFactory.createExecutableNormalizedOperationWithRawVariables
-import graphql.normalized.VariablePredicate
 import graphql.schema.GraphQLSchema
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
@@ -384,15 +383,13 @@ internal class NextgenEngine(
 
         val executionInput = executionContext.executionInput
 
-        val jsonPredicate: VariablePredicate = getDocumentVariablePredicate(executionContext.hints, service)
-
         val compileResult = timer.time(step = DocumentCompilation) {
             compileToDocument(
                 schema = service.underlyingSchema,
                 operationKind = topLevelFields.first().getOperationKind(engineSchema),
                 operationName = getOperationName(service, executionContext),
                 topLevelFields = topLevelFields,
-                variablePredicate = jsonPredicate,
+                variablePredicate = DocumentPredicates.allVariablesPredicate,
                 deferSupport = executionContext.hints.deferSupport(),
                 forcePrintBareFields = forcePrintBareFields,
             )
@@ -498,14 +495,6 @@ internal class NextgenEngine(
             && topLevelField.children.all { it.name == TypeNameMetaFieldDef.name }
     }
 
-    private fun getDocumentVariablePredicate(hints: NadelExecutionHints, service: Service): VariablePredicate {
-        return if (hints.allDocumentVariablesHint.invoke(service)) {
-            DocumentPredicates.allVariablesPredicate
-        } else {
-            DocumentPredicates.jsonPredicate
-        }
-    }
-
     private fun getOperationName(service: Service, executionContext: NadelExecutionContext): String? {
         val originalOperationName = executionContext.query.operationName
         return if (executionContext.hints.legacyOperationNames(service)) {

lib/src/main/java/graphql/nadel/engine/document/DocumentPredicates.kt

@@ -5,14 +5,6 @@ import graphql.normalized.VariablePredicate
 class DocumentPredicates {
 
     companion object {
-        /**
-         * A predicate that causes JSON arguments to be compiled as variables
-         */
-        val jsonPredicate =
-            VariablePredicate { _, _, normalizedInputValue ->
-                "JSON" == normalizedInputValue.unwrappedTypeName && normalizedInputValue.value != null
-            }
-
         /**
          * A predicate that causes ALL arguments to be compiled as variables
          */

lib/src/main/java/graphql/nadel/engine/transform/hydration/NadelHydrationInputBuilder.kt

@@ -124,7 +124,7 @@ internal class NadelHydrationInputBuilder private constructor(
         inputDef: NadelHydrationArgument,
     ): NormalizedInputValue? {
         return when (val valueSource = inputDef.valueSource) {
-            is ValueSource.ArgumentValue -> getArgumentValue(valueSource)
+            is ValueSource.ArgumentValue -> getArgumentValue(inputDef, valueSource)
             is ValueSource.FieldResultValue -> makeInputValue(
                 inputDef,
                 value = getResultValue(valueSource),
@@ -162,9 +162,16 @@ internal class NadelHydrationInputBuilder private constructor(
     }
 
     private fun getArgumentValue(
+        inputDef: NadelHydrationArgument,
         valueSource: ValueSource.ArgumentValue,
     ): NormalizedInputValue? {
-        return virtualField.getNormalizedArgument(valueSource.argumentName) ?: valueSource.defaultValue
+        val argumentValue = virtualField.getNormalizedArgument(valueSource.argumentName) ?: valueSource.defaultValue
+        return argumentValue?.let {
+            NormalizedInputValue(
+                GraphQLTypeUtil.simplePrint(inputDef.backingArgumentDef.type),
+                it.value,
+            )
+        }
     }
 
     private fun getResultValue(

lib/src/main/java/graphql/nadel/engine/transform/hydration/batch/NadelBatchHydrationInputBuilder.kt

@@ -30,7 +30,10 @@ internal object NadelBatchHydrationInputBuilder {
                             virtualField.normalizedArguments[valueSource.argumentName]
                                 ?: valueSource.defaultValue
                         if (argValue != null) {
-                            argument to argValue
+                            argument to NormalizedInputValue(
+                                GraphQLTypeUtil.simplePrint(argument.backingArgumentDef.type),
+                                argValue.value,
+                            )
                         } else {
                             null
                         }

lib/src/main/java/graphql/nadel/hints/AllDocumentVariablesHint.kt

@@ -67,7 +67,6 @@ class NadelPrefixTest {
                 "graphql.nadel.engine.transform.result.json.JsonNodeExtractor",
                 "graphql.nadel.engine.transform.result.json.JsonNodes",
                 "graphql.nadel.engine.util.AliasesKt",
-                "graphql.nadel.hints.AllDocumentVariablesHint",
                 "graphql.nadel.hints.LegacyOperationNamesHint",
                 "graphql.nadel.hints.NewBatchHydrationGroupingHint",
                 "graphql.nadel.hints.NewResultMergerAndNamespacedTypename",

lib/src/test/kotlin/graphql/nadel/archunit/NadelPrefixTest.kt

@@ -5,8 +5,6 @@ import graphql.GraphQLError
 import graphql.incremental.DeferPayload
 import graphql.incremental.DelayedIncrementalPartialResult
 import graphql.incremental.DelayedIncrementalPartialResultImpl.newIncrementalExecutionResult
-import graphql.language.AstPrinter
-import graphql.language.AstSorter
 import graphql.nadel.Nadel
 import graphql.nadel.NadelExecutionHints
 import graphql.nadel.NadelExecutionInput.Companion.newNadelExecutionInput
@@ -34,6 +32,7 @@ import kotlinx.coroutines.reactive.asPublisher
 import org.junit.jupiter.api.fail
 import org.reactivestreams.Publisher
 import java.io.File
+import java.math.BigDecimal
 import java.math.BigInteger
 import java.util.concurrent.CompletableFuture
 
@@ -163,7 +162,6 @@ private suspend fun execute(
             .overallWiringFactory(testHook.wiringFactory)
             .underlyingWiringFactory(testHook.wiringFactory)
             .serviceExecutionFactory(object : ServiceExecutionFactory {
-                private val astSorter = AstSorter()
                 private val serviceCalls = fixture.serviceCalls.toMutableList()
 
                 override fun getServiceExecution(serviceName: String): ServiceExecution {
@@ -172,9 +170,11 @@ private suspend fun execute(
                             val incomingQuery = params.query
                             val actualVariables = fixVariables(params.variables)
                             val actualOperationName = params.operationDefinition.name
-                            val actualQuery = AstPrinter.printAst(
-                                astSorter.sort(incomingQuery),
+                            val actualRequest = canonicalizeServiceRequest(
+                                document = incomingQuery,
+                                variables = actualVariables,
                             )
+                            val actualQuery = actualRequest.query
                             printSyncLine(actualQuery)
 
                             fun failWithFixtureContext(message: String): Nothing {
@@ -192,10 +192,14 @@ private suspend fun execute(
                             synchronized(serviceCalls) {
                                 val indexOfCall = serviceCalls
                                     .indexOfFirst {
+                                        val expectedRequest = canonicalizeServiceRequest(
+                                            document = it.request.document,
+                                            variables = it.request.variables,
+                                        )
+
                                         it.serviceName == serviceName
-                                            && AstPrinter.printAst(it.request.document) == actualQuery
+                                            && expectedRequest == actualRequest
                                             && it.request.operationName == actualOperationName
-                                            && it.request.variables == actualVariables
                                     }
                                     .takeIf { it != -1 }
 
@@ -303,6 +307,9 @@ private suspend fun execute(
                         } else {
                             value.toLong()
                         }
+                    } else if (value is BigDecimal) {
+                        // Jackson parses floating point fixture variables as Double
+                        value.toDouble()
                     } else if (value is AnyMap) {
                         @Suppress("UNCHECKED_CAST")
                         fixVariables(value as JsonMap)

test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt

@@ -0,0 +1,115 @@
+package graphql.nadel.tests
+
+import graphql.language.AstPrinter
+import graphql.language.AstSorter
+import graphql.language.AstTransformer
+import graphql.language.Document
+import graphql.language.Node
+import graphql.language.NodeTraverser
+import graphql.language.NodeVisitorStub
+import graphql.language.OperationDefinition
+import graphql.language.VariableDefinition
+import graphql.language.VariableReference
+import graphql.nadel.engine.util.JsonMap
+import graphql.parser.Parser
+import graphql.util.TraversalControl
+import graphql.util.TraverserContext
+import graphql.util.TreeTransformerUtil.changeNode
+
+internal data class CanonicalServiceRequest(
+    val query: String,
+    val variables: JsonMap,
+)
+
+internal fun canonicalizeServiceRequest(
+    query: String,
+    variables: JsonMap,
+): CanonicalServiceRequest {
+    return canonicalizeServiceRequest(
+        document = Parser().parseDocument(query),
+        variables = variables,
+    )
+}
+
+internal fun canonicalizeServiceRequest(
+    document: Document,
+    variables: JsonMap,
+): CanonicalServiceRequest {
+    val sortedDocument = AstSorter().sort(document)
+    val variableNames = linkedSetOf<String>()
+
+    NodeTraverser().preOrder(
+        object : NodeVisitorStub() {
+            override fun visitVariableReference(
+                node: VariableReference,
+                context: TraverserContext<Node<*>>,
+            ): TraversalControl {
+                variableNames += node.name
+                return TraversalControl.CONTINUE
+            }
+        },
+        sortedDocument,
+    )
+
+    val operation = sortedDocument.definitions
+        .filterIsInstance<OperationDefinition>()
+        .single()
+    val definedVariableNames = operation.variableDefinitions
+        .mapTo(linkedSetOf()) { it.name }
+
+    require(variableNames == definedVariableNames) {
+        "Service query variable definitions and references must match"
+    }
+    require(variables.keys.all(definedVariableNames::contains)) {
+        "Service variables must be declared by the service query"
+    }
+
+    val canonicalNames = variableNames
+        .withIndex()
+        .associate { (index, name) ->
+            name to "v$index"
+        }
+
+    val renamedDocument = AstTransformer().transform(
+        sortedDocument,
+        object : NodeVisitorStub() {
+            override fun visitVariableDefinition(
+                node: VariableDefinition,
+                context: TraverserContext<Node<*>>,
+            ): TraversalControl {
+                return changeNode(
+                    context,
+                    node.transform { builder ->
+                        builder.name(canonicalNames.getValue(node.name))
+                    },
+                )
+            }
+
+            override fun visitVariableReference(
+                node: VariableReference,
+                context: TraverserContext<Node<*>>,
+            ): TraversalControl {
+                return changeNode(
+                    context,
+                    node.transform { builder ->
+                        builder.name(canonicalNames.getValue(node.name))
+                    },
+                )
+            }
+        },
+    ) as Document
+
+    val canonicalVariables = linkedMapOf<String, Any?>()
+    canonicalNames.forEach { (name, canonicalName) ->
+        if (variables.containsKey(name)) {
+            canonicalVariables[canonicalName] = variables[name]
+        }
+    }
+
+    return CanonicalServiceRequest(
+        query = AstPrinter.printAst(
+            AstSorter().sort(renamedDocument),
+        ),
+        variables = canonicalVariables,
+    )
+}