From 2c42086f895173ce965dcdb922d36523d10ee01e Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 28 Apr 2024 17:21:45 +1000 Subject: [PATCH 1/2] This will validate @oneOf variable values when the variables are coerced --- .../execution/ValuesResolverConversion.java | 32 +- .../execution/ValuesResolverTest.groovy | 294 +++++++++++------- 2 files changed, 209 insertions(+), 117 deletions(-) diff --git a/src/main/java/graphql/execution/ValuesResolverConversion.java b/src/main/java/graphql/execution/ValuesResolverConversion.java index eff4d1cef1..66b84bb198 100644 --- a/src/main/java/graphql/execution/ValuesResolverConversion.java +++ b/src/main/java/graphql/execution/ValuesResolverConversion.java @@ -374,6 +374,7 @@ static CoercedVariables externalValueToInternalValueForVariables( coercedValues.put(variableName, null); } else { Object coercedValue = externalValueToInternalValueImpl( + variableName, inputInterceptor, fieldVisibility, variableInputType, @@ -398,11 +399,28 @@ static CoercedVariables externalValueToInternalValueForVariables( return CoercedVariables.of(coercedValues); } + static Object externalValueToInternalValueImpl( + InputInterceptor inputInterceptor, + GraphqlFieldVisibility fieldVisibility, + GraphQLInputType graphQLType, + Object originalValue, + GraphQLContext graphqlContext, + Locale locale + ) throws NonNullableValueCoercedAsNullException, CoercingParseValueException { + return externalValueToInternalValueImpl("externalValue", + inputInterceptor, + fieldVisibility, + graphQLType, + originalValue, + graphqlContext, + locale); + } + /** * Performs validation too */ - @SuppressWarnings("unchecked") static Object externalValueToInternalValueImpl( + String variableName, InputInterceptor inputInterceptor, GraphqlFieldVisibility fieldVisibility, GraphQLInputType graphQLType, @@ -412,6 +430,7 @@ static Object externalValueToInternalValueImpl( ) throws NonNullableValueCoercedAsNullException, CoercingParseValueException { if (isNonNull(graphQLType)) { Object returnValue = externalValueToInternalValueImpl( + variableName, inputInterceptor, fieldVisibility, unwrapOneAs(graphQLType), @@ -458,13 +477,18 @@ static Object externalValueToInternalValueImpl( locale); } else if (graphQLType instanceof GraphQLInputObjectType) { if (value instanceof Map) { - return externalValueToInternalValueForObject( + GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) graphQLType; + //noinspection unchecked + Map coercedMap = externalValueToInternalValueForObject( inputInterceptor, fieldVisibility, - (GraphQLInputObjectType) graphQLType, + inputObjectType, (Map) value, graphqlContext, locale); + + ValuesResolverOneOfValidation.validateOneOfInputTypes(inputObjectType, coercedMap, null, variableName, locale); + return coercedMap; } else { throw CoercingParseValueException.newCoercingParseValueException() .message("Expected type 'Map' but was '" + value.getClass().getSimpleName() + @@ -479,7 +503,7 @@ static Object externalValueToInternalValueImpl( /** * performs validation */ - private static Object externalValueToInternalValueForObject( + private static Map externalValueToInternalValueForObject( InputInterceptor inputInterceptor, GraphqlFieldVisibility fieldVisibility, GraphQLInputObjectType inputObjectType, diff --git a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy index 1a7aa22b3e..97af997047 100644 --- a/src/test/groovy/graphql/execution/ValuesResolverTest.groovy +++ b/src/test/groovy/graphql/execution/ValuesResolverTest.groovy @@ -23,9 +23,11 @@ import graphql.language.Value import graphql.language.VariableDefinition import graphql.language.VariableReference import graphql.schema.CoercingParseValueException +import graphql.schema.DataFetcher import spock.lang.Specification import spock.lang.Unroll +import static graphql.ExecutionInput.newExecutionInput import static graphql.Scalars.GraphQLBoolean import static graphql.Scalars.GraphQLFloat import static graphql.Scalars.GraphQLInt @@ -88,6 +90,72 @@ class ValuesResolverTest extends Specification { [name: 'x'] || [name: 'x'] } + def "getVariableValues: @oneOf map object as variable input"() { + given: + def aField = newInputObjectField() + .name("a") + .type(GraphQLString) + def bField = newInputObjectField() + .name("b") + .type(GraphQLString) + def inputType = newInputObject() + .name("Person") + .withAppliedDirective(Directives.OneOfDirective.toAppliedDirective()) + .field(aField) + .field(bField) + .build() + def schema = TestUtil.schemaWithInputType(inputType) + VariableDefinition variableDefinition = new VariableDefinition("variable", new TypeName("Person")) + + when: + def resolvedValues = ValuesResolver.coerceVariableValues(schema, [variableDefinition], RawVariables.of([variable: [a: 'x']]), graphQLContext, locale) + then: + resolvedValues.get('variable') == [a: 'x'] + + when: + resolvedValues = ValuesResolver.coerceVariableValues(schema, [variableDefinition], RawVariables.of([variable: [b: 'y']]), graphQLContext, locale) + then: + resolvedValues.get('variable') == [b: 'y'] + + when: + ValuesResolver.coerceVariableValues(schema, [variableDefinition], RawVariables.of([variable: [a: 'x', b: 'y']]), graphQLContext, locale) + then: + thrown(OneOfTooManyKeysException.class) + } + + def "can validate inner input oneOf fields"() { + // + // a test from https://github.com/graphql-java/graphql-java/issues/3572 + // + def sdl = ''' + input OneOf @oneOf { a: Int, b: Int } + type Outer { inner(oneof: OneOf!): Boolean } + type Query { outer: Outer } + ''' + + DataFetcher outer = { env -> return null } + def graphQL = TestUtil.graphQL(sdl, [Query: [outer: outer]]).build() + + def query = ''' + query ($oneof: OneOf!) { + outer { + # these variables are never accessed by a data fetcher because + # Query.outer always returns null + inner(oneof: $oneof) + } + } + ''' + + when: + def er = graphQL.execute( + newExecutionInput(query).variables([oneof: [a: 2, b: 1]]) + ) + + then: + er.errors.size() == 1 + er.errors[0].message == "Exactly one key must be specified for OneOf type 'OneOf'." + } + class Person { def name = "" @@ -460,59 +528,59 @@ class ValuesResolverTest extends Specification { e.message == "Exactly one key must be specified for OneOf type 'OneOfInputObject'." where: - testCase | inputValue | variables + testCase | inputValue | variables '{oneOfField: {a: "abc", b: 123} } {}' | buildObjectLiteral([ oneOfField: [ a: StringValue.of("abc"), b: IntValue.of(123) ] - ]) | CoercedVariables.emptyVariables() + ]) | CoercedVariables.emptyVariables() '{oneOfField: {a: null, b: 123 }} {}' | buildObjectLiteral([ oneOfField: [ a: NullValue.of(), b: IntValue.of(123) ] - ]) | CoercedVariables.emptyVariables() + ]) | CoercedVariables.emptyVariables() '{oneOfField: {a: $var, b: 123 }} { var: null }' | buildObjectLiteral([ oneOfField: [ a: VariableReference.of("var"), b: IntValue.of(123) ] - ]) | CoercedVariables.of(["var": null]) + ]) | CoercedVariables.of(["var": null]) '{oneOfField: {a: $var, b: 123 }} {}' | buildObjectLiteral([ oneOfField: [ a: VariableReference.of("var"), b: IntValue.of(123) ] - ]) | CoercedVariables.emptyVariables() + ]) | CoercedVariables.emptyVariables() '{oneOfField: {a : "abc", b : null}} {}' | buildObjectLiteral([ oneOfField: [ a: StringValue.of("abc"), b: NullValue.of() ] - ]) | CoercedVariables.emptyVariables() + ]) | CoercedVariables.emptyVariables() '{oneOfField: {a : null, b : null}} {}' | buildObjectLiteral([ oneOfField: [ a: NullValue.of(), b: NullValue.of() ] - ]) | CoercedVariables.emptyVariables() + ]) | CoercedVariables.emptyVariables() '{oneOfField: {a : $a, b : $b}} {a : "abc"}' | buildObjectLiteral([ oneOfField: [ a: VariableReference.of("a"), b: VariableReference.of("v") ] - ]) | CoercedVariables.of(["a": "abc"]) + ]) | CoercedVariables.of(["a": "abc"]) '$var {var : {oneOfField: { a : "abc", b : 123}}}' | VariableReference.of("var") - | CoercedVariables.of(["var": ["oneOfField": ["a": "abc", "b": 123]]]) + | CoercedVariables.of(["var": ["oneOfField": ["a": "abc", "b": 123]]]) - '$var {var : {oneOfField: {} }}' | VariableReference.of("var") - | CoercedVariables.of(["var": ["oneOfField": [:]]]) + '$var {var : {oneOfField: {} }}' | VariableReference.of("var") + | CoercedVariables.of(["var": ["oneOfField": [:]]]) } @@ -600,7 +668,7 @@ class ValuesResolverTest extends Specification { a: VariableReference.of("var") ]) | CoercedVariables.of(["var": null]) - '`{ a: $var }` { }' | buildObjectLiteral([ + '`{ a: $var }` { }' | buildObjectLiteral([ a: VariableReference.of("var") ]) | CoercedVariables.emptyVariables() } @@ -631,38 +699,38 @@ class ValuesResolverTest extends Specification { where: - testCase | inputArray | variables + testCase | inputArray | variables '[{ a: "abc", b: 123 }]' - | ArrayValue.newArrayValue() - .value(buildObjectLiteral([ - a: StringValue.of("abc"), - b: IntValue.of(123) - ])).build() - | CoercedVariables.emptyVariables() + | ArrayValue.newArrayValue() + .value(buildObjectLiteral([ + a: StringValue.of("abc"), + b: IntValue.of(123) + ])).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, { a: "xyz", b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - buildObjectLiteral([ + ]), + buildObjectLiteral([ a: StringValue.of("xyz"), b: IntValue.of(789) - ]), - ]).build() - | CoercedVariables.emptyVariables() + ]), + ]).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, $var ] [{ a: "abc" }, { a: "xyz", b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - VariableReference.of("var") - ]).build() - | CoercedVariables.of("var": [a: "xyz", b: 789]) + ]), + VariableReference.of("var") + ]).build() + | CoercedVariables.of("var": [a: "xyz", b: 789]) } @@ -692,31 +760,31 @@ class ValuesResolverTest extends Specification { where: - testCase | inputArray | variables + testCase | inputArray | variables '[{ a: "abc" }, { a: null }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - buildObjectLiteral([ + ]), + buildObjectLiteral([ a: NullValue.of() - ]), - ]).build() - | CoercedVariables.emptyVariables() + ]), + ]).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, { a: $var }] [{ a: "abc" }, { a: null }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - buildObjectLiteral([ + ]), + buildObjectLiteral([ a: VariableReference.of("var") - ]), - ]).build() - | CoercedVariables.of("var": null) + ]), + ]).build() + | CoercedVariables.of("var": null) } @@ -746,38 +814,38 @@ class ValuesResolverTest extends Specification { where: - testCase | inputArray | variables + testCase | inputArray | variables '[{ a: "abc", b: 123 }]' - | ArrayValue.newArrayValue() - .value(buildObjectLiteral([ - a: StringValue.of("abc"), - b: IntValue.of(123) - ])).build() - | CoercedVariables.emptyVariables() + | ArrayValue.newArrayValue() + .value(buildObjectLiteral([ + a: StringValue.of("abc"), + b: IntValue.of(123) + ])).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, { a: "xyz", b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - buildObjectLiteral([ + ]), + buildObjectLiteral([ a: StringValue.of("xyz"), b: IntValue.of(789) - ]), - ]).build() - | CoercedVariables.emptyVariables() + ]), + ]).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, $var ] [{ a: "abc" }, { a: "xyz", b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - VariableReference.of("var") - ]).build() - | CoercedVariables.of("var": [a: "xyz", b: 789]) + ]), + VariableReference.of("var") + ]).build() + | CoercedVariables.of("var": [a: "xyz", b: 789]) } @@ -807,38 +875,38 @@ class ValuesResolverTest extends Specification { where: - testCase | inputArray | variables + testCase | inputArray | variables '[{ a: "abc", b: 123 }]' - | ArrayValue.newArrayValue() - .value(buildObjectLiteral([ - a: StringValue.of("abc"), - b: IntValue.of(123) - ])).build() - | CoercedVariables.emptyVariables() + | ArrayValue.newArrayValue() + .value(buildObjectLiteral([ + a: StringValue.of("abc"), + b: IntValue.of(123) + ])).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, { a: "xyz", b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - buildObjectLiteral([ + ]), + buildObjectLiteral([ a: StringValue.of("xyz"), b: IntValue.of(789) - ]), - ]).build() - | CoercedVariables.emptyVariables() + ]), + ]).build() + | CoercedVariables.emptyVariables() '[{ a: "abc" }, $var ] [{ a: "abc" }, { a: "xyz", b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - VariableReference.of("var") - ]).build() - | CoercedVariables.of("var": [a: "xyz", b: 789]) + ]), + VariableReference.of("var") + ]).build() + | CoercedVariables.of("var": [a: "xyz", b: 789]) } @@ -915,26 +983,26 @@ class ValuesResolverTest extends Specification { where: - testCase | inputArray | variables | expectedValues + testCase | inputArray | variables | expectedValues '[{ a: "abc"}]' - | ArrayValue.newArrayValue() - .value(buildObjectLiteral([ - a: StringValue.of("abc"), - ])).build() - | CoercedVariables.emptyVariables() - | [arg: [[a: "abc"]]] + | ArrayValue.newArrayValue() + .value(buildObjectLiteral([ + a: StringValue.of("abc"), + ])).build() + | CoercedVariables.emptyVariables() + | [arg: [[a: "abc"]]] '[{ a: "abc" }, $var ] [{ a: "abc" }, { b: 789 }]' - | ArrayValue.newArrayValue() - .values([ - buildObjectLiteral([ + | ArrayValue.newArrayValue() + .values([ + buildObjectLiteral([ a: StringValue.of("abc") - ]), - VariableReference.of("var") - ]).build() - | CoercedVariables.of("var": [b: 789]) - | [arg: [[a: "abc"], [b: 789]]] + ]), + VariableReference.of("var") + ]).build() + | CoercedVariables.of("var": [b: 789]) + | [arg: [[a: "abc"], [b: 789]]] } @@ -1223,7 +1291,7 @@ class ValuesResolverTest extends Specification { } ''' - def executionInput = ExecutionInput.newExecutionInput() + def executionInput = newExecutionInput() .query(mutation) .variables([input: [name: 'Name', position: 'UNKNOWN_POSITION']]) .build() @@ -1261,7 +1329,7 @@ class ValuesResolverTest extends Specification { } ''' - def executionInput = ExecutionInput.newExecutionInput() + def executionInput = newExecutionInput() .query(mutation) .variables([input: [name: 'Name', hilarious: 'sometimes']]) .build() @@ -1299,7 +1367,7 @@ class ValuesResolverTest extends Specification { } ''' - def executionInput = ExecutionInput.newExecutionInput() + def executionInput = newExecutionInput() .query(mutation) .variables([input: [name: 'Name', laughsPerMinute: 'none']]) .build() From 6044ec401544d64205c5d45c894faa4a1d552c08 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 28 Apr 2024 17:43:30 +1000 Subject: [PATCH 2/2] This will validate @oneOf variable values when the variables are coerced - fixed tests --- .../groovy/graphql/schema/GraphQLInputObjectTypeTest.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/groovy/graphql/schema/GraphQLInputObjectTypeTest.groovy b/src/test/groovy/graphql/schema/GraphQLInputObjectTypeTest.groovy index 09ed18b731..94b0dd1782 100644 --- a/src/test/groovy/graphql/schema/GraphQLInputObjectTypeTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLInputObjectTypeTest.groovy @@ -169,14 +169,14 @@ class GraphQLInputObjectTypeTest extends Specification { er = graphQL.execute(ei) then: !er.errors.isEmpty() - er.errors[0].message == "Exception while fetching data (/f) : Exactly one key must be specified for OneOf type 'OneOf'." + er.errors[0].message == "Exactly one key must be specified for OneOf type 'OneOf'." when: ei = ExecutionInput.newExecutionInput('query q($var : OneOf) { f( arg : $var) { key value }}').variables([var: [a: null]]).build() er = graphQL.execute(ei) then: !er.errors.isEmpty() - er.errors[0].message == "Exception while fetching data (/f) : OneOf type field 'OneOf.a' must be non-null." + er.errors[0].message == "OneOf type field 'OneOf.a' must be non-null." // lots more covered in unit tests }