Skip to content

Commit 4f04eff

Browse files
dondonzdondonz
authored
Merge pull request #3310 from graphql-java/19.x-backport-of-3278-default-value
19.x backport of #3278 default value bugfix
2 parents 3e7f0e8 + 23a05a8 commit 4f04eff

File tree

8 files changed

+336
-41
lines changed

8 files changed

+336
-41
lines changed

‎src/main/java/graphql/validation/TraversalContext.java‎

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import graphql.schema.GraphQLType;
3434
import graphql.schema.GraphQLUnionType;
3535
import graphql.schema.GraphQLUnmodifiedType;
36+
import graphql.schema.InputValueWithState;
3637

3738
import java.util.ArrayList;
3839
import java.util.List;
@@ -48,6 +49,7 @@ public class TraversalContext implements DocumentVisitor {
4849
private final List<GraphQLOutputType> outputTypeStack = new ArrayList<>();
4950
private final List<GraphQLCompositeType> parentTypeStack = new ArrayList<>();
5051
private final List<GraphQLInputType> inputTypeStack = new ArrayList<>();
52+
private final List<InputValueWithState> defaultValueStack = new ArrayList<>();
5153
private final List<GraphQLFieldDefinition> fieldDefStack = new ArrayList<>();
5254
private final List<String> nameStack = new ArrayList<>();
5355
private GraphQLDirective directive;
@@ -156,6 +158,7 @@ private void enterImpl(Argument argument) {
156158
}
157159

158160
addInputType(argumentType != null ? argumentType.getType() : null);
161+
addDefaultValue(argumentType != null ? argumentType.getArgumentDefaultValue() : null);
159162
this.argument = argumentType;
160163
}
161164

@@ -166,23 +169,30 @@ private void enterImpl(ArrayValue arrayValue) {
166169
inputType = (GraphQLInputType) unwrapOne(nullableType);
167170
}
168171
addInputType(inputType);
172+
// List positions never have a default value. See graphql-js impl for inspiration
173+
addDefaultValue(null);
169174
}
170175

171176
private void enterImpl(ObjectField objectField) {
172177
GraphQLUnmodifiedType objectType = unwrapAll(getInputType());
173178
GraphQLInputType inputType = null;
179+
GraphQLInputObjectField inputField = null;
174180
if (objectType instanceof GraphQLInputObjectType) {
175181
GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) objectType;
176-
GraphQLInputObjectField inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName());
177-
if (inputField != null)
182+
inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName());
183+
if (inputField != null) {
178184
inputType = inputField.getType();
185+
}
179186
}
180187
addInputType(inputType);
188+
addDefaultValue(inputField != null ? inputField.getInputFieldDefaultValue() : null);
181189
}
182190

183191
private GraphQLArgument find(List<GraphQLArgument> arguments, String name) {
184192
for (GraphQLArgument argument : arguments) {
185-
if (argument.getName().equals(name)) return argument;
193+
if (argument.getName().equals(name)) {
194+
return argument;
195+
}
186196
}
187197
return null;
188198
}
@@ -191,29 +201,32 @@ private GraphQLArgument find(List<GraphQLArgument> arguments, String name) {
191201
@Override
192202
public void leave(Node node, List<Node> ancestors) {
193203
if (node instanceof OperationDefinition) {
194-
outputTypeStack.remove(outputTypeStack.size() - 1);
204+
pop(outputTypeStack);
195205
} else if (node instanceof SelectionSet) {
196-
parentTypeStack.remove(parentTypeStack.size() - 1);
206+
pop(parentTypeStack);
197207
} else if (node instanceof Field) {
198208
leaveName(((Field) node).getName());
199-
fieldDefStack.remove(fieldDefStack.size() - 1);
200-
outputTypeStack.remove(outputTypeStack.size() - 1);
209+
pop(fieldDefStack);
210+
pop(outputTypeStack);
201211
} else if (node instanceof Directive) {
202212
directive = null;
203213
} else if (node instanceof InlineFragment) {
204-
outputTypeStack.remove(outputTypeStack.size() - 1);
214+
pop(outputTypeStack);
205215
} else if (node instanceof FragmentDefinition) {
206216
leaveName(((FragmentDefinition) node).getName());
207-
outputTypeStack.remove(outputTypeStack.size() - 1);
217+
pop(outputTypeStack);
208218
} else if (node instanceof VariableDefinition) {
209219
inputTypeStack.remove(inputTypeStack.size() - 1);
210220
} else if (node instanceof Argument) {
211221
argument = null;
212-
inputTypeStack.remove(inputTypeStack.size() - 1);
222+
pop(inputTypeStack);
223+
pop(defaultValueStack);
213224
} else if (node instanceof ArrayValue) {
214-
inputTypeStack.remove(inputTypeStack.size() - 1);
225+
pop(inputTypeStack);
226+
pop(defaultValueStack);
215227
} else if (node instanceof ObjectField) {
216-
inputTypeStack.remove(inputTypeStack.size() - 1);
228+
pop(inputTypeStack);
229+
pop(defaultValueStack);
217230
}
218231
}
219232

@@ -250,10 +263,16 @@ private void addOutputType(GraphQLOutputType type) {
250263
}
251264

252265
private <T> T lastElement(List<T> list) {
253-
if (list.size() == 0) return null;
266+
if (list.isEmpty()) {
267+
return null;
268+
}
254269
return list.get(list.size() - 1);
255270
}
256271

272+
private <T> T pop(List<T> list) {
273+
return list.remove(list.size() - 1);
274+
}
275+
257276
/**
258277
* @return can be null if the parent is not a CompositeType
259278
*/
@@ -268,11 +287,18 @@ private void addParentType(GraphQLCompositeType compositeType) {
268287
public GraphQLInputType getInputType() {
269288
return lastElement(inputTypeStack);
270289
}
290+
public InputValueWithState getDefaultValue() {
291+
return lastElement(defaultValueStack);
292+
}
271293

272294
private void addInputType(GraphQLInputType graphQLInputType) {
273295
inputTypeStack.add(graphQLInputType);
274296
}
275297

298+
private void addDefaultValue(InputValueWithState defaultValue) {
299+
defaultValueStack.add(defaultValue);
300+
}
301+
276302
public GraphQLFieldDefinition getFieldDef() {
277303
return lastElement(fieldDefStack);
278304
}

‎src/main/java/graphql/validation/ValidationContext.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import graphql.schema.GraphQLInputType;
1414
import graphql.schema.GraphQLOutputType;
1515
import graphql.schema.GraphQLSchema;
16+
import graphql.schema.InputValueWithState;
1617

1718
import java.util.LinkedHashMap;
1819
import java.util.List;
@@ -37,8 +38,10 @@ public ValidationContext(GraphQLSchema schema, Document document, I18n i18n) {
3738
}
3839

3940
private void buildFragmentMap() {
40-
for (Definition definition : document.getDefinitions()) {
41-
if (!(definition instanceof FragmentDefinition)) continue;
41+
for (Definition<?> definition : document.getDefinitions()) {
42+
if (!(definition instanceof FragmentDefinition)) {
43+
continue;
44+
}
4245
FragmentDefinition fragmentDefinition = (FragmentDefinition) definition;
4346
fragmentDefinitionMap.put(fragmentDefinition.getName(), fragmentDefinition);
4447
}
@@ -68,6 +71,10 @@ public GraphQLInputType getInputType() {
6871
return traversalContext.getInputType();
6972
}
7073

74+
public InputValueWithState getDefaultValue() {
75+
return traversalContext.getDefaultValue();
76+
}
77+
7178
public GraphQLFieldDefinition getFieldDef() {
7279
return traversalContext.getFieldDef();
7380
}

‎src/main/java/graphql/validation/rules/VariableTypesMatch.java‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -59,24 +59,25 @@ public void checkVariable(VariableReference variableReference) {
5959
if (variableType == null) {
6060
return;
6161
}
62-
GraphQLInputType expectedType = getValidationContext().getInputType();
63-
Optional<InputValueWithState> schemaDefault = Optional.ofNullable(getValidationContext().getArgument()).map(v -> v.getArgumentDefaultValue());
64-
Value schemaDefaultValue = null;
65-
if (schemaDefault.isPresent() && schemaDefault.get().isLiteral()) {
66-
schemaDefaultValue = (Value) schemaDefault.get().getValue();
67-
} else if (schemaDefault.isPresent() && schemaDefault.get().isSet()) {
68-
schemaDefaultValue = ValuesResolver.valueToLiteral(schemaDefault.get(), expectedType);
69-
}
70-
if (expectedType == null) {
71-
// we must have a unknown variable say to not have a known type
62+
GraphQLInputType locationType = getValidationContext().getInputType();
63+
Optional<InputValueWithState> locationDefault = Optional.ofNullable(getValidationContext().getDefaultValue());
64+
if (locationType == null) {
65+
// we must have an unknown variable say to not have a known type
7266
return;
7367
}
74-
if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType) &&
75-
!variablesTypesMatcher.doesVariableTypesMatch(variableType, schemaDefaultValue, expectedType)) {
68+
Value<?> locationDefaultValue = null;
69+
if (locationDefault.isPresent() && locationDefault.get().isLiteral()) {
70+
locationDefaultValue = (Value<?>) locationDefault.get().getValue();
71+
} else if (locationDefault.isPresent() && locationDefault.get().isSet()) {
72+
locationDefaultValue = ValuesResolver.valueToLiteral(locationDefault.get(), locationType);
73+
}
74+
boolean variableDefMatches = variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), locationType, locationDefaultValue);
75+
if (!variableDefMatches) {
7676
GraphQLType effectiveType = variablesTypesMatcher.effectiveType(variableType, variableDefinition.getDefaultValue());
7777
String message = i18n(VariableTypeMismatch, "VariableTypesMatchRule.unexpectedType",
78+
variableDefinition.getName(),
7879
GraphQLTypeUtil.simplePrint(effectiveType),
79-
GraphQLTypeUtil.simplePrint(expectedType));
80+
GraphQLTypeUtil.simplePrint(locationType));
8081
addError(VariableTypeMismatch, variableReference.getSourceLocation(), message);
8182
}
8283
}

‎src/main/java/graphql/validation/rules/VariablesTypesMatcher.java‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,38 @@
99
import static graphql.schema.GraphQLNonNull.nonNull;
1010
import static graphql.schema.GraphQLTypeUtil.isList;
1111
import static graphql.schema.GraphQLTypeUtil.isNonNull;
12+
import static graphql.schema.GraphQLTypeUtil.unwrapNonNull;
1213
import static graphql.schema.GraphQLTypeUtil.unwrapOne;
1314

1415
@Internal
1516
public class VariablesTypesMatcher {
1617

17-
public boolean doesVariableTypesMatch(GraphQLType variableType, Value variableDefaultValue, GraphQLType expectedType) {
18-
return checkType(effectiveType(variableType, variableDefaultValue), expectedType);
18+
/**
19+
* This method and variable naming was inspired from the reference graphql-js implementation
20+
*
21+
* @param varType the variable type
22+
* @param varDefaultValue the default value for the variable
23+
* @param locationType the location type where the variable was encountered
24+
* @param locationDefaultValue the default value for that location
25+
*
26+
* @return true if the variable matches ok
27+
*/
28+
public boolean doesVariableTypesMatch(GraphQLType varType, Value<?> varDefaultValue, GraphQLType locationType, Value<?> locationDefaultValue) {
29+
if (isNonNull(locationType) && !isNonNull(varType)) {
30+
boolean hasNonNullVariableDefaultValue =
31+
varDefaultValue != null && !(varDefaultValue instanceof NullValue);
32+
boolean hasLocationDefaultValue = locationDefaultValue != null;
33+
if (!hasNonNullVariableDefaultValue && !hasLocationDefaultValue) {
34+
return false;
35+
}
36+
GraphQLType nullableLocationType = unwrapNonNull(locationType);
37+
return checkType(varType, nullableLocationType);
38+
}
39+
return checkType(varType, locationType);
1940
}
2041

21-
public GraphQLType effectiveType(GraphQLType variableType, Value defaultValue) {
42+
43+
public GraphQLType effectiveType(GraphQLType variableType, Value<?> defaultValue) {
2244
if (defaultValue == null || defaultValue instanceof NullValue) {
2345
return variableType;
2446
}

‎src/main/resources/i18n/Validation.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ VariableDefaultValuesOfCorrectType.badDefault=Validation error ({0}) : Bad defau
7676
#
7777
VariablesAreInputTypes.wrongType=Validation error ({0}) : Input variable ''{1}'' type ''{2}'' is not an input type
7878
#
79-
VariableTypesMatchRule.unexpectedType=Validation error ({0}) : Variable type ''{1}'' does not match expected type ''{2}''
79+
VariableTypesMatchRule.unexpectedType=Validation error ({0}) : Variable ''{1}'' of type ''{2}'' used in position expecting type ''{3}''
8080
#
8181
# These are used but IDEA cant find them easily as being called
8282
#

‎src/test/groovy/graphql/execution/ValuesResolverE2ETest.groovy‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
package graphql.execution
2+
3+
import graphql.ExecutionInput
4+
import graphql.ExecutionResult
5+
import graphql.GraphQL
6+
import graphql.Scalars
7+
import graphql.TestUtil
8+
import graphql.schema.DataFetcher
9+
import graphql.schema.DataFetchingEnvironment
10+
import graphql.schema.FieldCoordinates
11+
import graphql.schema.GraphQLCodeRegistry
12+
import graphql.schema.GraphQLInputObjectType
13+
import graphql.schema.GraphQLList
14+
import graphql.schema.GraphQLObjectType
15+
import graphql.schema.GraphQLSchema
16+
import spock.lang.Specification
17+
18+
class ValuesResolverE2ETest extends Specification {
19+
20+
def "issue 3276 - reported bug on validation problems as SDL"() {
21+
def sdl = '''
22+
type Query {
23+
items(pagination: Pagination = {limit: 10, offset: 0}): [String]
24+
}
25+
26+
input Pagination {
27+
limit: Int
28+
offset: Int
29+
}
30+
'''
31+
DataFetcher df = { DataFetchingEnvironment env ->
32+
def pagination = env.getArgument("pagination") as Map<String, Integer>
33+
def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value }
34+
return strings
35+
}
36+
def schema = TestUtil.schema(sdl, [Query: [items: df]])
37+
def graphQL = GraphQL.newGraphQL(schema).build()
38+
39+
when:
40+
def ei = ExecutionInput.newExecutionInput('''
41+
query Items($limit: Int, $offset: Int) {
42+
items(pagination: {limit: $limit, offset: $offset})
43+
}
44+
''').variables([limit: 5, offset: 0]).build()
45+
def er = graphQL.execute(ei)
46+
then:
47+
er.errors.isEmpty()
48+
er.data == [items : ["limit=5", "offset=0"]]
49+
}
50+
51+
def "issue 3276 - reported bug on validation problems as reported code"() {
52+
DataFetcher<?> dataFetcher = { env ->
53+
def pagination = env.getArgument("pagination") as Map<String, Integer>
54+
def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value }
55+
return strings
56+
}
57+
GraphQLSchema schema = GraphQLSchema.newSchema()
58+
.query(GraphQLObjectType.newObject()
59+
.name("Query")
60+
.field(items -> items
61+
.name("items")
62+
.type(GraphQLList.list(Scalars.GraphQLString))
63+
.argument(pagination -> pagination
64+
.name("pagination")
65+
//skipped adding the default limit/offset values as it doesn't change anything
66+
.defaultValueProgrammatic(new HashMap<>())
67+
.type(GraphQLInputObjectType.newInputObject()
68+
.name("Pagination")
69+
.field(limit -> limit
70+
.name("limit")
71+
.type(Scalars.GraphQLInt))
72+
.field(offset -> offset
73+
.name("offset")
74+
.type(Scalars.GraphQLInt))
75+
.build())))
76+
.build())
77+
.codeRegistry(GraphQLCodeRegistry.newCodeRegistry()
78+
.dataFetcher(FieldCoordinates.coordinates("Query", "items"), dataFetcher)
79+
.build())
80+
.build()
81+
82+
GraphQL gql = GraphQL.newGraphQL(schema).build()
83+
84+
Map<String, Object> vars = new HashMap<>()
85+
vars.put("limit", 5)
86+
vars.put("offset", 0)
87+
88+
ExecutionInput ei = ExecutionInput.newExecutionInput()
89+
.query("query Items( \$limit: Int, \$offset: Int) {\n" +
90+
" items(\n" +
91+
" pagination: {limit: \$limit, offset: \$offset} \n" +
92+
" )\n" +
93+
"}")
94+
.variables(vars)
95+
.build()
96+
97+
when:
98+
ExecutionResult result = gql.execute( ei)
99+
then:
100+
result.errors.isEmpty()
101+
result.data == [items : ["limit=5", "offset=0"]]
102+
}
103+
104+
def "issue 3276 - should end up in validation errors because location defaults are not present"() {
105+
def sdl = '''
106+
type Query {
107+
items(pagination: Pagination = {limit: 1, offset: 1}): [String]
108+
}
109+
input Pagination {
110+
limit: Int! #non-null this time, no default value
111+
offset: Int! #non-null this time, no default value
112+
}
113+
'''
114+
DataFetcher df = { DataFetchingEnvironment env ->
115+
def pagination = env.getArgument("pagination") as Map<String, Integer>
116+
def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value }
117+
return strings
118+
}
119+
def schema = TestUtil.schema(sdl, [Query: [items: df]])
120+
def graphQL = GraphQL.newGraphQL(schema).build()
121+
122+
when:
123+
def ei = ExecutionInput.newExecutionInput('''
124+
query Items( $limit: Int, $offset: Int) {
125+
items(
126+
pagination: {limit: $limit, offset: $offset}
127+
)
128+
}
129+
''').variables([limit: 5, offset: null]).build()
130+
def er = graphQL.execute(ei)
131+
then:
132+
er.errors.size() == 2
133+
er.errors[0].message == "Validation error (VariableTypeMismatch@[items]) : Variable 'limit' of type 'Int' used in position expecting type 'Int!'"
134+
er.errors[1].message == "Validation error (VariableTypeMismatch@[items]) : Variable 'offset' of type 'Int' used in position expecting type 'Int!'"
135+
}
136+
}

0 commit comments

Comments
 (0)