Skip to content

Commit 9c99711

Browse files
bbakermanbbakerman
committed
issue 3276 - more work on fixing this bug
1 parent 5e4e682 commit 9c99711

7 files changed

Lines changed: 196 additions & 39 deletions

File tree

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

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import graphql.schema.GraphQLType;
3333
import graphql.schema.GraphQLUnionType;
3434
import graphql.schema.GraphQLUnmodifiedType;
35+
import graphql.schema.InputValueWithState;
3536

3637
import java.util.ArrayList;
3738
import java.util.List;
@@ -47,6 +48,7 @@ public class TraversalContext implements DocumentVisitor {
4748
private final List<GraphQLOutputType> outputTypeStack = new ArrayList<>();
4849
private final List<GraphQLCompositeType> parentTypeStack = new ArrayList<>();
4950
private final List<GraphQLInputType> inputTypeStack = new ArrayList<>();
51+
private final List<InputValueWithState> defaultValueStack = new ArrayList<>();
5052
private final List<GraphQLFieldDefinition> fieldDefStack = new ArrayList<>();
5153
private final List<String> nameStack = new ArrayList<>();
5254
private GraphQLDirective directive;
@@ -155,6 +157,7 @@ private void enterImpl(Argument argument) {
155157
}
156158

157159
addInputType(argumentType != null ? argumentType.getType() : null);
160+
addDefaultValue(argumentType != null ? argumentType.getArgumentDefaultValue() : null);
158161
this.argument = argumentType;
159162
}
160163

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

170175
private void enterImpl(ObjectField objectField) {
171176
GraphQLUnmodifiedType objectType = unwrapAll(getInputType());
172177
GraphQLInputType inputType = null;
178+
GraphQLInputObjectField inputField = null;
173179
if (objectType instanceof GraphQLInputObjectType) {
174180
GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) objectType;
175-
GraphQLInputObjectField inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName());
176-
if (inputField != null)
181+
inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName());
182+
if (inputField != null) {
177183
inputType = inputField.getType();
184+
}
178185
}
179186
addInputType(inputType);
187+
addDefaultValue(inputField != null ? inputField.getInputFieldDefaultValue() : null);
180188
}
181189

182190
private GraphQLArgument find(List<GraphQLArgument> arguments, String name) {
183191
for (GraphQLArgument argument : arguments) {
184-
if (argument.getName().equals(name)) return argument;
192+
if (argument.getName().equals(name)) {
193+
return argument;
194+
}
185195
}
186196
return null;
187197
}
@@ -190,29 +200,32 @@ private GraphQLArgument find(List<GraphQLArgument> arguments, String name) {
190200
@Override
191201
public void leave(Node node, List<Node> ancestors) {
192202
if (node instanceof OperationDefinition) {
193-
outputTypeStack.remove(outputTypeStack.size() - 1);
203+
pop(outputTypeStack);
194204
} else if (node instanceof SelectionSet) {
195-
parentTypeStack.remove(parentTypeStack.size() - 1);
205+
pop(parentTypeStack);
196206
} else if (node instanceof Field) {
197207
leaveName(((Field) node).getName());
198-
fieldDefStack.remove(fieldDefStack.size() - 1);
199-
outputTypeStack.remove(outputTypeStack.size() - 1);
208+
pop(fieldDefStack);
209+
pop(outputTypeStack);
200210
} else if (node instanceof Directive) {
201211
directive = null;
202212
} else if (node instanceof InlineFragment) {
203-
outputTypeStack.remove(outputTypeStack.size() - 1);
213+
pop(outputTypeStack);
204214
} else if (node instanceof FragmentDefinition) {
205215
leaveName(((FragmentDefinition) node).getName());
206-
outputTypeStack.remove(outputTypeStack.size() - 1);
216+
pop(outputTypeStack);
207217
} else if (node instanceof VariableDefinition) {
208218
inputTypeStack.remove(inputTypeStack.size() - 1);
209219
} else if (node instanceof Argument) {
210220
argument = null;
211-
inputTypeStack.remove(inputTypeStack.size() - 1);
221+
pop(inputTypeStack);
222+
pop(defaultValueStack);
212223
} else if (node instanceof ArrayValue) {
213-
inputTypeStack.remove(inputTypeStack.size() - 1);
224+
pop(inputTypeStack);
225+
pop(defaultValueStack);
214226
} else if (node instanceof ObjectField) {
215-
inputTypeStack.remove(inputTypeStack.size() - 1);
227+
pop(inputTypeStack);
228+
pop(defaultValueStack);
216229
}
217230
}
218231

@@ -249,10 +262,16 @@ private void addOutputType(GraphQLOutputType type) {
249262
}
250263

251264
private <T> T lastElement(List<T> list) {
252-
if (list.size() == 0) return null;
265+
if (list.isEmpty()) {
266+
return null;
267+
}
253268
return list.get(list.size() - 1);
254269
}
255270

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

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

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

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import graphql.schema.GraphQLInputType;
1515
import graphql.schema.GraphQLOutputType;
1616
import graphql.schema.GraphQLSchema;
17+
import graphql.schema.InputValueWithState;
1718

1819
import java.util.LinkedHashMap;
1920
import java.util.List;
@@ -41,8 +42,10 @@ public ValidationContext(GraphQLSchema schema, Document document, I18n i18n) {
4142
}
4243

4344
private void buildFragmentMap() {
44-
for (Definition definition : document.getDefinitions()) {
45-
if (!(definition instanceof FragmentDefinition)) continue;
45+
for (Definition<?> definition : document.getDefinitions()) {
46+
if (!(definition instanceof FragmentDefinition)) {
47+
continue;
48+
}
4649
FragmentDefinition fragmentDefinition = (FragmentDefinition) definition;
4750
fragmentDefinitionMap.put(fragmentDefinition.getName(), fragmentDefinition);
4851
}
@@ -72,6 +75,10 @@ public GraphQLInputType getInputType() {
7275
return traversalContext.getInputType();
7376
}
7477

78+
public InputValueWithState getDefaultValue() {
79+
return traversalContext.getDefaultValue();
80+
}
81+
7582
public GraphQLFieldDefinition getFieldDef() {
7683
return traversalContext.getFieldDef();
7784
}

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

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import graphql.language.Value;
99
import graphql.language.VariableDefinition;
1010
import graphql.language.VariableReference;
11-
import graphql.schema.GraphQLArgument;
1211
import graphql.schema.GraphQLInputType;
1312
import graphql.schema.GraphQLType;
1413
import graphql.schema.GraphQLTypeUtil;
@@ -60,25 +59,24 @@ public void checkVariable(VariableReference variableReference) {
6059
if (variableType == null) {
6160
return;
6261
}
63-
GraphQLInputType expectedType = getValidationContext().getInputType();
64-
if (expectedType == null) {
62+
GraphQLInputType locationType = getValidationContext().getInputType();
63+
Optional<InputValueWithState> locationDefault = Optional.ofNullable(getValidationContext().getDefaultValue());
64+
if (locationType == null) {
6565
// we must have an unknown variable say to not have a known type
6666
return;
6767
}
68-
GraphQLArgument argument = getValidationContext().getArgument();
69-
Optional<InputValueWithState> schemaDefault = Optional.ofNullable(argument).map(v -> v.getArgumentDefaultValue());
70-
Value<?> schemaDefaultValue = null;
71-
if (schemaDefault.isPresent() && schemaDefault.get().isLiteral()) {
72-
schemaDefaultValue = (Value<?>) schemaDefault.get().getValue();
73-
} else if (schemaDefault.isPresent() && schemaDefault.get().isSet()) {
74-
schemaDefaultValue = ValuesResolver.valueToLiteral(schemaDefault.get(), argument.getType(), getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale());
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, getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale());
7573
}
76-
if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType) &&
77-
!variablesTypesMatcher.doesVariableTypesMatch(variableType, schemaDefaultValue, expectedType)) {
74+
boolean variableDefMatches = variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), locationType, locationDefaultValue);
75+
if (!variableDefMatches) {
7876
GraphQLType effectiveType = variablesTypesMatcher.effectiveType(variableType, variableDefinition.getDefaultValue());
7977
String message = i18n(VariableTypeMismatch, "VariableTypesMatchRule.unexpectedType",
8078
GraphQLTypeUtil.simplePrint(effectiveType),
81-
GraphQLTypeUtil.simplePrint(expectedType));
79+
GraphQLTypeUtil.simplePrint(locationType));
8280
addError(VariableTypeMismatch, variableReference.getSourceLocation(), message);
8381
}
8482
}

‎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/test/groovy/graphql/execution/ValuesResolverE2ETest.groovy‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import spock.lang.Specification
1717

1818
class ValuesResolverE2ETest extends Specification {
1919

20-
def "3276 - reported bug on validation problems as SDL"() {
20+
def "issue 3276 - reported bug on validation problems as SDL"() {
2121
def sdl = '''
2222
type Query {
2323
items(pagination: Pagination = {limit: 10, offset: 0}): [String]
@@ -48,7 +48,7 @@ class ValuesResolverE2ETest extends Specification {
4848
er.data == [items : ["limit=5", "offset=0"]]
4949
}
5050

51-
def "3276 - reported bug on validation problems as reported code"() {
51+
def "issue 3276 - reported bug on validation problems as reported code"() {
5252
DataFetcher<?> dataFetcher = { env ->
5353
def pagination = env.getArgument("pagination") as Map<String, Integer>
5454
def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value }
@@ -101,7 +101,7 @@ class ValuesResolverE2ETest extends Specification {
101101
result.data == [items : ["limit=5", "offset=0"]]
102102
}
103103

104-
def "3276 - reported bug on validation problems as non null inputs"() {
104+
def "issue 3276 - should end up in validation errors because location defaults are not present"() {
105105
def sdl = '''
106106
type Query {
107107
items(pagination: Pagination = {limit: 1, offset: 1}): [String]
@@ -129,7 +129,8 @@ class ValuesResolverE2ETest extends Specification {
129129
''').variables([limit: 5, offset: null]).build()
130130
def er = graphQL.execute(ei)
131131
then:
132-
er.errors.isEmpty()
133-
er.data == [items : ["limit=5", "offset=null"]]
132+
er.errors.size() == 2
133+
er.errors[0].message == "Validation error (VariableTypeMismatch@[items]) : Variable type 'Int' does not match expected type 'Int!'"
134+
er.errors[1].message == "Validation error (VariableTypeMismatch@[items]) : Variable type 'Int' does not match expected type 'Int!'"
134135
}
135136
}

0 commit comments

Comments
 (0)