diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 13a366a930..2e383fa619 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -7,6 +7,7 @@ on: pull_request: branches: - master + - 18.x jobs: buildAndTest: runs-on: ubuntu-latest diff --git a/src/main/java/graphql/analysis/MaxQueryComplexityInstrumentation.java b/src/main/java/graphql/analysis/MaxQueryComplexityInstrumentation.java index 66523d70e8..a24191fa4d 100644 --- a/src/main/java/graphql/analysis/MaxQueryComplexityInstrumentation.java +++ b/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(); } diff --git a/src/main/java/graphql/analysis/MaxQueryDepthInstrumentation.java b/src/main/java/graphql/analysis/MaxQueryDepthInstrumentation.java index 991a830a64..3a79f9ee0b 100644 --- a/src/main/java/graphql/analysis/MaxQueryDepthInstrumentation.java +++ b/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(); } diff --git a/src/main/java/graphql/analysis/QueryTraverser.java b/src/main/java/graphql/analysis/QueryTraverser.java index 7b5ff7735e..d26c9f9016 100644 --- a/src/main/java/graphql/analysis/QueryTraverser.java +++ b/src/main/java/graphql/analysis/QueryTraverser.java @@ -51,32 +51,38 @@ public class QueryTraverser { private QueryTraverser(GraphQLSchema schema, Document document, String operation, - Map variables) { - assertNotNull(document, () -> "document can't be null"); + CoercedVariables coercedVariables) { + this.schema = schema; NodeUtil.GetOperationResult getOperationResult = NodeUtil.getOperation(document, operation); - List 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 rawVariables, List 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 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 fragmentsByName, - Map 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 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 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 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 fragmentsByName) { - this.fragmentsByName = fragmentsByName; + this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null"); return this; } @@ -296,9 +317,18 @@ public Builder fragmentsByName(Map 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); } } diff --git a/src/main/java/graphql/execution/instrumentation/fieldvalidation/FieldValidationSupport.java b/src/main/java/graphql/execution/instrumentation/fieldvalidation/FieldValidationSupport.java index a3cdac22d4..888a012cc4 100644 --- a/src/main/java/graphql/execution/instrumentation/fieldvalidation/FieldValidationSupport.java +++ b/src/main/java/graphql/execution/instrumentation/fieldvalidation/FieldValidationSupport.java @@ -33,7 +33,7 @@ static List validateFieldsAndArguments(FieldValidation fieldValida .schema(executionContext.getGraphQLSchema()) .document(executionContext.getDocument()) .operationName(executionContext.getOperationDefinition().getName()) - .variables(executionContext.getVariables()) + .coercedVariables(executionContext.getCoercedVariables()) .build(); queryTraverser.visitPreOrder(new QueryVisitorStub() { diff --git a/src/test/groovy/graphql/analysis/QueryTraverserTest.groovy b/src/test/groovy/graphql/analysis/QueryTraverserTest.groovy index ab1b978d81..03a40919ca 100644 --- a/src/test/groovy/graphql/analysis/QueryTraverserTest.groovy +++ b/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 diff --git a/src/test/groovy/graphql/schema/CoercingTest.groovy b/src/test/groovy/graphql/schema/CoercingTest.groovy index 30c2c4ebe4..96c2168d89 100644 --- a/src/test/groovy/graphql/schema/CoercingTest.groovy +++ b/src/test/groovy/graphql/schema/CoercingTest.groovy @@ -2,6 +2,7 @@ package graphql.schema import graphql.ExecutionInput import graphql.TestUtil +import graphql.analysis.MaxQueryDepthInstrumentation import graphql.language.ArrayValue import graphql.language.BooleanValue import graphql.language.FloatValue @@ -14,6 +15,8 @@ import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.TypeRuntimeWiring import spock.lang.Specification +import java.time.ZonedDateTime + import static graphql.schema.GraphQLScalarType.newScalar class CoercingTest extends Specification { @@ -72,7 +75,6 @@ class CoercingTest extends Specification { }) .build() - def "end to end test of coercing with variables references"() { def spec = ''' @@ -214,4 +216,79 @@ class CoercingTest extends Specification { er.errors[0].message.contains("serialize message") er.errors[0].extensions == [serialize: true] } + + static def parseDateTimeValue(Object input) { + try { + if (input instanceof StringValue) { + return ZonedDateTime.parse(((StringValue) input).getValue()) + } else if (input instanceof String) { + return ZonedDateTime.parse((String) input) + } else throw new IllegalArgumentException("Unexpected input type: ${input.getClass()}") + } catch (Exception e) { + throw new CoercingParseValueException("Failed to parse input value $input as ZonedDateTime", e) + } + } + + GraphQLScalarType zonedDateTimeLikeScalar = newScalar().name("ZonedDateTimeLike").description("ZonedDateTimeLike").coercing(new Coercing() { + @Override + Object serialize(Object dataFetcherResult) throws CoercingSerializeException { + return dataFetcherResult + } + + @Override + Object parseValue(Object input) throws CoercingParseValueException { + return parseDateTimeValue(input) + } + + @Override + Object parseLiteral(Object input) throws CoercingParseLiteralException { + return input + } + }) + .build() + + def "custom scalars are only coerced once - end to end test with execution and instrumentation"() { + def spec = ''' + scalar ZonedDateTimeLike + + type Query { + field(argument : ZonedDateTimeLike) : ZonedDateTimeLike + } + ''' + DataFetcher df = new DataFetcher() { + @Override + Object get(DataFetchingEnvironment environment) { + def argument = environment.getArgument("argument") + return argument + } + } + + def runtimeWiring = RuntimeWiring.newRuntimeWiring() + .type(TypeRuntimeWiring.newTypeWiring("Query") + .dataFetcher("field", df)) + .scalar(zonedDateTimeLikeScalar) + .build() + + def executionInput = ExecutionInput.newExecutionInput() + .variables([zonedDateTime: "2022-05-16T19:52:37Z"]) + .query(''' + query myZonedDateTimeQuery($zonedDateTime : ZonedDateTimeLike) { + field(argument : $zonedDateTime) + }''') + .build() + + MaxQueryDepthInstrumentation maximumQueryDepthInstrumentation = new MaxQueryDepthInstrumentation(7) + + def graphQL = TestUtil.graphQL(spec, runtimeWiring).instrumentation(maximumQueryDepthInstrumentation).build() + + when: + def er = graphQL.execute(executionInput) + + then: + // If variables were coerced twice, would expect an IllegalArgumentException to be thrown, + // as the ZonedDateTime custom scalar parser can only parse strings, not instances of ZonedDateTime + notThrown(Exception) + er.errors.isEmpty() + er.data == [field: ZonedDateTime.parse("2022-05-16T19:52:37Z")] + } }