fix multi-query variable serialization

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Unreleased
 
+## Multi-query variable references
+
+`GraphQLMultiQueryRequest` now preserves variable references and declarations, including projection variables, fixing #860 and #669.
+Shared variables with compatible types use the strictest nullability.
+Single requests apply the same variable merging in regular and compact output.
+Reusing a variable name with incompatible types or conflicting defaults now produces a server validation error instead of silently dropping root variable values.
+Multi-query requests now preserve each request's operation field directives and explicit selection set.
+
 ## Operation roots (breaking change)
 
 Operation discovery now follows explicit `schema` declarations and `extend schema` declarations.

graphql-dgs-codegen-core/src/integTest/java/com/netflix/graphql/dgs/codegen/java/testcases/projections/projectionWithTypeAndArgs/test/ProjectionWithTypeAndArgsQueryTest.java

@@ -22,9 +22,17 @@
 import com.netflix.graphql.dgs.client.codegen.GraphQLQueryRequest;
 import com.netflix.graphql.dgs.codegen.java.testcases.projections.projectionWithTypeAndArgs.expected.client.PersonGraphQLQuery;
 import com.netflix.graphql.dgs.codegen.java.testcases.projections.projectionWithTypeAndArgs.expected.client.PersonProjectionRoot;
+import graphql.parser.Parser;
+import graphql.schema.idl.RuntimeWiring;
+import graphql.schema.idl.SchemaGenerator;
+import graphql.schema.idl.SchemaParser;
+import graphql.validation.Validator;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import java.util.List;
+import java.util.Locale;
 import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -75,6 +83,95 @@ void testQueryWithUnnamedArgs() {
                         }""");
     }
 
+    @Test
+    void testMultipleQueriesWithVariableReferences() {
+        var query1 = PersonGraphQLQuery.newRequest()
+                .queryName("Combined")
+                .a1("literal")
+                .a2Reference("firstName")
+                .build();
+        query1.setQueryAlias("first");
+
+        var query2 = PersonGraphQLQuery.newRequest()
+                .a2Reference("secondName")
+                .build();
+        query2.setQueryAlias("second");
+
+        var projection = new PersonProjectionRoot<>().firstname();
+        var request = new GraphQLMultiQueryRequest(List.of(
+                new GraphQLQueryRequest(query1, projection),
+                new GraphQLQueryRequest(query2, projection)
+        ));
+
+        assertThat(request.serialize()).isEqualTo("""
+                query Combined($firstName: String!, $secondName: String!) {
+                  first: person(a1: "literal", a2: $firstName) {
+                    firstname
+                  }
+                  second: person(a2: $secondName) {
+                    firstname
+                  }
+                }""");
+    }
+
+    @Test
+    void testMultipleQueriesWithSharedVariableReferencesValidate() {
+        var query1 = PersonGraphQLQuery.newRequest()
+                .a1Reference("shared")
+                .a2("literal")
+                .build();
+        query1.setQueryAlias("first");
+
+        var query2 = PersonGraphQLQuery.newRequest()
+                .a1Reference("shared")
+                .a2("literal")
+                .build();
+        query2.setQueryAlias("second");
+
+        var projection = new PersonProjectionRoot<>().firstname();
+        var request = new GraphQLMultiQueryRequest(List.of(
+                new GraphQLQueryRequest(query1, projection),
+                new GraphQLQueryRequest(query2, projection)
+        ));
+        var schema = new SchemaGenerator().makeExecutableSchema(
+                new SchemaParser().parse("""
+                        type Query { person(a1: String, a2: String!): Person }
+                        type Person { firstname: String }
+                        """),
+                RuntimeWiring.newRuntimeWiring().build()
+        );
+
+        assertThat(new Validator().validateDocument(schema, new Parser().parseDocument(request.serialize()), Locale.ROOT))
+                .isEmpty();
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {false, true})
+    void testSharedVariableWithNullableAndNonNullArguments(boolean reverseOrder) {
+        var nullable = PersonGraphQLQuery.newRequest().a1Reference("shared").a2("literal").build();
+        nullable.setQueryAlias("nullable");
+        var nonNull = PersonGraphQLQuery.newRequest().a2Reference("shared").build();
+        nonNull.setQueryAlias("nonNull");
+        var projection = new PersonProjectionRoot<>().firstname();
+        var nullableRequest = new GraphQLQueryRequest(nullable, projection);
+        var nonNullRequest = new GraphQLQueryRequest(nonNull, projection);
+        var requests = reverseOrder
+                ? List.of(nonNullRequest, nullableRequest)
+                : List.of(nullableRequest, nonNullRequest);
+        var serialized = new GraphQLMultiQueryRequest(requests).serialize();
+        var schema = new SchemaGenerator().makeExecutableSchema(
+                new SchemaParser().parse("""
+                        type Query { person(a1: String, a2: String!): Person }
+                        type Person { firstname: String }
+                        """),
+                RuntimeWiring.newRuntimeWiring().build()
+        );
+
+        assertThat(new Validator().validateDocument(schema, new Parser().parseDocument(serialized), Locale.ROOT))
+                .isEmpty();
+        assertThat(serialized).contains("query ($shared: String!)");
+    }
+
     @Test
     void testQueryWithAlias() {
         var query1 = PersonGraphQLQuery.newRequest()

graphql-dgs-codegen-shared-core/src/main/kotlin/com/netflix/graphql/dgs/client/codegen/GraphQLMultiQueryRequest.kt

@@ -32,48 +32,23 @@ class GraphQLMultiQueryRequest(
 
         val queryType = requests[0].query.getOperationType().toString()
         val variableDefinitions = mutableListOf<VariableDefinition>()
-        val selectionList: MutableList<Field.Builder> = mutableListOf()
+        val selectionList = mutableListOf<Field>()
 
         for (request in this.requests) {
             val query = request.query
+            variableDefinitions.addAll(query.variableDefinitions)
             // Graphql only supports multiple mutations or multiple queries. Not a combination of the two.
             // Graphql does not support multiple subscriptions in one request http://spec.graphql.org/June2018/#sec-Single-root-field
             if (!query.getOperationType().equals(queryType) || queryType == OperationDefinition.Operation.SUBSCRIPTION.name) {
                 throw AssertionError("Request has to have exclusively queries or mutations in a multi operation request")
             }
 
-            if (request.query.variableDefinitions.isNotEmpty()) {
-                variableDefinitions.addAll(request.query.variableDefinitions)
-            }
-
-            val selection = Field.newField(request.query.getOperationName())
-            if (query.input.isNotEmpty()) {
-                selection.arguments(
-                    query.input.map { (name, value) ->
-                        Argument(name, request.inputValueSerializer.toValue(value))
-                    },
-                )
-            }
-
-            if (request.projection != null) {
-                val selectionSet =
-                    if (request.projection is BaseSubProjectionNode<*, *> && request.projection.root() != null) {
-                        request.projectionSerializer.toSelectionSet(request.projection.root() as BaseProjectionNode)
-                    } else {
-                        request.projectionSerializer.toSelectionSet(request.projection)
-                    }
-                if (selectionSet.selections.isNotEmpty()) {
-                    selection.selectionSet(selectionSet)
-                }
-            }
-            if (query.queryAlias.isNotEmpty()) {
-                selection.alias(query.queryAlias)
-            }
-
-            selectionList.add(selection)
+            val selection = request.toField(variableDefinitions)
+            selectionList.add(if (query.queryAlias.isNotEmpty()) selection.transform { it.alias(query.queryAlias) } else selection)
         }
 
-        operationDef.selectionSet(SelectionSet.newSelectionSet(selectionList.map(Field.Builder::build).toList()).build())
+        operationDef.variableDefinitions(mergeVariableDefinitions(variableDefinitions))
+        operationDef.selectionSet(SelectionSet.newSelectionSet(selectionList).build())
 
         return AstPrinter.printAst(operationDef.build())
     }

graphql-dgs-codegen-shared-core/src/main/kotlin/com/netflix/graphql/dgs/client/codegen/GraphQLQueryRequest.kt

@@ -21,8 +21,13 @@ import graphql.language.Argument
 import graphql.language.AstPrinter
 import graphql.language.Directive
 import graphql.language.Field
+import graphql.language.ListType
+import graphql.language.NonNullType
 import graphql.language.OperationDefinition
 import graphql.language.SelectionSet
+import graphql.language.Type
+import graphql.language.TypeName
+import graphql.language.VariableDefinition
 import graphql.language.VariableReference
 import graphql.schema.Coercing
 
@@ -89,12 +94,28 @@ class GraphQLQueryRequest
 
         private fun serialize(compact: Boolean): String {
             val operationDef = OperationDefinition.newOperationDefinition()
+            val variableDefinitions = query.variableDefinitions.toMutableList()
 
             query.name?.let { operationDef.name(it) }
             query
                 .getOperationType()
                 ?.let { operationDef.operation(OperationDefinition.Operation.valueOf(it.uppercase())) }
 
+            val selection = toField(variableDefinitions)
+            if (variableDefinitions.isNotEmpty()) {
+                operationDef.variableDefinitions(mergeVariableDefinitions(variableDefinitions))
+            }
+
+            operationDef.selectionSet(SelectionSet.newSelectionSet().selection(selection).build())
+
+            return if (compact) {
+                AstPrinter.printAstCompact(operationDef.build())
+            } else {
+                AstPrinter.printAst(operationDef.build())
+            }
+        }
+
+        internal fun toField(variableDefinitions: MutableList<VariableDefinition>): Field {
             val selection = Field.newField(query.getOperationName())
             if (query.input.isNotEmpty()) {
                 selection.arguments(
@@ -113,29 +134,62 @@ class GraphQLQueryRequest
             if (projection != null) {
                 val selectionSetFromProjection =
                     if (projection is BaseSubProjectionNode<*, *> && projection.root() != null) {
-                        projectionSerializer.toSelectionSet(projection.root() as BaseProjectionNode)
+                        projectionSerializer.toSelectionSet(projection.root() as BaseProjectionNode, variableDefinitions)
                     } else {
-                        projectionSerializer.toSelectionSet(projection)
+                        projectionSerializer.toSelectionSet(projection, variableDefinitions)
                     }
                 if (selectionSetFromProjection.selections.isNotEmpty()) {
                     selection.selectionSet(selectionSetFromProjection)
                 }
             }
 
-            if (query.variableDefinitions.isNotEmpty()) {
-                operationDef.variableDefinitions(query.variableDefinitions)
-            }
-
             if (selectionSet != null) {
                 selection.selectionSet(selectionSet)
             }
 
-            operationDef.selectionSet(SelectionSet.newSelectionSet().selection(selection.build()).build())
+            return selection.build()
+        }
+    }
+
+@JvmSynthetic
+internal fun mergeVariableDefinitions(definitions: List<VariableDefinition>): List<VariableDefinition> =
+    definitions
+        .groupBy { definition ->
+            Triple(
+                definition.name,
+                definition.defaultValue?.let { AstPrinter.printAstCompact(it) },
+                definition.directives.map { AstPrinter.printAstCompact(it) },
+            )
+        }.values
+        .flatMap { group ->
+            var type: Type<*> = group.first().type
+            for (definition in group.drop(1)) {
+                // Conflicting types or list shapes must remain visible to server validation.
+                type = mergeVariableTypes(type, definition.type) ?: return@flatMap group.distinctBy { AstPrinter.printAstCompact(it) }
+            }
+            listOf(group.first().transform { it.type(type) })
+        }
+
+private fun mergeVariableTypes(
+    first: Type<*>,
+    second: Type<*>,
+): Type<*>? {
+    val firstType = if (first is NonNullType) first.type else first
+    val secondType = if (second is NonNullType) second.type else second
+    val merged =
+        when {
+            firstType is TypeName && secondType is TypeName && firstType.name == secondType.name -> {
+                firstType
+            }
 
-            return if (compact) {
-                AstPrinter.printAstCompact(operationDef.build())
-            } else {
-                AstPrinter.printAst(operationDef.build())
+            firstType is ListType && secondType is ListType -> {
+                ListType(mergeVariableTypes(firstType.type, secondType.type) ?: return null)
+            }
+
+            else -> {
+                return null
             }
         }
-    }
+    // A non-null variable can also satisfy a nullable argument, including within lists.
+    return if (first is NonNullType || second is NonNullType) NonNullType(merged) else merged
+}

graphql-dgs-codegen-shared-core/src/main/kotlin/com/netflix/graphql/dgs/client/codegen/ProjectionSerializer.kt

@@ -29,7 +29,12 @@ class ProjectionSerializer(
     private val inputValueSerializer: InputValueSerializerInterface,
     private val query: GraphQLQuery,
 ) {
-    fun toSelectionSet(projection: BaseProjectionNode): SelectionSet {
+    fun toSelectionSet(projection: BaseProjectionNode): SelectionSet = toSelectionSet(projection, query.variableDefinitions)
+
+    internal fun toSelectionSet(
+        projection: BaseProjectionNode,
+        variableDefinitions: MutableList<VariableDefinition>,
+    ): SelectionSet {
         val selectionSet = SelectionSet.newSelectionSet()
 
         for ((fieldName, value) in projection.fields) {
@@ -40,15 +45,15 @@ class ProjectionSerializer(
                     .arguments(
                         projection.inputArguments[fieldName].orEmpty().map { (argName, values, isReference, type) ->
                             if (isReference) {
-                                query.variableDefinitions.add(VariableDefinition(values as String, type))
+                                variableDefinitions.add(VariableDefinition(values as String, type))
                                 Argument(argName, VariableReference(values))
                             } else {
                                 Argument(argName, inputValueSerializer.toValue(values))
                             }
                         },
                     )
             if (value is BaseProjectionNode) {
-                val fieldSelectionSet = toSelectionSet(value)
+                val fieldSelectionSet = toSelectionSet(value, variableDefinitions)
                 if (fieldSelectionSet.selections.isNotEmpty()) {
                     fieldSelection.selectionSet(fieldSelectionSet)
                 }
@@ -82,7 +87,7 @@ class ProjectionSerializer(
                 InlineFragment
                     .newInlineFragment()
                     .typeCondition(typeCondition)
-                    .selectionSet(toSelectionSet(fragment))
+                    .selectionSet(toSelectionSet(fragment, variableDefinitions))
                     .build(),
             )
         }