Skip to content

Commit 26a012c

Browse files
dimdimychDmitry Kudryavtsev
dimdimych
and
Dmitry Kudryavtsev
authored
Proposed fix for graphql-java#2001 Allow non-list values on schema directive arguments of list type (graphql-java#2002)
* Allow non-list values on schema directive arguments of list type * graphql-java#2001 check if one dimension array is passed as a value for multi-dimension array Co-authored-by: Dmitry Kudryavtsev <dimak@versonix.com>
1 parent 2ed520e commit 26a012c

4 files changed

Lines changed: 92 additions & 10 deletions

File tree

‎src/main/java/graphql/schema/idl/ArgValueOfAllowedTypeChecker.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,16 +246,24 @@ private void checkArgValueMatchesAllowedListType(List<GraphQLError> errors, Valu
246246
return;
247247
}
248248

249+
250+
Type unwrappedAllowedType = allowedArgType.getType();
249251
if (!(instanceValue instanceof ArrayValue)) {
250-
addValidationError(errors, EXPECTED_LIST_MESSAGE, instanceValue.getClass().getSimpleName());
252+
checkArgValueMatchesAllowedType(errors, instanceValue, unwrappedAllowedType);
251253
return;
252254
}
253255

254256
ArrayValue arrayValue = ((ArrayValue) instanceValue);
255-
Type unwrappedAllowedType = allowedArgType.getType();
257+
boolean isUnwrappedList = unwrappedAllowedType instanceof ListType;
256258

257259
// validate each instance value in the list, all instances must match for the list to match
258-
arrayValue.getValues().forEach(value -> checkArgValueMatchesAllowedType(errors, value, unwrappedAllowedType));
260+
arrayValue.getValues().forEach(value -> {
261+
// restrictive check for sub-arrays
262+
if (isUnwrappedList && ! (value instanceof ArrayValue)) {
263+
addValidationError(errors, EXPECTED_LIST_MESSAGE, value.getClass().getSimpleName());
264+
}
265+
checkArgValueMatchesAllowedType(errors, value, unwrappedAllowedType);
266+
});
259267
}
260268

261269
private boolean isArgumentValueScalarLiteral(GraphQLScalarType scalarType, Value instanceValue) {

‎src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import graphql.schema.GraphQLEnumType;
2323
import graphql.schema.GraphQLInputObjectType;
2424
import graphql.schema.GraphQLInputType;
25+
import graphql.schema.GraphQLList;
2526
import graphql.schema.GraphQLScalarType;
2627
import graphql.schema.GraphQLType;
2728
import graphql.schema.GraphQLTypeUtil;
@@ -49,6 +50,7 @@
4950
import static graphql.schema.GraphQLTypeUtil.unwrapOne;
5051
import static java.util.Collections.emptyList;
5152
import static java.util.stream.Collectors.joining;
53+
import static java.util.stream.Collectors.reducing;
5254
import static java.util.stream.Collectors.toList;
5355
import static java.util.stream.Collectors.toMap;
5456

@@ -99,7 +101,7 @@ public Object buildValue(Value value, GraphQLType requiredType) {
99101
if (GraphQLTypeUtil.isNonNull(requiredType)) {
100102
requiredType = unwrapOne(requiredType);
101103
}
102-
if (value == null) {
104+
if (value == null || value instanceof NullValue) {
103105
return null;
104106
}
105107
if (requiredType instanceof GraphQLScalarType) {
@@ -108,11 +110,15 @@ public Object buildValue(Value value, GraphQLType requiredType) {
108110
result = ((EnumValue) value).getName();
109111
} else if (requiredType instanceof GraphQLEnumType && value instanceof StringValue) {
110112
result = ((StringValue) value).getValue();
111-
} else if (value instanceof ArrayValue && isList(requiredType)) {
112-
result = buildArrayValue(requiredType, (ArrayValue) value);
113+
} else if (isList(requiredType)) {
114+
if (value instanceof ArrayValue) {
115+
result = buildArrayValue(requiredType, (ArrayValue) value);
116+
} else {
117+
result = buildArrayValue(requiredType, ArrayValue.newArrayValue().value(value).build());
118+
}
113119
} else if (value instanceof ObjectValue && requiredType instanceof GraphQLInputObjectType) {
114120
result = buildObjectValue((ObjectValue) value, (GraphQLInputObjectType) requiredType);
115-
} else if (!(value instanceof NullValue)) {
121+
} else {
116122
assertShouldNeverHappen(
117123
"cannot build value of type %s from object class %s with instance %s", simplePrint(requiredType), value.getClass().getSimpleName(), String.valueOf(value));
118124
}

‎src/test/groovy/graphql/Issue2001.groovy‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package graphql
2+
3+
import graphql.language.ArrayValue
4+
import graphql.schema.CoercingParseLiteralException
5+
import graphql.schema.idl.RuntimeWiring
6+
import graphql.schema.idl.SchemaGenerator
7+
import graphql.schema.idl.SchemaParser
8+
import graphql.schema.idl.errors.SchemaProblem
9+
import spock.lang.Specification
10+
11+
class Issue2001 extends Specification {
12+
13+
def "test non-list value for a list argument of a directive"() {
14+
def spec = '''
15+
directive @test(value: [String] = "default") on FIELD_DEFINITION
16+
type Query {
17+
testDefaultWorks : String @test
18+
testItWorks : String @test(value: "test")
19+
testItIsNotBroken : String @test(value: ["test"])
20+
}
21+
'''
22+
23+
def closure = {
24+
return it.fieldDefinition
25+
.getDirective("test")
26+
.getArgument("value")
27+
.value[0]
28+
}
29+
def graphql = TestUtil.graphQL(spec, RuntimeWiring.newRuntimeWiring()
30+
.type("Query", {
31+
it.dataFetcher("testDefaultWorks", closure)
32+
.dataFetcher("testItWorks", closure)
33+
.dataFetcher("testItIsNotBroken", closure)
34+
}).build())
35+
.build()
36+
37+
when:
38+
def result = graphql.execute(' { testDefaultWorks testItWorks testItIsNotBroken }')
39+
40+
then:
41+
result.errors.isEmpty()
42+
result.data.testDefaultWorks == "default"
43+
result.data.testItWorks == "test"
44+
result.data.testItIsNotBroken == "test"
45+
}
46+
def "test an incorrect non-list value for a list argument of a directive"() {
47+
def spec = '''
48+
directive @test(value: [String]) on FIELD_DEFINITION
49+
type Query {
50+
test : String @test(value : 123)
51+
}
52+
'''
53+
54+
55+
when:
56+
def reader = new StringReader(spec)
57+
def registry = new SchemaParser().parse(reader)
58+
59+
def options = SchemaGenerator.Options.defaultOptions()
60+
61+
def schema = new SchemaGenerator().makeExecutableSchema(options, registry, TestUtil.mockRuntimeWiring)
62+
63+
then:
64+
thrown(SchemaProblem.class)
65+
}
66+
}

‎src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1501,15 +1501,17 @@ class SchemaTypeCheckerTest extends Specification {
15011501
"String" | '{ an: "object" }' | format(EXPECTED_SCALAR_MESSAGE, "ObjectValue")
15021502
"String" | '["str", "str2"]' | format(EXPECTED_SCALAR_MESSAGE, "ArrayValue")
15031503
"ACustomDate" | '"AFailingDate"' | format(NOT_A_VALID_SCALAR_LITERAL_MESSAGE, "ACustomDate")
1504-
"[String]" | '"str"' | format(EXPECTED_LIST_MESSAGE, "StringValue")
1505-
"[String]!" | '"str"' | format(EXPECTED_LIST_MESSAGE, "StringValue")
1504+
// Now allowed by #2001
1505+
// "[String]" | '"str"' | format(EXPECTED_LIST_MESSAGE, "StringValue")
1506+
// "[String]!" | '"str"' | format(EXPECTED_LIST_MESSAGE, "StringValue")
15061507
"[String!]" | '["str", null]' | format(EXPECTED_NON_NULL_MESSAGE)
15071508
"[[String!]!]" | '[["str"], ["str2", null]]' | format(EXPECTED_NON_NULL_MESSAGE)
15081509
"WEEKDAY" | '"somestr"' | format(EXPECTED_ENUM_MESSAGE, "StringValue")
15091510
"WEEKDAY" | 'SATURDAY' | format(MUST_BE_VALID_ENUM_VALUE_MESSAGE, "SATURDAY", "MONDAY,TUESDAY")
15101511
"UserInput" | '{ fieldNonNull: "str", fieldNonNull: "dupeKey" }' | format(DUPLICATED_KEYS_MESSAGE, "fieldNonNull")
15111512
"UserInput" | '{ fieldNonNull: "str", unknown: "field" }' | format(UNKNOWN_FIELDS_MESSAGE, "unknown", "UserInput")
1512-
"UserInput" | '{ fieldNonNull: "str", fieldArray: "strInsteadOfArray" }' | format(EXPECTED_LIST_MESSAGE, "StringValue")
1513+
// Now allowed by #2001
1514+
// "UserInput" | '{ fieldNonNull: "str", fieldArray: "strInsteadOfArray" }' | format(EXPECTED_LIST_MESSAGE, "StringValue")
15131515
"UserInput" | '{ fieldNonNull: "str", fieldArrayOfArray: ["ArrayInsteadOfArrayOfArray"] }' | format(EXPECTED_LIST_MESSAGE, "StringValue")
15141516
"UserInput" | '{ fieldNonNull: "str", fieldNestedInput: "strInsteadOfObject" }' | format(EXPECTED_OBJECT_MESSAGE, "StringValue")
15151517
"UserInput" | '{ fieldNonNull: "str", fieldNestedInput: { street: { s: "objectInsteadOfString" }} }' | format(EXPECTED_SCALAR_MESSAGE, "ObjectValue")

0 commit comments

Comments
 (0)