Skip to content

Commit eb3afc2

Browse files
andimarekandimarek
authored
Merge pull request #2308 from graphql-java/values-resolver-refactoring
refactor code to align more clearly with spec
2 parents d783ec7 + 9482ad2 commit eb3afc2

3 files changed

Lines changed: 105 additions & 75 deletions

File tree

‎src/main/java/graphql/execution/ValuesResolver.java‎

Lines changed: 94 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package graphql.execution;
22

33

4+
import graphql.Assert;
45
import graphql.Internal;
56
import graphql.language.Argument;
67
import graphql.language.ArrayValue;
@@ -17,6 +18,7 @@
1718
import graphql.schema.GraphQLEnumType;
1819
import graphql.schema.GraphQLInputObjectField;
1920
import graphql.schema.GraphQLInputObjectType;
21+
import graphql.schema.GraphQLInputType;
2022
import graphql.schema.GraphQLList;
2123
import graphql.schema.GraphQLScalarType;
2224
import graphql.schema.GraphQLSchema;
@@ -48,6 +50,7 @@ public class ValuesResolver {
4850
* @param schema the schema
4951
* @param variableDefinitions the variable definitions
5052
* @param variableValues the supplied variables
53+
*
5154
* @return coerced variable values as a map
5255
*/
5356
public Map<String, Object> coerceVariableValues(GraphQLSchema schema, List<VariableDefinition> variableDefinitions, Map<String, Object> variableValues) {
@@ -57,19 +60,26 @@ public Map<String, Object> coerceVariableValues(GraphQLSchema schema, List<Varia
5760
String variableName = variableDefinition.getName();
5861
List<Object> nameStack = new ArrayList<>();
5962
GraphQLType variableType = TypeFromAST.getTypeFromAST(schema, variableDefinition.getType());
60-
61-
if (!variableValues.containsKey(variableName)) {
62-
Value defaultValue = variableDefinition.getDefaultValue();
63-
if (defaultValue != null) {
64-
Object coercedValue = coerceValueAst(fieldVisibility, variableType, defaultValue, null);
63+
Assert.assertTrue(variableType instanceof GraphQLInputType);
64+
// can be NullValue
65+
Value defaultValue = variableDefinition.getDefaultValue();
66+
boolean hasValue = variableValues.containsKey(variableName);
67+
Object value = variableValues.get(variableName);
68+
if (!hasValue && defaultValue != null) {
69+
Object coercedDefaultValue = coerceValueAst(fieldVisibility, variableType, defaultValue, null);
70+
coercedValues.put(variableName, coercedDefaultValue);
71+
} else if (isNonNull(variableType) && (!hasValue || value == null)) {
72+
throw new NonNullableValueCoercedAsNullException(variableDefinition, variableType);
73+
} else if (hasValue) {
74+
if (value == null) {
75+
coercedValues.put(variableName, null);
76+
} else {
77+
Object coercedValue = coerceValue(fieldVisibility, variableDefinition, variableDefinition.getName(), variableType, value, nameStack);
6578
coercedValues.put(variableName, coercedValue);
66-
} else if (isNonNull(variableType)) {
67-
throw new NonNullableValueCoercedAsNullException(variableDefinition, variableType);
6879
}
6980
} else {
70-
Object value = variableValues.get(variableName);
71-
Object coercedValue = coerceValue(fieldVisibility, variableDefinition, variableDefinition.getName(), variableType, value, nameStack);
72-
coercedValues.put(variableName, coercedValue);
81+
// hasValue = false && no defaultValue for a nullable type
82+
// meaning no value was provided for variableName
7383
}
7484
}
7585

@@ -85,42 +95,51 @@ public Map<String, Object> getArgumentValues(GraphQLCodeRegistry codeRegistry, L
8595
return getArgumentValuesImpl(codeRegistry, argumentTypes, arguments, variables);
8696
}
8797

88-
private Map<String, Object> getArgumentValuesImpl(GraphQLCodeRegistry codeRegistry, List<GraphQLArgument> argumentTypes, List<Argument> arguments, Map<String, Object> variables) {
98+
private Map<String, Object> getArgumentValuesImpl(GraphQLCodeRegistry codeRegistry, List<GraphQLArgument> argumentTypes, List<Argument> arguments,
99+
Map<String, Object> coercedVariableValues) {
89100
if (argumentTypes.isEmpty()) {
90101
return Collections.emptyMap();
91102
}
92103

93-
Map<String, Object> result = new LinkedHashMap<>();
104+
Map<String, Object> coercedValues = new LinkedHashMap<>();
94105
Map<String, Argument> argumentMap = argumentMap(arguments);
95-
for (GraphQLArgument fieldArgument : argumentTypes) {
96-
String argName = fieldArgument.getName();
97-
Argument argument = argumentMap.get(argName);
98-
Object value = null;
99-
if (argument != null) {
100-
value = coerceValueAst(codeRegistry.getFieldVisibility(), fieldArgument.getType(), argument.getValue(), variables);
101-
}
102-
if (value == null
103-
&& !(argument != null && argument.getValue() instanceof NullValue)
104-
&& !(argument != null && argument.getValue() instanceof VariableReference && variables.containsKey(((VariableReference) argument.getValue()).getName()))
105-
) {
106-
value = fieldArgument.getDefaultValue();
106+
for (GraphQLArgument argumentDefinition : argumentTypes) {
107+
GraphQLInputType argumentType = argumentDefinition.getType();
108+
String argumentName = argumentDefinition.getName();
109+
Argument argument = argumentMap.get(argumentName);
110+
Object defaultValue = argumentDefinition.getDefaultValue();
111+
boolean hasValue = argument != null;
112+
Object value;
113+
Value argumentValue = argument != null ? argument.getValue() : null;
114+
if (argumentValue instanceof VariableReference) {
115+
String variableName = ((VariableReference) argumentValue).getName();
116+
hasValue = coercedVariableValues.containsKey(variableName);
117+
value = coercedVariableValues.get(variableName);
118+
} else {
119+
value = argumentValue;
107120
}
108-
boolean wasValueProvided = false;
109-
if (argumentMap.containsKey(argName)) {
110-
if (argument.getValue() instanceof VariableReference) {
111-
wasValueProvided = variables.containsKey(((VariableReference) argument.getValue()).getName());
121+
if (!hasValue && argumentDefinition.hasSetDefaultValue()) {
122+
//TODO: default value needs to be coerced
123+
coercedValues.put(argumentName, defaultValue);
124+
} else if (isNonNull(argumentType) && (!hasValue || value == null)) {
125+
throw new RuntimeException();
126+
} else if (hasValue) {
127+
if (value == null) {
128+
coercedValues.put(argumentName, null);
129+
} else if (argumentValue instanceof VariableReference) {
130+
coercedValues.put(argumentName, value);
112131
} else {
113-
wasValueProvided = true;
132+
value = coerceValueAst(codeRegistry.getFieldVisibility(), argumentType, argument.getValue(), coercedVariableValues);
133+
coercedValues.put(argumentName, value);
114134
}
135+
} else {
136+
// nullable type && hasValue == false && hasDefaultValue == false
137+
// meaning no value was provided for argumentName
115138
}
116-
if (fieldArgument.hasSetDefaultValue()) {
117-
wasValueProvided = true;
118-
}
119-
if (wasValueProvided) {
120-
result.put(argName, value);
121-
}
139+
122140
}
123-
return result;
141+
return coercedValues;
142+
124143
}
125144

126145

@@ -279,48 +298,51 @@ private Object coerceValueAstForList(GraphqlFieldVisibility fieldVisibility, Gra
279298
}
280299
}
281300

282-
private Object coerceValueAstForInputObject(GraphqlFieldVisibility fieldVisibility, GraphQLInputObjectType type, ObjectValue inputValue, Map<String, Object> variables) {
283-
Map<String, Object> result = new LinkedHashMap<>();
284-
285-
Map<String, ObjectField> inputValueFieldsByName = mapObjectValueFieldsByName(inputValue);
286-
287-
List<GraphQLInputObjectField> inputFields = fieldVisibility.getFieldDefinitions(type);
288-
for (GraphQLInputObjectField inputTypeField : inputFields) {
289-
if (inputValueFieldsByName.containsKey(inputTypeField.getName())) {
290-
boolean putObjectInMap = true;
291-
292-
ObjectField field = inputValueFieldsByName.get(inputTypeField.getName());
293-
Value fieldInputValue = field.getValue();
294-
295-
Object fieldObject = null;
296-
if (fieldInputValue instanceof VariableReference) {
297-
String varName = ((VariableReference) fieldInputValue).getName();
298-
if (!variables.containsKey(varName)) {
299-
putObjectInMap = false;
300-
} else {
301-
fieldObject = variables.get(varName);
302-
}
303-
} else {
304-
fieldObject = coerceValueAst(fieldVisibility, inputTypeField.getType(), fieldInputValue, variables);
305-
}
301+
private Object coerceValueAstForInputObject(GraphqlFieldVisibility fieldVisibility,
302+
GraphQLInputObjectType type,
303+
ObjectValue inputValue,
304+
Map<String, Object> coercedVariableValues) {
305+
Map<String, Object> coercedValues = new LinkedHashMap<>();
306306

307-
if (fieldObject == null) {
308-
if (!(field.getValue() instanceof NullValue)) {
309-
fieldObject = inputTypeField.getDefaultValue();
310-
}
311-
}
312-
if (putObjectInMap) {
313-
result.put(field.getName(), fieldObject);
307+
Map<String, ObjectField> inputFieldsByName = mapObjectValueFieldsByName(inputValue);
308+
309+
List<GraphQLInputObjectField> inputFieldTypes = fieldVisibility.getFieldDefinitions(type);
310+
for (GraphQLInputObjectField inputFieldType : inputFieldTypes) {
311+
312+
GraphQLInputType fieldType = inputFieldType.getType();
313+
String fieldName = inputFieldType.getName();
314+
ObjectField field = inputFieldsByName.get(fieldName);
315+
Object defaultValue = inputFieldType.getDefaultValue();
316+
boolean hasValue = field != null;
317+
Object value;
318+
Value fieldValue = field != null ? field.getValue() : null;
319+
if (fieldValue instanceof VariableReference) {
320+
String variableName = ((VariableReference) fieldValue).getName();
321+
hasValue = coercedVariableValues.containsKey(variableName);
322+
value = coercedVariableValues.get(variableName);
323+
} else {
324+
value = fieldValue;
325+
}
326+
if (!hasValue && inputFieldType.hasSetDefaultValue()) {
327+
//TODO: default value should be coerced
328+
coercedValues.put(fieldName, defaultValue);
329+
} else if (isNonNull(fieldType) && (!hasValue || value == null)) {
330+
throw new NonNullableValueCoercedAsNullException(inputFieldType);
331+
} else if (hasValue) {
332+
if (value == null) {
333+
coercedValues.put(fieldName, null);
334+
} else if (fieldValue instanceof VariableReference) {
335+
coercedValues.put(fieldName, value);
314336
} else {
315-
assertNonNullInputField(inputTypeField);
337+
value = coerceValueAst(fieldVisibility, fieldType, fieldValue, coercedVariableValues);
338+
coercedValues.put(fieldName, value);
316339
}
317-
} else if (inputTypeField.getDefaultValue() != null) {
318-
result.put(inputTypeField.getName(), inputTypeField.getDefaultValue());
319340
} else {
320-
assertNonNullInputField(inputTypeField);
341+
// nullable type && hasValue == false && hasDefaultValue == false
342+
// meaning no value was provided for this field
321343
}
322344
}
323-
return result;
345+
return coercedValues;
324346
}
325347

326348
private void assertNonNullInputField(GraphQLInputObjectField inputTypeField) {

‎src/main/java/graphql/schema/GraphQLInputObjectField.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ public class GraphQLInputObjectField implements GraphQLNamedSchemaElement, Graph
4141
public static final String CHILD_TYPE = "type";
4242
public static final String CHILD_DIRECTIVES = "directives";
4343

44+
private static final Object DEFAULT_VALUE_SENTINEL = new Object() {
45+
};
46+
47+
4448
/**
4549
* @param name the name
4650
* @param type the field type
@@ -111,7 +115,11 @@ public GraphQLInputType getType() {
111115
}
112116

113117
public Object getDefaultValue() {
114-
return defaultValue;
118+
return defaultValue == DEFAULT_VALUE_SENTINEL ? null : defaultValue;
119+
}
120+
121+
public boolean hasSetDefaultValue() {
122+
return defaultValue != DEFAULT_VALUE_SENTINEL;
115123
}
116124

117125
public String getDescription() {
@@ -246,7 +254,7 @@ public static Builder newInputObjectField() {
246254

247255
@PublicApi
248256
public static class Builder extends GraphqlTypeBuilder {
249-
private Object defaultValue;
257+
private Object defaultValue = DEFAULT_VALUE_SENTINEL;
250258
private GraphQLInputType type;
251259
private InputValueDefinition definition;
252260
private final List<GraphQLDirective> directives = new ArrayList<>();

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,7 @@ class ValuesResolverTest extends Specification {
488488

489489
then:
490490
def error = thrown(NonNullableValueCoercedAsNullException)
491-
error.message == "Field 'foo' of variable 'foo' has coerced Null value for NonNull type 'String!'"
491+
error.message == "Variable 'foo' has coerced Null value for NonNull type 'String!'"
492492
}
493493

494494
// Note: use NullValue defined in Field when it exists,

0 commit comments

Comments
 (0)