Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OneOf validation not being applied to nested inputs #3365

Merged
merged 4 commits into from
Oct 30, 2023
Merged

OneOf validation not being applied to nested inputs #3365

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72621ec
Add test that reproduces the bug
felipe-gdr-atlassian Oct 25, 2023
f1dd89e
Apply oneOf validation to nested input fields
felipe-gdr-atlassian Oct 26, 2023
1885437
Remove comment
felipe-gdr-atlassian Oct 26, 2023
904fd1e
Add @Internal to new class
felipe-gdr-atlassian Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 5 additions & 32 deletions src/main/java/graphql/execution/ValuesResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,41 +376,14 @@ private static Map<String, Object> getArgumentValuesImpl(
locale);
coercedValues.put(argumentName, value);
}
// @oneOf input must be checked now that all variables and literals have been converted
GraphQLType unwrappedType = GraphQLTypeUtil.unwrapNonNull(argumentType);
if (unwrappedType instanceof GraphQLInputObjectType) {
GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) unwrappedType;
if (inputObjectType.isOneOf() && ! ValuesResolverConversion.isNullValue(value)) {
validateOneOfInputTypes(inputObjectType, argumentValue, argumentName, value, locale);
}
}
}

}
return coercedValues;
}
ValuesResolverOneOfValidation.validateOneOfInputTypes(argumentType, value, argumentValue, argumentName, locale);

@SuppressWarnings("unchecked")
private static void validateOneOfInputTypes(GraphQLInputObjectType oneOfInputType, Value argumentValue, String argumentName, Object inputValue, Locale locale) {
Assert.assertTrue(inputValue instanceof Map, () -> String.format("The coerced argument %s GraphQLInputObjectType is unexpectedly not a map", argumentName));
Map<String, Object> objectMap = (Map<String, Object>) inputValue;
int mapSize;
if (argumentValue instanceof ObjectValue) {
mapSize = ((ObjectValue) argumentValue).getObjectFields().size();
} else {
mapSize = objectMap.size();
}
if (mapSize != 1) {
String msg = I18n.i18n(I18n.BundleType.Execution, locale)
.msg("Execution.handleOneOfNotOneFieldError", oneOfInputType.getName());
throw new OneOfTooManyKeysException(msg);
}
String fieldName = objectMap.keySet().iterator().next();
if (objectMap.get(fieldName) == null) {
String msg = I18n.i18n(I18n.BundleType.Execution, locale)
.msg("Execution.handleOneOfValueIsNullError", oneOfInputType.getName() + "." + fieldName);
throw new OneOfNullValueException(msg);
}
}


return coercedValues;
}

private static Map<String, Argument> argumentMap(List<Argument> arguments) {
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/graphql/execution/ValuesResolverConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ static Object valueToLiteralImpl(GraphqlFieldVisibility fieldVisibility,
* @param type the type of input value
* @param graphqlContext the GraphqlContext to use
* @param locale the Locale to use
*
* @return a value converted to an internal value
*/
static Object externalValueToInternalValue(GraphqlFieldVisibility fieldVisibility,
Expand Down Expand Up @@ -596,7 +595,6 @@ private static List externalValueToInternalValueForList(
* @param coercedVariables the coerced variable values
* @param graphqlContext the GraphqlContext to use
* @param locale the Locale to use
*
* @return literal converted to an internal value
*/
static Object literalToInternalValue(
Expand Down
78 changes: 78 additions & 0 deletions src/main/java/graphql/execution/ValuesResolverOneOfValidation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package graphql.execution;

import graphql.Assert;
import graphql.i18n.I18n;
import graphql.language.ObjectField;
import graphql.language.ObjectValue;
import graphql.language.Value;
import graphql.schema.GraphQLInputObjectField;
import graphql.schema.GraphQLInputObjectType;
import graphql.schema.GraphQLInputType;
import graphql.schema.GraphQLType;
import graphql.schema.GraphQLTypeUtil;

import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;

final class ValuesResolverOneOfValidation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Internal please - albeit its package scoped but still

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ;-)


@SuppressWarnings("unchecked")
static void validateOneOfInputTypes(GraphQLType type, Object inputValue, Value<?> argumentValue, String argumentName, Locale locale) {
GraphQLType unwrappedType = GraphQLTypeUtil.unwrapNonNull(type);

if (unwrappedType instanceof GraphQLInputObjectType && !ValuesResolverConversion.isNullValue(inputValue)) {
Assert.assertTrue(inputValue instanceof Map, () -> String.format("The coerced argument %s GraphQLInputObjectType is unexpectedly not a map", argumentName));
Map<String, Object> objectMap = (Map<String, Object>) inputValue;

GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) unwrappedType;

if (inputObjectType.isOneOf()) {
validateOneOfInputTypesInternal(inputObjectType, argumentValue, objectMap, locale);
}

for (GraphQLInputObjectField fieldDefinition : inputObjectType.getFields()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the new code is in this for block.
Here is where I navigate to child fields to apply validation on them.

GraphQLInputType childFieldType = fieldDefinition.getType();
String childFieldName = fieldDefinition.getName();
Object childFieldInputValue = objectMap.get(childFieldName);

if (argumentValue instanceof ObjectValue) {
List<Value> values = ((ObjectValue) argumentValue).getObjectFields().stream()
.filter(of -> of.getName().equals(childFieldName))
.map(ObjectField::getValue)
.collect(Collectors.toList());

if (values.size() > 1) {
Assert.assertShouldNeverHappen(String.format("argument %s has %s object fields with the same name: '%s'. A maximum of 1 is expected", argumentName, values.size(), childFieldName));
} else if (!values.isEmpty()) {
validateOneOfInputTypes(childFieldType, childFieldInputValue, values.get(0), argumentName, locale);
}
} else {
validateOneOfInputTypes(childFieldType, childFieldInputValue, argumentValue, argumentName, locale);
}
}
}
}

private static void validateOneOfInputTypesInternal(GraphQLInputObjectType oneOfInputType, Value<?> argumentValue, Map<String, Object> objectMap, Locale locale) {
int mapSize;

if (argumentValue instanceof ObjectValue) {
mapSize = ((ObjectValue) argumentValue).getObjectFields().size();
} else {
mapSize = objectMap.size();
}
if (mapSize != 1) {
String msg = I18n.i18n(I18n.BundleType.Execution, locale)
.msg("Execution.handleOneOfNotOneFieldError", oneOfInputType.getName());
throw new OneOfTooManyKeysException(msg);
}
String fieldName = objectMap.keySet().iterator().next();
if (objectMap.get(fieldName) == null) {
String msg = I18n.i18n(I18n.BundleType.Execution, locale)
.msg("Execution.handleOneOfValueIsNullError", oneOfInputType.getName() + "." + fieldName);
throw new OneOfNullValueException(msg);
}
}
}
138 changes: 138 additions & 0 deletions src/test/groovy/graphql/execution/ValuesResolverTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,144 @@ class ValuesResolverTest extends Specification {
| CoercedVariables.of(["var": [:]])
}

def "getArgumentValues: invalid oneOf nested input because of duplicate keys - #testCase"() {
given: "schema defining input object"
def oneOfObjectType = newInputObject()
.name("OneOfInputObject")
.withAppliedDirective(Directives.OneOfDirective.toAppliedDirective())
.field(newInputObjectField()
.name("a")
.type(GraphQLString)
.build())
.field(newInputObjectField()
.name("b")
.type(GraphQLInt)
.build())
.build()

def parentObjectType = newInputObject()
.name("ParentInputObject")
.field(newInputObjectField()
.name("oneOfField")
.type(oneOfObjectType)
.build())
.build()

def argument = new Argument("arg", inputValue)

when:
def fieldArgument = newArgument().name("arg").type(parentObjectType).build()
ValuesResolver.getArgumentValues([fieldArgument], [argument], variables, graphQLContext, locale)

then:
def e = thrown(OneOfTooManyKeysException)
e.message == "Exactly one key must be specified for OneOf type 'OneOfInputObject'."

where:
testCase | inputValue | variables
'{oneOfField: {a: "abc", b: 123} } {}' | buildObjectLiteral([
oneOfField: [
a: StringValue.of("abc"),
b: IntValue.of(123)
]
]) | CoercedVariables.emptyVariables()
'{oneOfField: {a: null, b: 123 }} {}' | buildObjectLiteral([
oneOfField: [
a: NullValue.of(),
b: IntValue.of(123)
]
]) | CoercedVariables.emptyVariables()

'{oneOfField: {a: $var, b: 123 }} { var: null }' | buildObjectLiteral([
oneOfField: [
a: VariableReference.of("var"),
b: IntValue.of(123)
]
]) | CoercedVariables.of(["var": null])

'{oneOfField: {a: $var, b: 123 }} {}' | buildObjectLiteral([
oneOfField: [
a: VariableReference.of("var"),
b: IntValue.of(123)
]
]) | CoercedVariables.emptyVariables()

'{oneOfField: {a : "abc", b : null}} {}' | buildObjectLiteral([
oneOfField: [
a: StringValue.of("abc"),
b: NullValue.of()
]
]) | CoercedVariables.emptyVariables()

'{oneOfField: {a : null, b : null}} {}' | buildObjectLiteral([
oneOfField: [
a: NullValue.of(),
b: NullValue.of()
]
]) | CoercedVariables.emptyVariables()

'{oneOfField: {a : $a, b : $b}} {a : "abc"}' | buildObjectLiteral([
oneOfField: [
a: VariableReference.of("a"),
b: VariableReference.of("v")
]
]) | CoercedVariables.of(["a": "abc"])
'$var {var : {oneOfField: { a : "abc", b : 123}}}' | VariableReference.of("var")
| CoercedVariables.of(["var": ["oneOfField": ["a": "abc", "b": 123]]])

'$var {var : {oneOfField: {} }}' | VariableReference.of("var")
| CoercedVariables.of(["var": ["oneOfField": [:]]])

}

def "getArgumentValues: invalid oneOf nested input because of null value - #testCase"() {
given: "schema defining input object"
def oneOfObjectType = newInputObject()
.name("OneOfInputObject")
.withAppliedDirective(Directives.OneOfDirective.toAppliedDirective())
.field(newInputObjectField()
.name("a")
.type(GraphQLString)
.build())
.field(newInputObjectField()
.name("b")
.type(GraphQLInt)
.build())
.build()

def parentObjectType = newInputObject()
.name("ParentInputObject")
.field(newInputObjectField()
.name("oneOfField")
.type(oneOfObjectType)
.build())
.build()


def fieldArgument = newArgument().name("arg").type(parentObjectType).build()

when:
def argument = new Argument("arg", inputValue)
ValuesResolver.getArgumentValues([fieldArgument], [argument], variables, graphQLContext, locale)

then:
def e = thrown(OneOfNullValueException)
e.message == "OneOf type field 'OneOfInputObject.a' must be non-null."

where:
// from https://github.com/graphql/graphql-spec/pull/825/files#diff-30a69c5a5eded8e1aea52e53dad1181e6ec8f549ca2c50570b035153e2de1c43R1692
testCase | inputValue | variables

'`{ oneOfField: { a: null }}` {}' | buildObjectLiteral([
oneOfField: [a: NullValue.of()]
]) | CoercedVariables.emptyVariables()

'`{ oneOfField: { a: $var }}` { var : null}' | buildObjectLiteral([
oneOfField: [a: VariableReference.of("var")]
]) | CoercedVariables.of(["var": null])

}

def "getArgumentValues: invalid oneOf input because of null value - #testCase"() {
given: "schema defining input object"
def inputObjectType = newInputObject()
Expand Down