Merge pull request #2867 from graphql-java/double-coercion-fix-cherry…

…-pick

Cherry pick double variable coercion fix

.github/workflows/pull_request.yml

@@ -7,6 +7,7 @@ on:
   pull_request:
     branches:
       - master
+      - 18.x
 jobs:
   buildAndTest:
     runs-on: ubuntu-latest

src/main/java/graphql/analysis/MaxQueryComplexityInstrumentation.java

@@ -147,7 +147,7 @@ QueryTraverser newQueryTraverser(ExecutionContext executionContext) {
                 .schema(executionContext.getGraphQLSchema())
                 .document(executionContext.getDocument())
                 .operationName(executionContext.getExecutionInput().getOperationName())
-                .variables(executionContext.getVariables())
+                .coercedVariables(executionContext.getCoercedVariables())
                 .build();
     }
 

src/main/java/graphql/analysis/MaxQueryDepthInstrumentation.java

@@ -84,7 +84,7 @@ QueryTraverser newQueryTraverser(ExecutionContext executionContext) {
                 .schema(executionContext.getGraphQLSchema())
                 .document(executionContext.getDocument())
                 .operationName(executionContext.getExecutionInput().getOperationName())
-                .variables(executionContext.getVariables())
+                .coercedVariables(executionContext.getCoercedVariables())
                 .build();
     }
 

src/main/java/graphql/analysis/QueryTraverser.java

@@ -51,32 +51,38 @@ public class QueryTraverser {
     private QueryTraverser(GraphQLSchema schema,
                            Document document,
                            String operation,
-                           Map<String, Object> variables) {
-        assertNotNull(document, () -> "document can't be null");
+                           CoercedVariables coercedVariables) {
+        this.schema = schema;
         NodeUtil.GetOperationResult getOperationResult = NodeUtil.getOperation(document, operation);
-        List<VariableDefinition> variableDefinitions = getOperationResult.operationDefinition.getVariableDefinitions();
-        this.schema = assertNotNull(schema, () -> "schema can't be null");
         this.fragmentsByName = getOperationResult.fragmentsByName;
         this.roots = singletonList(getOperationResult.operationDefinition);
         this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition);
-        this.coercedVariables = coerceVariables(assertNotNull(variables, () -> "variables can't be null"), variableDefinitions);
+        this.coercedVariables = coercedVariables;
     }
 
-    private CoercedVariables coerceVariables(Map<String, Object> rawVariables, List<VariableDefinition> variableDefinitions) {
-        return new ValuesResolver().coerceVariableValues(schema, variableDefinitions, new RawVariables(rawVariables));
+    private QueryTraverser(GraphQLSchema schema,
+                           Document document,
+                           String operation,
+                           RawVariables rawVariables) {
+        this.schema = schema;
+        NodeUtil.GetOperationResult getOperationResult = NodeUtil.getOperation(document, operation);
+        List<VariableDefinition> variableDefinitions = getOperationResult.operationDefinition.getVariableDefinitions();
+        this.fragmentsByName = getOperationResult.fragmentsByName;
+        this.roots = singletonList(getOperationResult.operationDefinition);
+        this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition);
+        this.coercedVariables = new ValuesResolver().coerceVariableValues(schema, variableDefinitions, rawVariables);
     }
 
     private QueryTraverser(GraphQLSchema schema,
                            Node root,
                            GraphQLCompositeType rootParentType,
                            Map<String, FragmentDefinition> fragmentsByName,
-                           Map<String, Object> variables) {
-        this.schema = assertNotNull(schema, () -> "schema can't be null");
-        assertNotNull(root, () -> "root can't be null");
+                           CoercedVariables coercedVariables) {
+        this.schema = schema;
         this.roots = Collections.singleton(root);
-        this.rootParentType = assertNotNull(rootParentType, () -> "rootParentType can't be null");
-        this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null");
-        this.coercedVariables = new CoercedVariables(assertNotNull(variables, () -> "variables can't be null"));
+        this.rootParentType = rootParentType;
+        this.fragmentsByName = fragmentsByName;
+        this.coercedVariables = coercedVariables;
     }
 
     public Object visitDepthFirst(QueryVisitor queryVisitor) {
@@ -196,7 +202,8 @@ public static class Builder {
         private GraphQLSchema schema;
         private Document document;
         private String operation;
-        private Map<String, Object> variables;
+        private CoercedVariables coercedVariables = CoercedVariables.emptyVariables();
+        private RawVariables rawVariables;
 
         private Node root;
         private GraphQLCompositeType rootParentType;
@@ -211,7 +218,7 @@ public static class Builder {
          * @return this builder
          */
         public Builder schema(GraphQLSchema schema) {
-            this.schema = schema;
+            this.schema = assertNotNull(schema, () -> "schema can't be null");
             return this;
         }
 
@@ -237,19 +244,33 @@ public Builder operationName(String operationName) {
          * @return this builder
          */
         public Builder document(Document document) {
-            this.document = document;
+            this.document = assertNotNull(document, () -> "document can't be null");
             return this;
         }
 
         /**
-         * Variables used in the query.
+         * Raw variables used in the query.
          *
          * @param variables the variables to use
          *
          * @return this builder
          */
         public Builder variables(Map<String, Object> variables) {
-            this.variables = variables;
+            assertNotNull(variables, () -> "variables can't be null");
+            this.rawVariables = new RawVariables(variables);
+            return this;
+        }
+
+        /**
+         * Variables (already coerced) used in the query.
+         *
+         * @param coercedVariables the variables to use
+         *
+         * @return this builder
+         */
+        public Builder coercedVariables(CoercedVariables coercedVariables) {
+            assertNotNull(coercedVariables, () -> "coercedVariables can't be null");
+            this.coercedVariables = coercedVariables;
             return this;
         }
 
@@ -262,7 +283,7 @@ public Builder variables(Map<String, Object> variables) {
          * @return this builder
          */
         public Builder root(Node root) {
-            this.root = root;
+            this.root = assertNotNull(root, () -> "root can't be null");
             return this;
         }
 
@@ -274,7 +295,7 @@ public Builder root(Node root) {
          * @return this builder
          */
         public Builder rootParentType(GraphQLCompositeType rootParentType) {
-            this.rootParentType = rootParentType;
+            this.rootParentType = assertNotNull(rootParentType, () -> "rootParentType can't be null");
             return this;
         }
 
@@ -286,7 +307,7 @@ public Builder rootParentType(GraphQLCompositeType rootParentType) {
          * @return this builder
          */
         public Builder fragmentsByName(Map<String, FragmentDefinition> fragmentsByName) {
-            this.fragmentsByName = fragmentsByName;
+            this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null");
             return this;
         }
 
@@ -296,9 +317,18 @@ public Builder fragmentsByName(Map<String, FragmentDefinition> fragmentsByName)
         public QueryTraverser build() {
             checkState();
             if (document != null) {
-                return new QueryTraverser(schema, document, operation, variables);
+                if (rawVariables != null) {
+                    return new QueryTraverser(schema, document, operation, rawVariables);
+                }
+                return new QueryTraverser(schema, document, operation, coercedVariables);
             } else {
-                return new QueryTraverser(schema, root, rootParentType, fragmentsByName, variables);
+                if (rawVariables != null) {
+                    // When traversing with an arbitrary root, there is no variable definition context available
+                    // Thus, the variables must have already been coerced
+                    // Retaining this builder for backwards compatibility
+                    return new QueryTraverser(schema, root, rootParentType, fragmentsByName, new CoercedVariables(rawVariables.toMap()));
+                }
+                return new QueryTraverser(schema, root, rootParentType, fragmentsByName, coercedVariables);
             }
         }
 

src/main/java/graphql/execution/instrumentation/fieldvalidation/FieldValidationSupport.java

@@ -33,7 +33,7 @@ static List<GraphQLError> validateFieldsAndArguments(FieldValidation fieldValida
                 .schema(executionContext.getGraphQLSchema())
                 .document(executionContext.getDocument())
                 .operationName(executionContext.getOperationDefinition().getName())
-                .variables(executionContext.getVariables())
+                .coercedVariables(executionContext.getCoercedVariables())
                 .build();
 
         queryTraverser.visitPreOrder(new QueryVisitorStub() {

src/test/groovy/graphql/analysis/QueryTraverserTest.groovy

@@ -1,6 +1,8 @@
 package graphql.analysis
 
+import graphql.AssertException
 import graphql.TestUtil
+import graphql.execution.CoercedVariables
 import graphql.language.ArrayValue
 import graphql.language.Document
 import graphql.language.Field
@@ -428,7 +430,7 @@ class QueryTraverserTest extends Specification {
     }
 
 
-    def "test preOrder and postOrder order for fragment definitions"() {
+    def "test preOrder and postOrder order for fragment definitions and raw variables"() {
         given:
         def schema = TestUtil.schema("""
             type Query{
@@ -475,6 +477,53 @@ class QueryTraverserTest extends Specification {
         1 * visitor.visitFragmentDefinition({ QueryVisitorFragmentDefinitionEnvironment env -> env.fragmentDefinition == fragments["F1"] })
     }
 
+    def "test preOrder and postOrder order for fragment definitions and coerced variables"() {
+        given:
+        def schema = TestUtil.schema("""
+            type Query{
+                foo: Foo
+                bar: String
+            }
+            type Foo {
+                subFoo: String  
+            }
+        """)
+        def visitor = mockQueryVisitor()
+        def query = createQuery("""
+                {
+                    ...F1
+                }
+                
+                fragment F1 on Query {
+                    foo {
+                        subFoo
+                    }
+                }
+                """)
+
+        def fragments = NodeUtil.getFragmentsByName(query)
+
+        QueryTraverser queryTraversal = QueryTraverser.newQueryTraverser()
+                .schema(schema)
+                .root(fragments["F1"])
+                .rootParentType(schema.getQueryType())
+                .fragmentsByName(fragments)
+                .coercedVariables(CoercedVariables.emptyVariables())
+                .build()
+
+        when:
+        queryTraversal.visitPreOrder(visitor)
+
+        then:
+        1 * visitor.visitFragmentDefinition({ QueryVisitorFragmentDefinitionEnvironment env -> env.fragmentDefinition == fragments["F1"] })
+
+        when:
+        queryTraversal.visitPostOrder(visitor)
+
+        then:
+        1 * visitor.visitFragmentDefinition({ QueryVisitorFragmentDefinitionEnvironment env -> env.fragmentDefinition == fragments["F1"] })
+    }
+
     def "works for mutations()"() {
         given:
         def schema = TestUtil.schema("""
@@ -1378,7 +1427,9 @@ class QueryTraverserTest extends Specification {
     }
 
 
-    def "can select an arbitrary root node"() {
+    def "can select an arbitrary root node with coerced variables as plain map"() {
+        // When using an arbitrary root node, there is no variable definition context available.
+        // Thus the variables must have already been coerced, but may appear as a plain map rather than CoercedVariables
         given:
         def schema = TestUtil.schema("""
             type Query{
@@ -1418,9 +1469,48 @@ class QueryTraverserTest extends Specification {
 
     }
 
+    def "can select an arbitrary root node with coerced variables"() {
+        given:
+        def schema = TestUtil.schema("""
+            type Query{
+                foo: Foo
+            }
+            type Foo {
+                subFoo: SubFoo
+            }
+            type SubFoo {
+               id: String 
+            }
+        """)
+        def visitor = mockQueryVisitor()
+        def query = createQuery("""
+            {foo { subFoo {id}} }
+            """)
+        def subFooAsRoot = query.children[0].children[0].children[0].children[0].children[0]
+        assert subFooAsRoot instanceof Field
+        ((Field) subFooAsRoot).name == "subFoo"
+        def rootParentType = schema.getType("Foo")
+        QueryTraverser queryTraversal = QueryTraverser.newQueryTraverser()
+                .schema(schema)
+                .root(subFooAsRoot)
+                .rootParentType(rootParentType)
+                .coercedVariables(CoercedVariables.emptyVariables())
+                .fragmentsByName(emptyMap())
+                .build()
+        when:
+        queryTraversal.visitPreOrder(visitor)
+
+        then:
+        1 * visitor.visitField({ QueryVisitorFieldEnvironmentImpl it ->
+            it.field.name == "subFoo" && it.fieldDefinition.type.name == "SubFoo"
+        })
+        then:
+        1 * visitor.visitField({ QueryVisitorFieldEnvironmentImpl it -> it.field.name == "id" && it.fieldDefinition.type.name == "String" && it.parentType.name == "SubFoo" })
+
+    }
 
     @Unroll
-    def "builder doesn't allow ambiguous arguments"() {
+    def "builder doesn't allow null arguments"() {
         when:
         QueryTraverser.newQueryTraverser()
                 .document(document)
@@ -1431,7 +1521,7 @@ class QueryTraverserTest extends Specification {
                 .build()
 
         then:
-        thrown(IllegalStateException)
+        thrown(AssertException)
 
         where:
         document             | operationName | root                     | rootParentType          | fragmentsByName
@@ -1446,11 +1536,24 @@ class QueryTraverserTest extends Specification {
         null                 | "foo"         | null                     | Mock(GraphQLObjectType) | emptyMap()
         null                 | "foo"         | null                     | Mock(GraphQLObjectType) | null
         null                 | "foo"         | null                     | null                    | emptyMap()
+    }
 
+    @Unroll
+    def "builder doesn't allow ambiguous arguments"() {
+        when:
+        QueryTraverser.newQueryTraverser()
+                .document(createQuery("{foo}"))
+                .operationName("foo")
+                .root(Field.newField().build())
+                .rootParentType(Mock(GraphQLObjectType))
+                .fragmentsByName(emptyMap())
+                .build()
 
+        then:
+        thrown(IllegalStateException)
     }
 
-    def "typename special field doens't have a fields container and throws exception"() {
+    def "typename special field doesn't have a fields container and throws exception"() {
         given:
         def schema = TestUtil.schema("""
             type Query{
@@ -1585,7 +1688,7 @@ class QueryTraverserTest extends Specification {
 
     }
 
-    def "test accumulate  is returned"() {
+    def "test accumulate is returned"() {
         given:
         def schema = TestUtil.schema("""
             type Query{
@@ -1711,7 +1814,7 @@ class QueryTraverserTest extends Specification {
 
     }
 
-    def     "can copy with Scalar ObjectField visits"() {
+    def "can copy with Scalar ObjectField visits"() {
         given:
         def schema = TestUtil.schema('''
             scalar JSON