From 35524b13d1097b1761fa673efd8f86c81f2b05df Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 23 May 2017 08:22:55 +1000 Subject: [PATCH 01/10] #427 added null support but have not added optional arguments yet --- src/main/antlr/Graphql.g4 | 4 ++ .../graphql/execution/ValuesResolver.java | 3 ++ .../java/graphql/language/AstPrinter.java | 3 ++ src/main/java/graphql/language/NullValue.java | 33 +++++++++++++ .../parser/GraphqlAntlrToLanguage.java | 9 ++++ .../graphql/schema/idl/SchemaGenerator.java | 3 ++ .../graphql/validation/ValidationUtil.java | 3 ++ .../execution/ValuesResolverTest.groovy | 22 +++++++++ .../graphql/language/AstPrinterTest.groovy | 49 +++++++++++++++++++ .../validation/ValidationUtilTest.groovy | 10 ++++ 10 files changed, 139 insertions(+) create mode 100644 src/main/java/graphql/language/NullValue.java diff --git a/src/main/antlr/Graphql.g4 b/src/main/antlr/Graphql.g4 index d7a3b649f9..51b6be8e07 100644 --- a/src/main/antlr/Graphql.g4 +++ b/src/main/antlr/Graphql.g4 @@ -66,6 +66,7 @@ IntValue | FloatValue | StringValue | BooleanValue | +NullValue | enumValue | arrayValue | objectValue; @@ -76,6 +77,7 @@ IntValue | FloatValue | StringValue | BooleanValue | +NullValue | enumValue | arrayValueWithVariable | objectValueWithVariable; @@ -176,6 +178,8 @@ directiveLocations '|' directiveLocation BooleanValue: 'true' | 'false'; +NullValue: 'null'; + FRAGMENT: 'fragment'; QUERY: 'query'; MUTATION: 'mutation'; diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index f03dabf381..e73d53364d 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -129,6 +129,9 @@ private Object coerceValueAst(GraphQLType type, Value inputValue, Map getChildren() { + return Collections.emptyList(); + } + + @Override + public boolean isEqualTo(Node o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + return true; + + } + + @Override + public String toString() { + return "NullValue{" + + '}'; + } +} diff --git a/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java b/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java index 37f49c196a..b9c4e07361 100644 --- a/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java +++ b/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java @@ -58,7 +58,10 @@ import java.util.Deque; import java.util.List; +import static graphql.language.NullValue.Null; + @Internal + public class GraphqlAntlrToLanguage extends GraphqlBaseVisitor { private final CommonTokenStream tokens; @@ -668,6 +671,9 @@ private Value getValue(GraphqlParser.ValueWithVariableContext ctx) { BooleanValue booleanValue = new BooleanValue(Boolean.parseBoolean(ctx.BooleanValue().getText())); newNode(booleanValue, ctx); return booleanValue; + } else if (ctx.NullValue() != null) { + newNode(Null, ctx); + return Null; } else if (ctx.StringValue() != null) { StringValue stringValue = new StringValue(parseString(ctx.StringValue().getText())); newNode(stringValue, ctx); @@ -713,6 +719,9 @@ private Value getValue(GraphqlParser.ValueContext ctx) { BooleanValue booleanValue = new BooleanValue(Boolean.parseBoolean(ctx.BooleanValue().getText())); newNode(booleanValue, ctx); return booleanValue; + } else if (ctx.NullValue() != null) { + newNode(Null, ctx); + return Null; } else if (ctx.StringValue() != null) { StringValue stringValue = new StringValue(parseString(ctx.StringValue().getText())); newNode(stringValue, ctx); diff --git a/src/main/java/graphql/schema/idl/SchemaGenerator.java b/src/main/java/graphql/schema/idl/SchemaGenerator.java index fd1c805bae..de15157c36 100644 --- a/src/main/java/graphql/schema/idl/SchemaGenerator.java +++ b/src/main/java/graphql/schema/idl/SchemaGenerator.java @@ -13,6 +13,7 @@ import graphql.language.IntValue; import graphql.language.InterfaceTypeDefinition; import graphql.language.Node; +import graphql.language.NullValue; import graphql.language.ObjectTypeDefinition; import graphql.language.ObjectValue; import graphql.language.OperationTypeDefinition; @@ -462,6 +463,8 @@ private Object buildValue(Value value) { result = arrayValue.getValues().stream().map(this::buildValue).toArray(); } else if (value instanceof ObjectValue) { result = buildObjectValue((ObjectValue) value); + } else if (value instanceof NullValue) { + result = null; } return result; diff --git a/src/main/java/graphql/validation/ValidationUtil.java b/src/main/java/graphql/validation/ValidationUtil.java index e75928fcbc..2ef7f448a8 100644 --- a/src/main/java/graphql/validation/ValidationUtil.java +++ b/src/main/java/graphql/validation/ValidationUtil.java @@ -25,6 +25,9 @@ public boolean isValidLiteralValue(Value value, GraphQLType type) { if (value == null) { return !(type instanceof GraphQLNonNull); } + if (value instanceof NullValue) { + return !(type instanceof GraphQLNonNull); + } if (value instanceof VariableReference) { return true; } diff --git a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy index 30b4fb1da8..63cbb7fdbe 100644 --- a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy +++ b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy @@ -383,4 +383,26 @@ class ValuesResolverTest extends Specification { [intKey: 10] | _ [intKey: 10, requiredField: null] | _ } + + def "getVariableValues: simple types with values not provided in variables map"() { + given: + + def schema = TestUtil.schemaWithInputType(GraphQLString) + VariableDefinition fooVarDef = new VariableDefinition("foo", new TypeName("String")) + VariableDefinition barVarDef = new VariableDefinition("bar", new TypeName("String")) + + when: + def resolvedValues = resolver.getVariableValues(schema, [fooVarDef, barVarDef], InputValue) + + then: + resolvedValues == outputValue + + where: + InputValue || outputValue + [foo: "added", bar: null] || [foo: "added", bar: null] + + // later this will be true once we apply missing value code + //[foo: "added"] || [foo: "added"] + } + } diff --git a/src/test/groovy/graphql/language/AstPrinterTest.groovy b/src/test/groovy/graphql/language/AstPrinterTest.groovy index d7344728f4..482218ce49 100644 --- a/src/test/groovy/graphql/language/AstPrinterTest.groovy +++ b/src/test/groovy/graphql/language/AstPrinterTest.groovy @@ -323,4 +323,53 @@ query HeroNameAndFriends($episode: Episode = "JEDI") { ''' } +//------------------------------------------------- + def "ast printing of null"() { + def query = ''' +query NullEpisodeQuery { + hero(episode: null) { + name + } +} +''' + def document = parse(query) + String output = printAst(document) + + expect: + output == '''query NullEpisodeQuery { + hero(episode: null) { + name + } +} +''' + } + + //------------------------------------------------- + def "ast printing of default variables with null"() { + def query = ''' +query NullVariableDefaultValueQuery($episode: Episode = null) { + hero(episode: $episode) { + name + friends { + name + } + } +} +''' + def document = parse(query) + String output = printAst(document) + + expect: + output == '''query NullVariableDefaultValueQuery($episode: Episode = null) { + hero(episode: $episode) { + name + friends { + name + } + } +} +''' + } + + } diff --git a/src/test/groovy/graphql/validation/ValidationUtilTest.groovy b/src/test/groovy/graphql/validation/ValidationUtilTest.groovy index 9e4f908127..3beaf42f40 100644 --- a/src/test/groovy/graphql/validation/ValidationUtilTest.groovy +++ b/src/test/groovy/graphql/validation/ValidationUtilTest.groovy @@ -39,6 +39,11 @@ class ValidationUtilTest extends Specification { !validationUtil.isValidLiteralValue(null, nonNull(GraphQLString)) } + def "NullValue and NonNull is invalid"() { + expect: + !validationUtil.isValidLiteralValue(NullValue.Null, nonNull(GraphQLString)) + } + def "a nonNull value for a NonNull type is valid"() { expect: validationUtil.isValidLiteralValue(new StringValue("string"), nonNull(GraphQLString)) @@ -49,6 +54,11 @@ class ValidationUtilTest extends Specification { validationUtil.isValidLiteralValue(null, GraphQLString) } + def "NullValue is valid when type is NonNull"() { + expect: + validationUtil.isValidLiteralValue(NullValue.Null, GraphQLString) + } + def "variables are valid"() { expect: validationUtil.isValidLiteralValue(new VariableReference("var"), GraphQLBoolean) From c2ad125c70c6a49bbf8c48f77e2f032b2f20a52f Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 23 May 2017 23:50:30 +1000 Subject: [PATCH 02/10] #427 added spec tests and values resolver support --- .../graphql/execution/ExecutionStrategy.java | 4 +- .../graphql/execution/ValuesResolver.java | 87 ++++++-- .../graphql/CapturingDataFetcher.groovy | 19 ++ src/test/groovy/graphql/GraphQLTest.groovy | 12 +- .../graphql/NullValueSupportTest.groovy | 210 ++++++++++++++++++ src/test/groovy/graphql/TestUtil.groovy | 29 +++ 6 files changed, 340 insertions(+), 21 deletions(-) create mode 100644 src/test/groovy/graphql/CapturingDataFetcher.groovy create mode 100644 src/test/groovy/graphql/NullValueSupportTest.groovy diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 9c813b0bc5..f5cd3d0574 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -11,6 +11,7 @@ import graphql.execution.instrumentation.parameters.FieldFetchParameters; import graphql.execution.instrumentation.parameters.FieldParameters; import graphql.language.Field; +import graphql.schema.DataFetcher; import graphql.schema.DataFetchingEnvironment; import graphql.schema.DataFetchingEnvironmentImpl; import graphql.schema.DataFetchingFieldSelectionSet; @@ -96,7 +97,8 @@ protected ExecutionResult resolveField(ExecutionContext executionContext, Execut InstrumentationContext fetchCtx = instrumentation.beginFieldFetch(new FieldFetchParameters(executionContext, fieldDef, environment)); Object resolvedValue = null; try { - resolvedValue = fieldDef.getDataFetcher().get(environment); + DataFetcher dataFetcher = fieldDef.getDataFetcher(); + resolvedValue = dataFetcher.get(environment); fetchCtx.onEnd(resolvedValue); } catch (Exception e) { diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index e73d53364d..beefb89cd6 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -2,18 +2,43 @@ import graphql.GraphQLException; -import graphql.language.*; -import graphql.schema.*; +import graphql.language.Argument; +import graphql.language.ArrayValue; +import graphql.language.NullValue; +import graphql.language.ObjectField; +import graphql.language.ObjectValue; +import graphql.language.Value; +import graphql.language.VariableDefinition; +import graphql.language.VariableReference; +import graphql.schema.GraphQLArgument; +import graphql.schema.GraphQLEnumType; +import graphql.schema.GraphQLInputObjectField; +import graphql.schema.GraphQLInputObjectType; +import graphql.schema.GraphQLList; +import graphql.schema.GraphQLNonNull; +import graphql.schema.GraphQLScalarType; +import graphql.schema.GraphQLSchema; +import graphql.schema.GraphQLType; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; public class ValuesResolver { - public Map getVariableValues(GraphQLSchema schema, List variableDefinitions, Map inputs) { + public Map getVariableValues(GraphQLSchema schema, List variableDefinitions, Map args) { Map result = new LinkedHashMap<>(); for (VariableDefinition variableDefinition : variableDefinitions) { - result.put(variableDefinition.getName(), getVariableValue(schema, variableDefinition, inputs.get(variableDefinition.getName()))); + String varName = variableDefinition.getName(); + // we transfer the variable as field arguments if its present as value + if (args.containsKey(varName)) { + Object arg = args.get(varName); + Object variableValue = getVariableValue(schema, variableDefinition, arg); + result.put(varName, variableValue); + } } return result; } @@ -23,14 +48,19 @@ public Map getArgumentValues(List argumentTypes Map result = new LinkedHashMap<>(); Map argumentMap = argumentMap(arguments); for (GraphQLArgument fieldArgument : argumentTypes) { - Argument argument = argumentMap.get(fieldArgument.getName()); + String argName = fieldArgument.getName(); + Argument argument = argumentMap.get(argName); Object value; if (argument != null) { value = coerceValueAst(fieldArgument.getType(), argument.getValue(), variables); } else { value = fieldArgument.getDefaultValue(); } - result.put(fieldArgument.getName(), value); + // only put an arg into the result IF they specified a variable at all or + // the default value ended up being something non null + if (argumentMap.containsKey(argName) || value != null) { + result.put(argName, value); + } } return result; } @@ -48,6 +78,7 @@ private Map argumentMap(List arguments) { private Object getVariableValue(GraphQLSchema schema, VariableDefinition variableDefinition, Object inputValue) { GraphQLType type = TypeFromAST.getTypeFromAST(schema, variableDefinition.getType()); + //noinspection ConstantConditions if (!isValid(type, inputValue)) { throw new GraphQLException("Invalid value for type"); } @@ -81,6 +112,7 @@ private Object coerceValue(GraphQLType graphQLType, Object value) { } else if (graphQLType instanceof GraphQLList) { return coerceValueForList((GraphQLList) graphQLType, value); } else if (graphQLType instanceof GraphQLInputObjectType && value instanceof Map) { + //noinspection unchecked return coerceValueForInputObjectType((GraphQLInputObjectType) graphQLType, (Map) value); } else if (graphQLType instanceof GraphQLInputObjectType) { return value; @@ -170,22 +202,49 @@ private Object coerceValueAstForInputObject(GraphQLInputObjectType type, ObjectV for (GraphQLInputObjectField inputTypeField : type.getFields()) { if (inputValueFieldsByName.containsKey(inputTypeField.getName())) { + boolean putObjectInMap = true; + ObjectField field = inputValueFieldsByName.get(inputTypeField.getName()); - Object fieldValue = coerceValueAst(inputTypeField.getType(), field.getValue(), variables); - if (fieldValue == null) { - fieldValue = inputTypeField.getDefaultValue(); + Value fieldInputValue = field.getValue(); + + Object fieldObject = null; + if (fieldInputValue instanceof VariableReference) { + String varName = ((VariableReference) fieldInputValue).getName(); + if (!variables.containsKey(varName)) { + putObjectInMap = false; + } else { + fieldObject = variables.get(varName); + } + } else { + fieldObject = coerceValueAst(inputTypeField.getType(), fieldInputValue, variables); + } + + if (fieldObject == null) { + if (!field.getValue().isEqualTo(NullValue.Null)) { + fieldObject = inputTypeField.getDefaultValue(); + } + } + if (putObjectInMap) { + result.put(field.getName(), fieldObject); + } else { + assertNonNullInputField(inputTypeField); } - result.put(field.getName(), fieldValue); } else if (inputTypeField.getDefaultValue() != null) { result.put(inputTypeField.getName(), inputTypeField.getDefaultValue()); - } else if (inputTypeField.getType() instanceof GraphQLNonNull) { - // Possibly overkill; an object literal with a missing non null field shouldn't pass validation - throw new GraphQLException("Null value for NonNull type " + inputTypeField.getType()); + } else { + assertNonNullInputField(inputTypeField); } } return result; } + private void assertNonNullInputField(GraphQLInputObjectField inputTypeField) { + if (inputTypeField.getType() instanceof GraphQLNonNull) { + // Possibly overkill; an object literal with a missing non null field shouldn't pass validation + throw new GraphQLException("Null value for NonNull type " + inputTypeField.getType()); + } + } + private Map mapObjectValueFieldsByName(ObjectValue inputValue) { Map inputValueFieldsByName = new LinkedHashMap<>(); for (ObjectField objectField : inputValue.getObjectFields()) { diff --git a/src/test/groovy/graphql/CapturingDataFetcher.groovy b/src/test/groovy/graphql/CapturingDataFetcher.groovy new file mode 100644 index 0000000000..b6a83885ef --- /dev/null +++ b/src/test/groovy/graphql/CapturingDataFetcher.groovy @@ -0,0 +1,19 @@ +package graphql + +import graphql.schema.DataFetcher +import graphql.schema.DataFetchingEnvironment + +/** + * Help to capture data fetcher arguments passed in + */ +class CapturingDataFetcher implements DataFetcher { + Map args + DataFetchingEnvironment environment + + @Override + Object get(DataFetchingEnvironment environment) { + this.environment = environment + this.args = environment.getArguments() + null + } +} diff --git a/src/test/groovy/graphql/GraphQLTest.groovy b/src/test/groovy/graphql/GraphQLTest.groovy index 9b439ec06a..09bc0a89da 100644 --- a/src/test/groovy/graphql/GraphQLTest.groovy +++ b/src/test/groovy/graphql/GraphQLTest.groovy @@ -157,10 +157,10 @@ class GraphQLTest extends Specification { set.add("One") set.add("Two") - def schema = GraphQLSchema.newSchema() - .query(GraphQLObjectType.newObject() + def schema = newSchema() + .query(newObject() .name("QueryType") - .field(GraphQLFieldDefinition.newFieldDefinition() + .field(newFieldDefinition() .name("set") .type(new GraphQLList(GraphQLString)) .dataFetcher({ set }))) @@ -231,7 +231,7 @@ class GraphQLTest extends Specification { .build() when: - def result = new GraphQL(schema).execute("mutation { doesNotExist }"); + def result = new GraphQL(schema).execute("mutation { doesNotExist }") then: result.errors.size() == 1 @@ -373,12 +373,12 @@ class GraphQLTest extends Specification { .build() def query = "{foo}" when: - def result = GraphQL.newGraphQL(schema).build().execute(query) + GraphQL.newGraphQL(schema).build().execute(query) then: 1 * dataFetcher.get(_) >> { DataFetchingEnvironment env -> - assert env.arguments.size() == 1 + assert env.arguments.size() == 0 assert env.arguments['bar'] == null } } diff --git a/src/test/groovy/graphql/NullValueSupportTest.groovy b/src/test/groovy/graphql/NullValueSupportTest.groovy new file mode 100644 index 0000000000..8d8ea8beb4 --- /dev/null +++ b/src/test/groovy/graphql/NullValueSupportTest.groovy @@ -0,0 +1,210 @@ +package graphql + +import graphql.validation.ValidationError +import graphql.validation.ValidationErrorType +import spock.lang.Specification + +/* + * Taken from http://facebook.github.io/graphql/#sec-Input-Objects + * + * + + Test Case Original Value Variables Coerced Value + A { a: "abc", b: 123 } null { a: "abc", b: 123 } + B { a: 123, b: "123" } null { a: "123", b: 123 } + C { a: "abc" } null Error: Missing required field b + D { a: "abc", b: null } null Error: b must be non‐null. + E { a: null, b: 1 } null { a: null, b: 1 } + F { b: $var } { var: 123 } { b: 123 } + G { b: $var } {} Error: Missing required field b. + H { b: $var } { var: null } Error: b must be non‐null. + I { a: $var, b: 1 } { var: null } { a: null, b: 1 } + J { a: $var, b: 1 } {} { b: 1 } + + */ + +class NullValueSupportTest extends Specification { + + def graphqlSpecExamples = ''' + schema { + query : Query + mutation : Mutation + } + + type Query { + a : String + b: Int! + } + + type Mutation { + mutate(inputArg : ExampleInputObject) : Query + } + + input ExampleInputObject { + a: String + b: Int! + } + + ''' + + def "test graphql spec examples that output results"() { + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) + + when: + + def result = GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) + + then: + assert result.errors.isEmpty(): "Validation Failure in case ${testCase} : $result.errors" + assert fetcher.args == expectedArgs: "Argument Failure in case ${testCase} : was ${fetcher.args}" + + where: + + testCase | queryStr | variables || expectedArgs + + // ------------------------------ + 'A' | ''' + mutation mutate { + mutate(inputArg : { a: "abc", b: 123 }) { + a + } + } + ''' | [:] || [inputArg: [a: "abc", b: 123]] + + // ------------------------------ + // coerced from string -> int and vice versus + // + // spec says it should work. but we think the spec is wrong since + // the reference implementation will not cross coerce these types + // + /* + 'B' | ''' + mutation mutate { + mutate(inputArg : { a: 123, b: "123" }) { + a + } + } + ''' | [:] || [inputArg: [a: "123", b: 123]] + */ + + // ------------------------------ + 'E' | ''' + mutation mutate { + mutate(inputArg : { a: null, b: 1 }) { + a + } + } + ''' | [:] || [inputArg: [a: null, b: 1]] + + // ------------------------------ + 'F' | ''' + mutation mutate($var : Int!) { + mutate(inputArg : { b: $var }) { + a + } + } + ''' | [var: 123] || [inputArg: [b: 123]] + + // ------------------------------ + 'I' | ''' + mutation mutate($var : String) { + mutate(inputArg : { a: $var, b: 1 }) { + a + } + } + ''' | [var: null] || [inputArg: [a: null, b: 1]] + + // ------------------------------ + 'J' | ''' + mutation mutate($var : String) { + mutate(inputArg : { a: $var, b: 1 }) { + a + } + } + ''' | [:] || [inputArg: [b: 1]] + } + + def "test graphql spec examples that output errors"() { + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) + + when: + + ExecutionResult result = null + try { + result = GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) + } catch (GraphQLException e) { + assert false: "Unexpected exception during ${testCase} : ${e.message}" + } + + then: + assert !result.errors.isEmpty(): "Expected errors in ${testCase}" + result.errors[0] instanceof ValidationError + (result.errors[0] as ValidationError).validationErrorType == expectedError + + + where: + + testCase | queryStr | variables || expectedError + + // ------------------------------ + 'C' | ''' + mutation mutate { + mutate(inputArg : { a: "abc"}) { + a + } + } + ''' | [:] || ValidationErrorType.WrongType + + // ------------------------------ + 'D' | ''' + mutation mutate { + mutate(inputArg : { a: "abc", b: null }) { + a + } + } + ''' | [:] || ValidationErrorType.WrongType + } + + def "test graphql spec examples that output exception"() { + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) + + when: + + GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) + + then: + def err = thrown(GraphQLException) + assert err.message.startsWith(expectedExceptionMsg): "Expected GraphQLError msg in ${testCase} : ${err.message}" + + + where: + + testCase | queryStr | variables || expectedExceptionMsg + + // ------------------------------ + 'G' | ''' + mutation mutate($var : Int!) { + mutate(inputArg : { b: $var }) { + a + } + } + ''' | [:] || "Null value for NonNull type GraphQLNonNull" + + // ------------------------------ + 'H' | ''' + mutation mutate($var : Int!) { + mutate(inputArg : { b: $var }) { + a + } + } + ''' | [var: null] || "Null value for NonNull type GraphQLNonNull" + + } + +} diff --git a/src/test/groovy/graphql/TestUtil.groovy b/src/test/groovy/graphql/TestUtil.groovy index 399e15e979..321c4a0604 100644 --- a/src/test/groovy/graphql/TestUtil.groovy +++ b/src/test/groovy/graphql/TestUtil.groovy @@ -1,6 +1,11 @@ package graphql import graphql.schema.* +import graphql.schema.idl.RuntimeWiring +import graphql.schema.idl.SchemaGenerator +import graphql.schema.idl.SchemaParser +import graphql.schema.idl.TypeRuntimeWiring +import graphql.schema.idl.errors.SchemaProblem import static graphql.Scalars.GraphQLString import static graphql.schema.GraphQLArgument.newArgument @@ -21,4 +26,28 @@ class TestUtil { .name("QueryType") .build()) .build() + + static GraphQLSchema schema(String spec, Map> dataFetchers) { + def wiring = RuntimeWiring.newRuntimeWiring() + dataFetchers.each { type, fieldFetchers -> + def tw = TypeRuntimeWiring.newTypeWiring(type).dataFetchers(fieldFetchers) + wiring.type(tw) + } + schema(spec, wiring) + } + + static GraphQLSchema schema(String spec, RuntimeWiring.Builder runtimeWiring) { + schema(spec, runtimeWiring.build()) + } + + static GraphQLSchema schema(String spec, RuntimeWiring runtimeWiring) { + try { + def registry = new SchemaParser().parse(spec) + return new SchemaGenerator().makeExecutableSchema(registry, runtimeWiring) + } catch (SchemaProblem e) { + assert false: "The schema could not be compiled : ${e}" + e.printStackTrace() + null + } + } } From ea26593b5c058338abb88caf3aecef7881a0a385 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Wed, 24 May 2017 10:06:42 +1000 Subject: [PATCH 03/10] #427 more tests and a specific exception for coerced null values --- ...onNullableValueCoercedAsNullException.java | 16 ++++++ .../graphql/execution/ValuesResolver.java | 5 +- .../graphql/NullValueSupportTest.groovy | 49 +++++++++++++++++-- .../execution/ValuesResolverTest.groovy | 8 ++- 4 files changed, 65 insertions(+), 13 deletions(-) create mode 100644 src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java diff --git a/src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java b/src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java new file mode 100644 index 0000000000..52e1de3c23 --- /dev/null +++ b/src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java @@ -0,0 +1,16 @@ +package graphql.execution; + +import graphql.GraphQLException; +import graphql.schema.GraphQLType; +import graphql.schema.GraphQLTypeUtil; + +/** + * This is thrown if a non nullable value is coerced to a null value + */ +public class NonNullableValueCoercedAsNullException extends GraphQLException { + + public NonNullableValueCoercedAsNullException(GraphQLType graphQLType) { + super(String.format("Null value for NonNull type '%s", GraphQLTypeUtil.getUnwrappedTypeName(graphQLType))); + } + +} diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index beefb89cd6..a9b74d0eb0 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -98,7 +98,7 @@ private Object coerceValue(GraphQLType graphQLType, Object value) { if (graphQLType instanceof GraphQLNonNull) { Object returnValue = coerceValue(((GraphQLNonNull) graphQLType).getWrappedType(), value); if (returnValue == null) { - throw new GraphQLException("Null value for NonNull type " + graphQLType); + throw new NonNullableValueCoercedAsNullException(graphQLType); } return returnValue; } @@ -240,8 +240,7 @@ private Object coerceValueAstForInputObject(GraphQLInputObjectType type, ObjectV private void assertNonNullInputField(GraphQLInputObjectField inputTypeField) { if (inputTypeField.getType() instanceof GraphQLNonNull) { - // Possibly overkill; an object literal with a missing non null field shouldn't pass validation - throw new GraphQLException("Null value for NonNull type " + inputTypeField.getType()); + throw new NonNullableValueCoercedAsNullException(inputTypeField.getType()); } } diff --git a/src/test/groovy/graphql/NullValueSupportTest.groovy b/src/test/groovy/graphql/NullValueSupportTest.groovy index 8d8ea8beb4..359a49915a 100644 --- a/src/test/groovy/graphql/NullValueSupportTest.groovy +++ b/src/test/groovy/graphql/NullValueSupportTest.groovy @@ -1,5 +1,6 @@ package graphql +import graphql.execution.NonNullableValueCoercedAsNullException import graphql.validation.ValidationError import graphql.validation.ValidationErrorType import spock.lang.Specification @@ -179,13 +180,12 @@ class NullValueSupportTest extends Specification { GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) then: - def err = thrown(GraphQLException) - assert err.message.startsWith(expectedExceptionMsg): "Expected GraphQLError msg in ${testCase} : ${err.message}" + thrown(expectedException) where: - testCase | queryStr | variables || expectedExceptionMsg + testCase | queryStr | variables || expectedException // ------------------------------ 'G' | ''' @@ -194,7 +194,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || "Null value for NonNull type GraphQLNonNull" + ''' | [:] || NonNullableValueCoercedAsNullException // ------------------------------ 'H' | ''' @@ -203,8 +203,47 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: null] || "Null value for NonNull type GraphQLNonNull" + ''' | [var: null] || NonNullableValueCoercedAsNullException } + def "nulls in literal places are supported in general"() { + + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(""" + schema { query : Query } + + type Query { + list(arg : [String]) : Int + scalar(arg : String) : Int + complex(arg : ComplexInputObject) : Int + } + + input ComplexInputObject { + a: String + b: Int! + } + + """, + ["Query": [ + "list" : fetcher, + "scalar" : fetcher, + "complex": fetcher, + ]]) + + when: + def result = GraphQL.newGraphQL(schema).build().execute(queryStr, null, "ctx", [:]) + assert result.errors.isEmpty(): "Unexpected query errors : ${result.errors}" + + then: + fetcher.args == expectedArgs + + where: + queryStr | expectedArgs + '''{ list(arg : ["abc", null, "xyz"]) }''' | [arg: ["abc", null, "xyz"]] + '''{ scalar(arg : null) }''' | [arg: null] + '''{ complex(arg : null) }''' | [arg: null] + + } } diff --git a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy index 63cbb7fdbe..8aab82a69a 100644 --- a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy +++ b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy @@ -251,8 +251,8 @@ class ValuesResolverTest extends Specification { def "getArgumentValues: resolves enum literals"() { given: "the ast" - EnumValue enumValue1 = new EnumValue("PLUTO"); - EnumValue enumValue2 = new EnumValue("MARS"); + EnumValue enumValue1 = new EnumValue("PLUTO") + EnumValue enumValue2 = new EnumValue("MARS") def argument1 = new Argument("arg1", enumValue1) def argument2 = new Argument("arg2", enumValue2) @@ -400,9 +400,7 @@ class ValuesResolverTest extends Specification { where: InputValue || outputValue [foo: "added", bar: null] || [foo: "added", bar: null] - - // later this will be true once we apply missing value code - //[foo: "added"] || [foo: "added"] + [foo: "added"] || [foo: "added"] } } From ef62c13e447b810e7f12cd66381649a3f3e71c94 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 23 May 2017 08:22:55 +1000 Subject: [PATCH 04/10] #427 added null support but have not added optional arguments yet --- src/main/antlr/Graphql.g4 | 4 ++ .../graphql/execution/ValuesResolver.java | 3 ++ .../java/graphql/language/AstPrinter.java | 3 ++ src/main/java/graphql/language/NullValue.java | 33 +++++++++++++ .../parser/GraphqlAntlrToLanguage.java | 9 ++++ .../graphql/schema/idl/SchemaGenerator.java | 3 ++ .../graphql/validation/ValidationUtil.java | 3 ++ .../execution/ValuesResolverTest.groovy | 22 +++++++++ .../graphql/language/AstPrinterTest.groovy | 49 +++++++++++++++++++ .../validation/ValidationUtilTest.groovy | 10 ++++ 10 files changed, 139 insertions(+) create mode 100644 src/main/java/graphql/language/NullValue.java diff --git a/src/main/antlr/Graphql.g4 b/src/main/antlr/Graphql.g4 index d7a3b649f9..51b6be8e07 100644 --- a/src/main/antlr/Graphql.g4 +++ b/src/main/antlr/Graphql.g4 @@ -66,6 +66,7 @@ IntValue | FloatValue | StringValue | BooleanValue | +NullValue | enumValue | arrayValue | objectValue; @@ -76,6 +77,7 @@ IntValue | FloatValue | StringValue | BooleanValue | +NullValue | enumValue | arrayValueWithVariable | objectValueWithVariable; @@ -176,6 +178,8 @@ directiveLocations '|' directiveLocation BooleanValue: 'true' | 'false'; +NullValue: 'null'; + FRAGMENT: 'fragment'; QUERY: 'query'; MUTATION: 'mutation'; diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index f03dabf381..e73d53364d 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -129,6 +129,9 @@ private Object coerceValueAst(GraphQLType type, Value inputValue, Map getChildren() { + return Collections.emptyList(); + } + + @Override + public boolean isEqualTo(Node o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + return true; + + } + + @Override + public String toString() { + return "NullValue{" + + '}'; + } +} diff --git a/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java b/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java index a9a2c26b90..aee0ac3401 100644 --- a/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java +++ b/src/main/java/graphql/parser/GraphqlAntlrToLanguage.java @@ -58,7 +58,10 @@ import java.util.Deque; import java.util.List; +import static graphql.language.NullValue.Null; + @Internal + public class GraphqlAntlrToLanguage extends GraphqlBaseVisitor { private final CommonTokenStream tokens; @@ -668,6 +671,9 @@ private Value getValue(GraphqlParser.ValueWithVariableContext ctx) { BooleanValue booleanValue = new BooleanValue(Boolean.parseBoolean(ctx.BooleanValue().getText())); newNode(booleanValue, ctx); return booleanValue; + } else if (ctx.NullValue() != null) { + newNode(Null, ctx); + return Null; } else if (ctx.StringValue() != null) { StringValue stringValue = new StringValue(parseString(ctx.StringValue().getText())); newNode(stringValue, ctx); @@ -713,6 +719,9 @@ private Value getValue(GraphqlParser.ValueContext ctx) { BooleanValue booleanValue = new BooleanValue(Boolean.parseBoolean(ctx.BooleanValue().getText())); newNode(booleanValue, ctx); return booleanValue; + } else if (ctx.NullValue() != null) { + newNode(Null, ctx); + return Null; } else if (ctx.StringValue() != null) { StringValue stringValue = new StringValue(parseString(ctx.StringValue().getText())); newNode(stringValue, ctx); diff --git a/src/main/java/graphql/schema/idl/SchemaGenerator.java b/src/main/java/graphql/schema/idl/SchemaGenerator.java index 890b4f901c..5e1936b692 100644 --- a/src/main/java/graphql/schema/idl/SchemaGenerator.java +++ b/src/main/java/graphql/schema/idl/SchemaGenerator.java @@ -13,6 +13,7 @@ import graphql.language.IntValue; import graphql.language.InterfaceTypeDefinition; import graphql.language.Node; +import graphql.language.NullValue; import graphql.language.ObjectTypeDefinition; import graphql.language.ObjectValue; import graphql.language.OperationTypeDefinition; @@ -475,6 +476,8 @@ private Object buildValue(Value value) { result = arrayValue.getValues().stream().map(this::buildValue).toArray(); } else if (value instanceof ObjectValue) { result = buildObjectValue((ObjectValue) value); + } else if (value instanceof NullValue) { + result = null; } return result; diff --git a/src/main/java/graphql/validation/ValidationUtil.java b/src/main/java/graphql/validation/ValidationUtil.java index e75928fcbc..2ef7f448a8 100644 --- a/src/main/java/graphql/validation/ValidationUtil.java +++ b/src/main/java/graphql/validation/ValidationUtil.java @@ -25,6 +25,9 @@ public boolean isValidLiteralValue(Value value, GraphQLType type) { if (value == null) { return !(type instanceof GraphQLNonNull); } + if (value instanceof NullValue) { + return !(type instanceof GraphQLNonNull); + } if (value instanceof VariableReference) { return true; } diff --git a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy index 30b4fb1da8..63cbb7fdbe 100644 --- a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy +++ b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy @@ -383,4 +383,26 @@ class ValuesResolverTest extends Specification { [intKey: 10] | _ [intKey: 10, requiredField: null] | _ } + + def "getVariableValues: simple types with values not provided in variables map"() { + given: + + def schema = TestUtil.schemaWithInputType(GraphQLString) + VariableDefinition fooVarDef = new VariableDefinition("foo", new TypeName("String")) + VariableDefinition barVarDef = new VariableDefinition("bar", new TypeName("String")) + + when: + def resolvedValues = resolver.getVariableValues(schema, [fooVarDef, barVarDef], InputValue) + + then: + resolvedValues == outputValue + + where: + InputValue || outputValue + [foo: "added", bar: null] || [foo: "added", bar: null] + + // later this will be true once we apply missing value code + //[foo: "added"] || [foo: "added"] + } + } diff --git a/src/test/groovy/graphql/language/AstPrinterTest.groovy b/src/test/groovy/graphql/language/AstPrinterTest.groovy index d7344728f4..482218ce49 100644 --- a/src/test/groovy/graphql/language/AstPrinterTest.groovy +++ b/src/test/groovy/graphql/language/AstPrinterTest.groovy @@ -323,4 +323,53 @@ query HeroNameAndFriends($episode: Episode = "JEDI") { ''' } +//------------------------------------------------- + def "ast printing of null"() { + def query = ''' +query NullEpisodeQuery { + hero(episode: null) { + name + } +} +''' + def document = parse(query) + String output = printAst(document) + + expect: + output == '''query NullEpisodeQuery { + hero(episode: null) { + name + } +} +''' + } + + //------------------------------------------------- + def "ast printing of default variables with null"() { + def query = ''' +query NullVariableDefaultValueQuery($episode: Episode = null) { + hero(episode: $episode) { + name + friends { + name + } + } +} +''' + def document = parse(query) + String output = printAst(document) + + expect: + output == '''query NullVariableDefaultValueQuery($episode: Episode = null) { + hero(episode: $episode) { + name + friends { + name + } + } +} +''' + } + + } diff --git a/src/test/groovy/graphql/validation/ValidationUtilTest.groovy b/src/test/groovy/graphql/validation/ValidationUtilTest.groovy index 9e4f908127..3beaf42f40 100644 --- a/src/test/groovy/graphql/validation/ValidationUtilTest.groovy +++ b/src/test/groovy/graphql/validation/ValidationUtilTest.groovy @@ -39,6 +39,11 @@ class ValidationUtilTest extends Specification { !validationUtil.isValidLiteralValue(null, nonNull(GraphQLString)) } + def "NullValue and NonNull is invalid"() { + expect: + !validationUtil.isValidLiteralValue(NullValue.Null, nonNull(GraphQLString)) + } + def "a nonNull value for a NonNull type is valid"() { expect: validationUtil.isValidLiteralValue(new StringValue("string"), nonNull(GraphQLString)) @@ -49,6 +54,11 @@ class ValidationUtilTest extends Specification { validationUtil.isValidLiteralValue(null, GraphQLString) } + def "NullValue is valid when type is NonNull"() { + expect: + validationUtil.isValidLiteralValue(NullValue.Null, GraphQLString) + } + def "variables are valid"() { expect: validationUtil.isValidLiteralValue(new VariableReference("var"), GraphQLBoolean) From 5934af18c3b02d6e6830da6a175abcebdd406314 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 23 May 2017 23:50:30 +1000 Subject: [PATCH 05/10] #427 added spec tests and values resolver support --- .../graphql/execution/ExecutionStrategy.java | 4 +- .../graphql/execution/ValuesResolver.java | 87 ++++++-- .../graphql/CapturingDataFetcher.groovy | 19 ++ src/test/groovy/graphql/GraphQLTest.groovy | 12 +- .../graphql/NullValueSupportTest.groovy | 210 ++++++++++++++++++ src/test/groovy/graphql/TestUtil.groovy | 29 +++ 6 files changed, 340 insertions(+), 21 deletions(-) create mode 100644 src/test/groovy/graphql/CapturingDataFetcher.groovy create mode 100644 src/test/groovy/graphql/NullValueSupportTest.groovy diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 9c813b0bc5..f5cd3d0574 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -11,6 +11,7 @@ import graphql.execution.instrumentation.parameters.FieldFetchParameters; import graphql.execution.instrumentation.parameters.FieldParameters; import graphql.language.Field; +import graphql.schema.DataFetcher; import graphql.schema.DataFetchingEnvironment; import graphql.schema.DataFetchingEnvironmentImpl; import graphql.schema.DataFetchingFieldSelectionSet; @@ -96,7 +97,8 @@ protected ExecutionResult resolveField(ExecutionContext executionContext, Execut InstrumentationContext fetchCtx = instrumentation.beginFieldFetch(new FieldFetchParameters(executionContext, fieldDef, environment)); Object resolvedValue = null; try { - resolvedValue = fieldDef.getDataFetcher().get(environment); + DataFetcher dataFetcher = fieldDef.getDataFetcher(); + resolvedValue = dataFetcher.get(environment); fetchCtx.onEnd(resolvedValue); } catch (Exception e) { diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index e73d53364d..beefb89cd6 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -2,18 +2,43 @@ import graphql.GraphQLException; -import graphql.language.*; -import graphql.schema.*; +import graphql.language.Argument; +import graphql.language.ArrayValue; +import graphql.language.NullValue; +import graphql.language.ObjectField; +import graphql.language.ObjectValue; +import graphql.language.Value; +import graphql.language.VariableDefinition; +import graphql.language.VariableReference; +import graphql.schema.GraphQLArgument; +import graphql.schema.GraphQLEnumType; +import graphql.schema.GraphQLInputObjectField; +import graphql.schema.GraphQLInputObjectType; +import graphql.schema.GraphQLList; +import graphql.schema.GraphQLNonNull; +import graphql.schema.GraphQLScalarType; +import graphql.schema.GraphQLSchema; +import graphql.schema.GraphQLType; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; public class ValuesResolver { - public Map getVariableValues(GraphQLSchema schema, List variableDefinitions, Map inputs) { + public Map getVariableValues(GraphQLSchema schema, List variableDefinitions, Map args) { Map result = new LinkedHashMap<>(); for (VariableDefinition variableDefinition : variableDefinitions) { - result.put(variableDefinition.getName(), getVariableValue(schema, variableDefinition, inputs.get(variableDefinition.getName()))); + String varName = variableDefinition.getName(); + // we transfer the variable as field arguments if its present as value + if (args.containsKey(varName)) { + Object arg = args.get(varName); + Object variableValue = getVariableValue(schema, variableDefinition, arg); + result.put(varName, variableValue); + } } return result; } @@ -23,14 +48,19 @@ public Map getArgumentValues(List argumentTypes Map result = new LinkedHashMap<>(); Map argumentMap = argumentMap(arguments); for (GraphQLArgument fieldArgument : argumentTypes) { - Argument argument = argumentMap.get(fieldArgument.getName()); + String argName = fieldArgument.getName(); + Argument argument = argumentMap.get(argName); Object value; if (argument != null) { value = coerceValueAst(fieldArgument.getType(), argument.getValue(), variables); } else { value = fieldArgument.getDefaultValue(); } - result.put(fieldArgument.getName(), value); + // only put an arg into the result IF they specified a variable at all or + // the default value ended up being something non null + if (argumentMap.containsKey(argName) || value != null) { + result.put(argName, value); + } } return result; } @@ -48,6 +78,7 @@ private Map argumentMap(List arguments) { private Object getVariableValue(GraphQLSchema schema, VariableDefinition variableDefinition, Object inputValue) { GraphQLType type = TypeFromAST.getTypeFromAST(schema, variableDefinition.getType()); + //noinspection ConstantConditions if (!isValid(type, inputValue)) { throw new GraphQLException("Invalid value for type"); } @@ -81,6 +112,7 @@ private Object coerceValue(GraphQLType graphQLType, Object value) { } else if (graphQLType instanceof GraphQLList) { return coerceValueForList((GraphQLList) graphQLType, value); } else if (graphQLType instanceof GraphQLInputObjectType && value instanceof Map) { + //noinspection unchecked return coerceValueForInputObjectType((GraphQLInputObjectType) graphQLType, (Map) value); } else if (graphQLType instanceof GraphQLInputObjectType) { return value; @@ -170,22 +202,49 @@ private Object coerceValueAstForInputObject(GraphQLInputObjectType type, ObjectV for (GraphQLInputObjectField inputTypeField : type.getFields()) { if (inputValueFieldsByName.containsKey(inputTypeField.getName())) { + boolean putObjectInMap = true; + ObjectField field = inputValueFieldsByName.get(inputTypeField.getName()); - Object fieldValue = coerceValueAst(inputTypeField.getType(), field.getValue(), variables); - if (fieldValue == null) { - fieldValue = inputTypeField.getDefaultValue(); + Value fieldInputValue = field.getValue(); + + Object fieldObject = null; + if (fieldInputValue instanceof VariableReference) { + String varName = ((VariableReference) fieldInputValue).getName(); + if (!variables.containsKey(varName)) { + putObjectInMap = false; + } else { + fieldObject = variables.get(varName); + } + } else { + fieldObject = coerceValueAst(inputTypeField.getType(), fieldInputValue, variables); + } + + if (fieldObject == null) { + if (!field.getValue().isEqualTo(NullValue.Null)) { + fieldObject = inputTypeField.getDefaultValue(); + } + } + if (putObjectInMap) { + result.put(field.getName(), fieldObject); + } else { + assertNonNullInputField(inputTypeField); } - result.put(field.getName(), fieldValue); } else if (inputTypeField.getDefaultValue() != null) { result.put(inputTypeField.getName(), inputTypeField.getDefaultValue()); - } else if (inputTypeField.getType() instanceof GraphQLNonNull) { - // Possibly overkill; an object literal with a missing non null field shouldn't pass validation - throw new GraphQLException("Null value for NonNull type " + inputTypeField.getType()); + } else { + assertNonNullInputField(inputTypeField); } } return result; } + private void assertNonNullInputField(GraphQLInputObjectField inputTypeField) { + if (inputTypeField.getType() instanceof GraphQLNonNull) { + // Possibly overkill; an object literal with a missing non null field shouldn't pass validation + throw new GraphQLException("Null value for NonNull type " + inputTypeField.getType()); + } + } + private Map mapObjectValueFieldsByName(ObjectValue inputValue) { Map inputValueFieldsByName = new LinkedHashMap<>(); for (ObjectField objectField : inputValue.getObjectFields()) { diff --git a/src/test/groovy/graphql/CapturingDataFetcher.groovy b/src/test/groovy/graphql/CapturingDataFetcher.groovy new file mode 100644 index 0000000000..b6a83885ef --- /dev/null +++ b/src/test/groovy/graphql/CapturingDataFetcher.groovy @@ -0,0 +1,19 @@ +package graphql + +import graphql.schema.DataFetcher +import graphql.schema.DataFetchingEnvironment + +/** + * Help to capture data fetcher arguments passed in + */ +class CapturingDataFetcher implements DataFetcher { + Map args + DataFetchingEnvironment environment + + @Override + Object get(DataFetchingEnvironment environment) { + this.environment = environment + this.args = environment.getArguments() + null + } +} diff --git a/src/test/groovy/graphql/GraphQLTest.groovy b/src/test/groovy/graphql/GraphQLTest.groovy index 321c7aa168..9c5b974b24 100644 --- a/src/test/groovy/graphql/GraphQLTest.groovy +++ b/src/test/groovy/graphql/GraphQLTest.groovy @@ -157,10 +157,10 @@ class GraphQLTest extends Specification { set.add("One") set.add("Two") - def schema = GraphQLSchema.newSchema() - .query(GraphQLObjectType.newObject() + def schema = newSchema() + .query(newObject() .name("QueryType") - .field(GraphQLFieldDefinition.newFieldDefinition() + .field(newFieldDefinition() .name("set") .type(new GraphQLList(GraphQLString)) .dataFetcher({ set }))) @@ -231,7 +231,7 @@ class GraphQLTest extends Specification { .build() when: - def result = new GraphQL(schema).execute("mutation { doesNotExist }"); + def result = new GraphQL(schema).execute("mutation { doesNotExist }") then: result.errors.size() == 1 @@ -372,12 +372,12 @@ class GraphQLTest extends Specification { .build() def query = "{foo}" when: - def result = GraphQL.newGraphQL(schema).build().execute(query) + GraphQL.newGraphQL(schema).build().execute(query) then: 1 * dataFetcher.get(_) >> { DataFetchingEnvironment env -> - assert env.arguments.size() == 1 + assert env.arguments.size() == 0 assert env.arguments['bar'] == null } } diff --git a/src/test/groovy/graphql/NullValueSupportTest.groovy b/src/test/groovy/graphql/NullValueSupportTest.groovy new file mode 100644 index 0000000000..8d8ea8beb4 --- /dev/null +++ b/src/test/groovy/graphql/NullValueSupportTest.groovy @@ -0,0 +1,210 @@ +package graphql + +import graphql.validation.ValidationError +import graphql.validation.ValidationErrorType +import spock.lang.Specification + +/* + * Taken from http://facebook.github.io/graphql/#sec-Input-Objects + * + * + + Test Case Original Value Variables Coerced Value + A { a: "abc", b: 123 } null { a: "abc", b: 123 } + B { a: 123, b: "123" } null { a: "123", b: 123 } + C { a: "abc" } null Error: Missing required field b + D { a: "abc", b: null } null Error: b must be non‐null. + E { a: null, b: 1 } null { a: null, b: 1 } + F { b: $var } { var: 123 } { b: 123 } + G { b: $var } {} Error: Missing required field b. + H { b: $var } { var: null } Error: b must be non‐null. + I { a: $var, b: 1 } { var: null } { a: null, b: 1 } + J { a: $var, b: 1 } {} { b: 1 } + + */ + +class NullValueSupportTest extends Specification { + + def graphqlSpecExamples = ''' + schema { + query : Query + mutation : Mutation + } + + type Query { + a : String + b: Int! + } + + type Mutation { + mutate(inputArg : ExampleInputObject) : Query + } + + input ExampleInputObject { + a: String + b: Int! + } + + ''' + + def "test graphql spec examples that output results"() { + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) + + when: + + def result = GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) + + then: + assert result.errors.isEmpty(): "Validation Failure in case ${testCase} : $result.errors" + assert fetcher.args == expectedArgs: "Argument Failure in case ${testCase} : was ${fetcher.args}" + + where: + + testCase | queryStr | variables || expectedArgs + + // ------------------------------ + 'A' | ''' + mutation mutate { + mutate(inputArg : { a: "abc", b: 123 }) { + a + } + } + ''' | [:] || [inputArg: [a: "abc", b: 123]] + + // ------------------------------ + // coerced from string -> int and vice versus + // + // spec says it should work. but we think the spec is wrong since + // the reference implementation will not cross coerce these types + // + /* + 'B' | ''' + mutation mutate { + mutate(inputArg : { a: 123, b: "123" }) { + a + } + } + ''' | [:] || [inputArg: [a: "123", b: 123]] + */ + + // ------------------------------ + 'E' | ''' + mutation mutate { + mutate(inputArg : { a: null, b: 1 }) { + a + } + } + ''' | [:] || [inputArg: [a: null, b: 1]] + + // ------------------------------ + 'F' | ''' + mutation mutate($var : Int!) { + mutate(inputArg : { b: $var }) { + a + } + } + ''' | [var: 123] || [inputArg: [b: 123]] + + // ------------------------------ + 'I' | ''' + mutation mutate($var : String) { + mutate(inputArg : { a: $var, b: 1 }) { + a + } + } + ''' | [var: null] || [inputArg: [a: null, b: 1]] + + // ------------------------------ + 'J' | ''' + mutation mutate($var : String) { + mutate(inputArg : { a: $var, b: 1 }) { + a + } + } + ''' | [:] || [inputArg: [b: 1]] + } + + def "test graphql spec examples that output errors"() { + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) + + when: + + ExecutionResult result = null + try { + result = GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) + } catch (GraphQLException e) { + assert false: "Unexpected exception during ${testCase} : ${e.message}" + } + + then: + assert !result.errors.isEmpty(): "Expected errors in ${testCase}" + result.errors[0] instanceof ValidationError + (result.errors[0] as ValidationError).validationErrorType == expectedError + + + where: + + testCase | queryStr | variables || expectedError + + // ------------------------------ + 'C' | ''' + mutation mutate { + mutate(inputArg : { a: "abc"}) { + a + } + } + ''' | [:] || ValidationErrorType.WrongType + + // ------------------------------ + 'D' | ''' + mutation mutate { + mutate(inputArg : { a: "abc", b: null }) { + a + } + } + ''' | [:] || ValidationErrorType.WrongType + } + + def "test graphql spec examples that output exception"() { + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) + + when: + + GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) + + then: + def err = thrown(GraphQLException) + assert err.message.startsWith(expectedExceptionMsg): "Expected GraphQLError msg in ${testCase} : ${err.message}" + + + where: + + testCase | queryStr | variables || expectedExceptionMsg + + // ------------------------------ + 'G' | ''' + mutation mutate($var : Int!) { + mutate(inputArg : { b: $var }) { + a + } + } + ''' | [:] || "Null value for NonNull type GraphQLNonNull" + + // ------------------------------ + 'H' | ''' + mutation mutate($var : Int!) { + mutate(inputArg : { b: $var }) { + a + } + } + ''' | [var: null] || "Null value for NonNull type GraphQLNonNull" + + } + +} diff --git a/src/test/groovy/graphql/TestUtil.groovy b/src/test/groovy/graphql/TestUtil.groovy index 399e15e979..321c4a0604 100644 --- a/src/test/groovy/graphql/TestUtil.groovy +++ b/src/test/groovy/graphql/TestUtil.groovy @@ -1,6 +1,11 @@ package graphql import graphql.schema.* +import graphql.schema.idl.RuntimeWiring +import graphql.schema.idl.SchemaGenerator +import graphql.schema.idl.SchemaParser +import graphql.schema.idl.TypeRuntimeWiring +import graphql.schema.idl.errors.SchemaProblem import static graphql.Scalars.GraphQLString import static graphql.schema.GraphQLArgument.newArgument @@ -21,4 +26,28 @@ class TestUtil { .name("QueryType") .build()) .build() + + static GraphQLSchema schema(String spec, Map> dataFetchers) { + def wiring = RuntimeWiring.newRuntimeWiring() + dataFetchers.each { type, fieldFetchers -> + def tw = TypeRuntimeWiring.newTypeWiring(type).dataFetchers(fieldFetchers) + wiring.type(tw) + } + schema(spec, wiring) + } + + static GraphQLSchema schema(String spec, RuntimeWiring.Builder runtimeWiring) { + schema(spec, runtimeWiring.build()) + } + + static GraphQLSchema schema(String spec, RuntimeWiring runtimeWiring) { + try { + def registry = new SchemaParser().parse(spec) + return new SchemaGenerator().makeExecutableSchema(registry, runtimeWiring) + } catch (SchemaProblem e) { + assert false: "The schema could not be compiled : ${e}" + e.printStackTrace() + null + } + } } From cfe904973d1871a22a58c546ea6ce4763f7cdcfa Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Wed, 24 May 2017 10:06:42 +1000 Subject: [PATCH 06/10] #427 more tests and a specific exception for coerced null values --- ...onNullableValueCoercedAsNullException.java | 16 ++++++ .../graphql/execution/ValuesResolver.java | 5 +- .../graphql/NullValueSupportTest.groovy | 49 +++++++++++++++++-- .../execution/ValuesResolverTest.groovy | 8 ++- 4 files changed, 65 insertions(+), 13 deletions(-) create mode 100644 src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java diff --git a/src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java b/src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java new file mode 100644 index 0000000000..52e1de3c23 --- /dev/null +++ b/src/main/java/graphql/execution/NonNullableValueCoercedAsNullException.java @@ -0,0 +1,16 @@ +package graphql.execution; + +import graphql.GraphQLException; +import graphql.schema.GraphQLType; +import graphql.schema.GraphQLTypeUtil; + +/** + * This is thrown if a non nullable value is coerced to a null value + */ +public class NonNullableValueCoercedAsNullException extends GraphQLException { + + public NonNullableValueCoercedAsNullException(GraphQLType graphQLType) { + super(String.format("Null value for NonNull type '%s", GraphQLTypeUtil.getUnwrappedTypeName(graphQLType))); + } + +} diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index beefb89cd6..a9b74d0eb0 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -98,7 +98,7 @@ private Object coerceValue(GraphQLType graphQLType, Object value) { if (graphQLType instanceof GraphQLNonNull) { Object returnValue = coerceValue(((GraphQLNonNull) graphQLType).getWrappedType(), value); if (returnValue == null) { - throw new GraphQLException("Null value for NonNull type " + graphQLType); + throw new NonNullableValueCoercedAsNullException(graphQLType); } return returnValue; } @@ -240,8 +240,7 @@ private Object coerceValueAstForInputObject(GraphQLInputObjectType type, ObjectV private void assertNonNullInputField(GraphQLInputObjectField inputTypeField) { if (inputTypeField.getType() instanceof GraphQLNonNull) { - // Possibly overkill; an object literal with a missing non null field shouldn't pass validation - throw new GraphQLException("Null value for NonNull type " + inputTypeField.getType()); + throw new NonNullableValueCoercedAsNullException(inputTypeField.getType()); } } diff --git a/src/test/groovy/graphql/NullValueSupportTest.groovy b/src/test/groovy/graphql/NullValueSupportTest.groovy index 8d8ea8beb4..359a49915a 100644 --- a/src/test/groovy/graphql/NullValueSupportTest.groovy +++ b/src/test/groovy/graphql/NullValueSupportTest.groovy @@ -1,5 +1,6 @@ package graphql +import graphql.execution.NonNullableValueCoercedAsNullException import graphql.validation.ValidationError import graphql.validation.ValidationErrorType import spock.lang.Specification @@ -179,13 +180,12 @@ class NullValueSupportTest extends Specification { GraphQL.newGraphQL(schema).build().execute(queryStr, "mutate", "ctx", variables) then: - def err = thrown(GraphQLException) - assert err.message.startsWith(expectedExceptionMsg): "Expected GraphQLError msg in ${testCase} : ${err.message}" + thrown(expectedException) where: - testCase | queryStr | variables || expectedExceptionMsg + testCase | queryStr | variables || expectedException // ------------------------------ 'G' | ''' @@ -194,7 +194,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || "Null value for NonNull type GraphQLNonNull" + ''' | [:] || NonNullableValueCoercedAsNullException // ------------------------------ 'H' | ''' @@ -203,8 +203,47 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: null] || "Null value for NonNull type GraphQLNonNull" + ''' | [var: null] || NonNullableValueCoercedAsNullException } + def "nulls in literal places are supported in general"() { + + def fetcher = new CapturingDataFetcher() + + def schema = TestUtil.schema(""" + schema { query : Query } + + type Query { + list(arg : [String]) : Int + scalar(arg : String) : Int + complex(arg : ComplexInputObject) : Int + } + + input ComplexInputObject { + a: String + b: Int! + } + + """, + ["Query": [ + "list" : fetcher, + "scalar" : fetcher, + "complex": fetcher, + ]]) + + when: + def result = GraphQL.newGraphQL(schema).build().execute(queryStr, null, "ctx", [:]) + assert result.errors.isEmpty(): "Unexpected query errors : ${result.errors}" + + then: + fetcher.args == expectedArgs + + where: + queryStr | expectedArgs + '''{ list(arg : ["abc", null, "xyz"]) }''' | [arg: ["abc", null, "xyz"]] + '''{ scalar(arg : null) }''' | [arg: null] + '''{ complex(arg : null) }''' | [arg: null] + + } } diff --git a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy index 63cbb7fdbe..8aab82a69a 100644 --- a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy +++ b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy @@ -251,8 +251,8 @@ class ValuesResolverTest extends Specification { def "getArgumentValues: resolves enum literals"() { given: "the ast" - EnumValue enumValue1 = new EnumValue("PLUTO"); - EnumValue enumValue2 = new EnumValue("MARS"); + EnumValue enumValue1 = new EnumValue("PLUTO") + EnumValue enumValue2 = new EnumValue("MARS") def argument1 = new Argument("arg1", enumValue1) def argument2 = new Argument("arg2", enumValue2) @@ -400,9 +400,7 @@ class ValuesResolverTest extends Specification { where: InputValue || outputValue [foo: "added", bar: null] || [foo: "added", bar: null] - - // later this will be true once we apply missing value code - //[foo: "added"] || [foo: "added"] + [foo: "added"] || [foo: "added"] } } From 433f3199138f28cfcf5f4e4826ba3377bd399556 Mon Sep 17 00:00:00 2001 From: andimarek Date: Wed, 24 May 2017 16:32:41 +0200 Subject: [PATCH 07/10] add null value parser test --- src/test/groovy/graphql/parser/ParserTest.groovy | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/test/groovy/graphql/parser/ParserTest.groovy b/src/test/groovy/graphql/parser/ParserTest.groovy index f9c69e1ed1..6f72f48e4f 100644 --- a/src/test/groovy/graphql/parser/ParserTest.groovy +++ b/src/test/groovy/graphql/parser/ParserTest.groovy @@ -465,4 +465,19 @@ class ParserTest extends Specification { then: noExceptionThrown() } + + + def "parses null value"() { + given: + def input = "{ foo(bar: null) }" + + when: + def document = new Parser().parseDocument(input) + def operation = document.definitions[0] as OperationDefinition + def selection = operation.selectionSet.selections[0] as Field + + then: + selection.arguments[0].value == NullValue.Null + + } } From d03a21b29968bf6ab319ca4b75d556b01b0b686c Mon Sep 17 00:00:00 2001 From: andimarek Date: Wed, 24 May 2017 16:34:54 +0200 Subject: [PATCH 08/10] cleanup: remove dead code --- src/test/groovy/graphql/TestUtil.groovy | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/groovy/graphql/TestUtil.groovy b/src/test/groovy/graphql/TestUtil.groovy index 321c4a0604..2cd8569bd6 100644 --- a/src/test/groovy/graphql/TestUtil.groovy +++ b/src/test/groovy/graphql/TestUtil.groovy @@ -46,8 +46,6 @@ class TestUtil { return new SchemaGenerator().makeExecutableSchema(registry, runtimeWiring) } catch (SchemaProblem e) { assert false: "The schema could not be compiled : ${e}" - e.printStackTrace() - null } } } From 215934552236ae0bc4e0397f74948f7b05b06122 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sat, 27 May 2017 20:09:52 +1000 Subject: [PATCH 09/10] #427 - more tests for null variables values --- .../graphql/NullValueSupportTest.groovy | 96 ++++++++++++++----- 1 file changed, 73 insertions(+), 23 deletions(-) diff --git a/src/test/groovy/graphql/NullValueSupportTest.groovy b/src/test/groovy/graphql/NullValueSupportTest.groovy index 359a49915a..ba34179bfb 100644 --- a/src/test/groovy/graphql/NullValueSupportTest.groovy +++ b/src/test/groovy/graphql/NullValueSupportTest.groovy @@ -4,23 +4,32 @@ import graphql.execution.NonNullableValueCoercedAsNullException import graphql.validation.ValidationError import graphql.validation.ValidationErrorType import spock.lang.Specification +import spock.lang.Unroll /* * Taken from http://facebook.github.io/graphql/#sec-Input-Objects * * - Test Case Original Value Variables Coerced Value - A { a: "abc", b: 123 } null { a: "abc", b: 123 } - B { a: 123, b: "123" } null { a: "123", b: 123 } - C { a: "abc" } null Error: Missing required field b - D { a: "abc", b: null } null Error: b must be non‐null. - E { a: null, b: 1 } null { a: null, b: 1 } - F { b: $var } { var: 123 } { b: 123 } - G { b: $var } {} Error: Missing required field b. - H { b: $var } { var: null } Error: b must be non‐null. - I { a: $var, b: 1 } { var: null } { a: null, b: 1 } - J { a: $var, b: 1 } {} { b: 1 } + Test Case Original Value Variables Coerced Value + -------------------------------------------------------------------------------------------- + A { a: "abc", b: 123 } null { a: "abc", b: 123 } + B { a: 123, b: "123" } null { a: "123", b: 123 } + C { a: "abc" } null Error: Missing required field b + D { a: "abc", b: null } null Error: b must be non‐null. + E { a: null, b: 1 } null { a: null, b: 1 } + F { b: $var } { var: 123 } { b: 123 } + G { b: $var } {} Error: Missing required field b. + H { b: $var } { var: null } Error: b must be non‐null. + I { a: $var, b: 1 } { var: null } { a: null, b: 1 } + J { a: $var, b: 1 } {} { b: 1 } + + These did not come from the spec but added by us as extra tests + + K { $var } { a : "abc", b:123 } { a: "abc", b: 123 } + L { $var } { b:123 } { b: 123 } + M { $var } { a : "abc", b:null } Error: b must be non‐null. + N { $var } { a : "abc" } Error: b must be non‐null. */ @@ -48,7 +57,8 @@ class NullValueSupportTest extends Specification { ''' - def "test graphql spec examples that output results"() { + @Unroll + "test graphql spec examples that output results : #testCase"() { def fetcher = new CapturingDataFetcher() def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) @@ -63,7 +73,7 @@ class NullValueSupportTest extends Specification { where: - testCase | queryStr | variables || expectedArgs + testCase | queryStr | variables || expectedArgs // ------------------------------ 'A' | ''' @@ -72,7 +82,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || [inputArg: [a: "abc", b: 123]] + ''' | [:] || [inputArg: [a: "abc", b: 123]] // ------------------------------ // coerced from string -> int and vice versus @@ -97,7 +107,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || [inputArg: [a: null, b: 1]] + ''' | [:] || [inputArg: [a: null, b: 1]] // ------------------------------ 'F' | ''' @@ -106,7 +116,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: 123] || [inputArg: [b: 123]] + ''' | [var: 123] || [inputArg: [b: 123]] // ------------------------------ 'I' | ''' @@ -115,7 +125,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: null] || [inputArg: [a: null, b: 1]] + ''' | [var: null] || [inputArg: [a: null, b: 1]] // ------------------------------ 'J' | ''' @@ -124,10 +134,30 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || [inputArg: [b: 1]] + ''' | [:] || [inputArg: [b: 1]] + + // ------------------------------ + 'K' | ''' + mutation mutate($var : ExampleInputObject) { + mutate(inputArg : $var) { + a + } + } + ''' | [var: [a: "abc", b: 123]] || [inputArg: [a: "abc", b: 123]] + + // ------------------------------ + 'L' | ''' + mutation mutate($var : ExampleInputObject) { + mutate(inputArg : $var) { + a + } + } + ''' | [var: [b: 123]] || [inputArg: [b: 123]] + } - def "test graphql spec examples that output errors"() { + @Unroll + "test graphql spec examples that output errors #testCase"() { def fetcher = new CapturingDataFetcher() def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) @@ -170,7 +200,8 @@ class NullValueSupportTest extends Specification { ''' | [:] || ValidationErrorType.WrongType } - def "test graphql spec examples that output exception"() { + @Unroll + "test graphql spec examples that output exception : #testCase"() { def fetcher = new CapturingDataFetcher() def schema = TestUtil.schema(graphqlSpecExamples, ["Mutation": ["mutate": fetcher]]) @@ -185,7 +216,7 @@ class NullValueSupportTest extends Specification { where: - testCase | queryStr | variables || expectedException + testCase | queryStr | variables || expectedException // ------------------------------ 'G' | ''' @@ -194,7 +225,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || NonNullableValueCoercedAsNullException + ''' | [:] || NonNullableValueCoercedAsNullException // ------------------------------ 'H' | ''' @@ -203,7 +234,26 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: null] || NonNullableValueCoercedAsNullException + ''' | [var: null] || NonNullableValueCoercedAsNullException + + // ------------------------------ + 'M' | ''' + mutation mutate($var : ExampleInputObject) { + mutate(inputArg : $var) { + a + } + } + ''' | [var: [a: "abc", b: null]] || NonNullableValueCoercedAsNullException + + // ------------------------------ + 'N' | ''' + mutation mutate($var : ExampleInputObject) { + mutate(inputArg : $var) { + a + } + } + ''' | [var: [a: "abc"]] || NonNullableValueCoercedAsNullException + } From 5811228651dd86a1f73ac30ab763412079152236 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 28 May 2017 09:18:49 +1000 Subject: [PATCH 10/10] #427 - extra test for This unordered map should not contain any entries with names not defined by a field of this input object type, otherwise an error should be thrown. --- ...InputMapDefinesTooManyFieldsException.java | 18 +++++++++++++++++ .../graphql/execution/ValuesResolver.java | 11 +++++++++- .../graphql/NullValueSupportTest.groovy | 20 ++++++++++++++----- 3 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 src/main/java/graphql/execution/InputMapDefinesTooManyFieldsException.java diff --git a/src/main/java/graphql/execution/InputMapDefinesTooManyFieldsException.java b/src/main/java/graphql/execution/InputMapDefinesTooManyFieldsException.java new file mode 100644 index 0000000000..30bb61a3eb --- /dev/null +++ b/src/main/java/graphql/execution/InputMapDefinesTooManyFieldsException.java @@ -0,0 +1,18 @@ +package graphql.execution; + +import graphql.GraphQLException; +import graphql.schema.GraphQLType; +import graphql.schema.GraphQLTypeUtil; + +/** + * https://facebook.github.io/graphql/#sec-Input-Objects + * + * - This unordered map should not contain any entries with names not defined by a field of this input object type, otherwise an error should be thrown. + */ +public class InputMapDefinesTooManyFieldsException extends GraphQLException { + + public InputMapDefinesTooManyFieldsException(GraphQLType graphQLType, String fieldName) { + super(String.format("The variables input contains a field name '%s' that is not defined for input object type '%s' ", GraphQLTypeUtil.getUnwrappedTypeName(graphQLType), fieldName)); + } + +} diff --git a/src/main/java/graphql/execution/ValuesResolver.java b/src/main/java/graphql/execution/ValuesResolver.java index a9b74d0eb0..fe7be18ef1 100644 --- a/src/main/java/graphql/execution/ValuesResolver.java +++ b/src/main/java/graphql/execution/ValuesResolver.java @@ -25,6 +25,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; public class ValuesResolver { @@ -123,7 +124,15 @@ private Object coerceValue(GraphQLType graphQLType, Object value) { private Object coerceValueForInputObjectType(GraphQLInputObjectType inputObjectType, Map input) { Map result = new LinkedHashMap<>(); - for (GraphQLInputObjectField inputField : inputObjectType.getFields()) { + List fields = inputObjectType.getFields(); + List fieldNames = fields.stream().map(GraphQLInputObjectField::getName).collect(Collectors.toList()); + for (String inputFieldName : input.keySet()) { + if (!fieldNames.contains(inputFieldName)) { + throw new InputMapDefinesTooManyFieldsException(inputObjectType, inputFieldName); + } + } + + for (GraphQLInputObjectField inputField : fields) { if (input.containsKey(inputField.getName()) || alwaysHasValue(inputField)) { Object value = coerceValue(inputField.getType(), input.get(inputField.getName())); result.put(inputField.getName(), value == null ? inputField.getDefaultValue() : value); diff --git a/src/test/groovy/graphql/NullValueSupportTest.groovy b/src/test/groovy/graphql/NullValueSupportTest.groovy index ba34179bfb..08fc6f3b4b 100644 --- a/src/test/groovy/graphql/NullValueSupportTest.groovy +++ b/src/test/groovy/graphql/NullValueSupportTest.groovy @@ -1,5 +1,6 @@ package graphql +import graphql.execution.InputMapDefinesTooManyFieldsException import graphql.execution.NonNullableValueCoercedAsNullException import graphql.validation.ValidationError import graphql.validation.ValidationErrorType @@ -30,6 +31,7 @@ import spock.lang.Unroll L { $var } { b:123 } { b: 123 } M { $var } { a : "abc", b:null } Error: b must be non‐null. N { $var } { a : "abc" } Error: b must be non‐null. + O { $var } { a : "abc", b: 123, c:"xyz" } Error: c is not a valid field */ @@ -216,7 +218,7 @@ class NullValueSupportTest extends Specification { where: - testCase | queryStr | variables || expectedException + testCase | queryStr | variables || expectedException // ------------------------------ 'G' | ''' @@ -225,7 +227,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [:] || NonNullableValueCoercedAsNullException + ''' | [:] || NonNullableValueCoercedAsNullException // ------------------------------ 'H' | ''' @@ -234,7 +236,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: null] || NonNullableValueCoercedAsNullException + ''' | [var: null] || NonNullableValueCoercedAsNullException // ------------------------------ 'M' | ''' @@ -243,7 +245,7 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: [a: "abc", b: null]] || NonNullableValueCoercedAsNullException + ''' | [var: [a: "abc", b: null]] || NonNullableValueCoercedAsNullException // ------------------------------ 'N' | ''' @@ -252,8 +254,16 @@ class NullValueSupportTest extends Specification { a } } - ''' | [var: [a: "abc"]] || NonNullableValueCoercedAsNullException + ''' | [var: [a: "abc"]] || NonNullableValueCoercedAsNullException + // ------------------------------ + 'O' | ''' + mutation mutate($var : ExampleInputObject) { + mutate(inputArg : $var) { + a + } + } + ''' | [var: [a: "abc", b: 123, c: "xyz"]] || InputMapDefinesTooManyFieldsException }