Remove all document variables hint

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
@@ -15,7 +14,6 @@ import graphql.nadel.hints.NadelSharedTypeRenamesHint
 
 data class NadelExecutionHints(
     val legacyOperationNames: LegacyOperationNamesHint,
-    val allDocumentVariablesHint: AllDocumentVariablesHint,
     val deferSupport: NadelDeferSupportHint,
     val sharedTypeRenames: NadelSharedTypeRenamesHint,
     val executeOnEngineSchema: NadelExecuteOnEngineSchemaHint,
@@ -39,7 +37,6 @@ data class NadelExecutionHints(
 
     class Builder {
         private var legacyOperationNames = LegacyOperationNamesHint { false }
-        private var allDocumentVariablesHint = AllDocumentVariablesHint { false }
         private var deferSupport = NadelDeferSupportHint { false }
         private var sharedTypeRenames = NadelSharedTypeRenamesHint { false }
         private var executeOnEngineSchema = NadelExecuteOnEngineSchemaHint { false }
@@ -55,7 +52,6 @@ data class NadelExecutionHints(
 
         constructor(nadelExecutionHints: NadelExecutionHints) {
             legacyOperationNames = nadelExecutionHints.legacyOperationNames
-            allDocumentVariablesHint = nadelExecutionHints.allDocumentVariablesHint
             deferSupport = nadelExecutionHints.deferSupport
             sharedTypeRenames = nadelExecutionHints.sharedTypeRenames
             executeOnEngineSchema = nadelExecutionHints.executeOnEngineSchema
@@ -73,11 +69,6 @@ data class NadelExecutionHints(
             return this
         }
 
-        fun allDocumentVariablesHint(flag: AllDocumentVariablesHint): Builder {
-            allDocumentVariablesHint = flag
-            return this
-        }
-
         fun deferSupport(flag: NadelDeferSupportHint): Builder {
             deferSupport = flag
             return this
@@ -131,7 +122,6 @@ data class NadelExecutionHints(
         fun build(): NadelExecutionHints {
             return NadelExecutionHints(
                 legacyOperationNames,
-                allDocumentVariablesHint,
                 deferSupport,
                 sharedTypeRenames,
                 executeOnEngineSchema,

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

@@ -57,7 +57,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
@@ -380,15 +379,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,
             )
@@ -494,14 +491,6 @@ internal class NextgenEngine(
             && topLevelField.children.none(::isSkipIncludeArtificialField)
     }
 
-    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/test/kotlin/graphql/nadel/archunit/NadelPrefixTest.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.hooks.CreateServiceContextParams",

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

@@ -8,6 +8,7 @@ import graphql.nadel.ServiceExecution
 import graphql.nadel.engine.transform.NadelTransform
 import graphql.nadel.schema.NeverWiringFactory
 import graphql.nadel.schema.SchemaTransformationHook
+import graphql.nadel.tests.legacy.NadelLegacyIntegrationTest
 import graphql.nadel.tests.util.join
 import graphql.nadel.tests.util.toSlug
 import graphql.nadel.validation.NadelSchemaValidationError
@@ -129,20 +130,26 @@ private object Util {
 
         // TODO: provide single source of truth for this logic - duplicated in EngineTests
         @Suppress("RECEIVER_NULLABILITY_MISMATCH_BASED_ON_JAVA_ANNOTATIONS")
-        val allFixtureFileNames = File(javaClass.classLoader.getResource("fixtures").path)
+        val allTestNames = File(javaClass.classLoader.getResource("fixtures").path)
             .walkTopDown()
             .filter { it.extension == "yml" || it.extension == "yaml" }
             .map { it.nameWithoutExtension }
             .toHashSet()
+            .apply {
+                Reflections(NadelLegacyIntegrationTest::class.java.packageName)
+                    .getSubTypesOf(NadelLegacyIntegrationTest::class.java)
+                    .mapTo(this) { it.simpleName.toSlug() }
+            }
 
-        hookImpls
+        val hooksWithoutTests = hookImpls
             .filter { it.isAnnotationPresent(UseHook::class.java) }
-            .forEach { hookImpl ->
-                val fixtureName = hookImpl.simpleName
-                if (fixtureName !in allFixtureFileNames) {
-                    error("Unable to find matching test for hook: $fixtureName")
-                }
-            }
+            .map { it.simpleName }
+            .filterNot { it in allTestNames }
+            .sorted()
+
+        check(hooksWithoutTests.isEmpty()) {
+            "Unable to find matching tests for hooks: ${hooksWithoutTests.joinToString()}"
+        }
 
         return true
     }

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

@@ -34,6 +34,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
 
@@ -193,9 +194,13 @@ private suspend fun execute(
                                 val indexOfCall = serviceCalls
                                     .indexOfFirst {
                                         it.serviceName == serviceName
-                                            && AstPrinter.printAst(it.request.document) == actualQuery
                                             && it.request.operationName == actualOperationName
-                                            && it.request.variables == actualVariables
+                                            && serviceRequestsMatchIgnoringVariableNames(
+                                                expectedDocument = it.request.document,
+                                                expectedVariables = it.request.variables,
+                                                actualDocument = incomingQuery,
+                                                actualVariables = actualVariables,
+                                            )
                                     }
                                     .takeIf { it != -1 }
 
@@ -303,6 +308,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/ServiceRequestComparator.kt

@@ -0,0 +1,132 @@
+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.util.TraversalControl
+import graphql.util.TraverserContext
+import graphql.util.TreeTransformerUtil.changeNode
+
+/**
+ * Compares service requests while allowing variables to be consistently renamed.
+ *
+ * Variable names are paired by their first use in the sorted queries. The same
+ * pairing is then applied to the actual query and its variables map before both
+ * are compared exactly.
+ */
+internal fun serviceRequestsMatchIgnoringVariableNames(
+    expectedDocument: Document,
+    expectedVariables: JsonMap,
+    actualDocument: Document,
+    actualVariables: JsonMap,
+): Boolean {
+    val expected = prepareServiceRequest(expectedDocument, expectedVariables)
+    val actual = prepareServiceRequest(actualDocument, actualVariables)
+
+    if (expected.variableNames.size != actual.variableNames.size) {
+        return false
+    }
+
+    val actualToExpectedNames = actual.variableNames
+        .zip(expected.variableNames)
+        .toMap()
+    val renamedActualDocument = actual.document.renameVariables(actualToExpectedNames)
+    val renamedActualVariables = actual.variables
+        .mapKeys { (name, _) -> actualToExpectedNames.getValue(name) }
+
+    return AstPrinter.printAstCompact(expected.document) ==
+        AstPrinter.printAstCompact(AstSorter().sort(renamedActualDocument)) &&
+        compareJsonObject(
+            expected = expected.variables,
+            actual = renamedActualVariables,
+        ).passed()
+}
+
+private data class PreparedServiceRequest(
+    val document: Document,
+    val variableNames: List<String>,
+    val variables: JsonMap,
+)
+
+private fun prepareServiceRequest(
+    document: Document,
+    variables: JsonMap,
+): PreparedServiceRequest {
+    val sortedDocument = AstSorter().sort(document)
+    val operation = sortedDocument.definitions
+        .filterIsInstance<OperationDefinition>()
+        .single()
+    val definedVariableNames = operation.variableDefinitions
+        .map { it.name }
+
+    require(definedVariableNames.distinct().size == definedVariableNames.size) {
+        "Service query contains duplicate variable definitions"
+    }
+
+    val referencedVariableNames = linkedSetOf<String>()
+    NodeTraverser().preOrder(
+        object : NodeVisitorStub() {
+            override fun visitVariableReference(
+                node: VariableReference,
+                context: TraverserContext<Node<*>>,
+            ): TraversalControl {
+                referencedVariableNames += node.name
+                return TraversalControl.CONTINUE
+            }
+        },
+        sortedDocument,
+    )
+
+    val definedVariableNameSet = definedVariableNames.toSet()
+    require(referencedVariableNames == definedVariableNameSet) {
+        "Service query variable definitions and references must match"
+    }
+    require(variables.keys == definedVariableNameSet) {
+        "Service query variable definitions and variables map keys must match"
+    }
+
+    return PreparedServiceRequest(
+        document = sortedDocument,
+        variableNames = referencedVariableNames.toList(),
+        variables = variables,
+    )
+}
+
+private fun Document.renameVariables(names: Map<String, String>): Document {
+    return AstTransformer().transform(
+        this,
+        object : NodeVisitorStub() {
+            override fun visitVariableDefinition(
+                node: VariableDefinition,
+                context: TraverserContext<Node<*>>,
+            ): TraversalControl {
+                return changeNode(
+                    context,
+                    node.transform { builder ->
+                        builder.name(names.getValue(node.name))
+                    },
+                )
+            }
+
+            override fun visitVariableReference(
+                node: VariableReference,
+                context: TraverserContext<Node<*>>,
+            ): TraversalControl {
+                return changeNode(
+                    context,
+                    node.transform { builder ->
+                        builder.name(names.getValue(node.name))
+                    },
+                )
+            }
+        },
+    ) as Document
+}