Skip to content

Commit ace990f

Browse files
leebyronleebyron
committed
Changes based on feedback:
This preserves the behavior of explicit null values taking precedence over default values, greatly reducing the scope of breaking changes. This adds a change to argument coercion to enforce null checking on variable valuesand removes one validation rule: VariablesDefaultValueAllowed. This allows supplying default values to non-nullable variables, widening the gap between "required" and "nullable". It also changes the validation rules for arguments - allowing non-null arguments with default values to be omitted. This preserves all existing behavior (with the critical exception of no longer allowing `null` values into non-null arguments) while allowing queries which were previously considered invalid to be valid.
1 parent c1076c3 commit ace990f

16 files changed

+266
-331
lines changed

‎src/execution/__tests__/nonnull-test.js

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,6 @@ describe('Execute: handles non-nullable types', () => {
497497
args: {
498498
cannotBeNull: {
499499
type: GraphQLNonNull(GraphQLString),
500-
defaultValue: null,
501500
},
502501
},
503502
resolve: async (_, args) => {
@@ -567,26 +566,6 @@ describe('Execute: handles non-nullable types', () => {
567566
});
568567
});
569568

570-
it('succeeds when null variable has default value', async () => {
571-
const result = await execute({
572-
schema: schemaWithNonNullArg,
573-
document: parse(`
574-
query ($testVar: String = "default value") {
575-
withNonNullArg (cannotBeNull: $testVar)
576-
}
577-
`),
578-
variableValues: {
579-
testVar: null,
580-
},
581-
});
582-
583-
expect(result).to.deep.equal({
584-
data: {
585-
withNonNullArg: 'Passed: default value',
586-
},
587-
});
588-
});
589-
590569
it('field error when missing non-null arg', async () => {
591570
// Note: validation should identify this issue first (missing args rule)
592571
// however execution should still protect against this.
@@ -673,5 +652,33 @@ describe('Execute: handles non-nullable types', () => {
673652
],
674653
});
675654
});
655+
656+
it('field error when non-null arg provided variable with explicit null value', async () => {
657+
const result = await execute({
658+
schema: schemaWithNonNullArg,
659+
document: parse(`
660+
query ($testVar: String = "default value") {
661+
withNonNullArg (cannotBeNull: $testVar)
662+
}
663+
`),
664+
variableValues: {
665+
testVar: null,
666+
},
667+
});
668+
669+
expect(result).to.deep.equal({
670+
data: {
671+
withNonNullArg: null,
672+
},
673+
errors: [
674+
{
675+
message:
676+
'Argument "cannotBeNull" of non-null type "String!" must not be null.',
677+
locations: [{ line: 3, column: 43 }],
678+
path: ['withNonNullArg'],
679+
},
680+
],
681+
});
682+
});
676683
});
677684
});

‎src/execution/__tests__/variables-test.js

Lines changed: 46 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ const TestType = new GraphQLObjectType({
8686
}),
8787
fieldWithNonNullableStringInputAndDefaultArgumentValue: fieldWithInputArg({
8888
type: GraphQLNonNull(GraphQLString),
89-
defaultValue: 'Unreachable',
89+
defaultValue: 'Hello World',
9090
}),
9191
fieldWithNestedInputObject: fieldWithInputArg({
9292
type: TestNestedInputObject,
@@ -226,6 +226,40 @@ describe('Execute: Handles inputs', () => {
226226
});
227227
});
228228

229+
it('uses undefined when variable not provided', () => {
230+
const result = executeQuery(
231+
`
232+
query q($input: String) {
233+
fieldWithNullableStringInput(input: $input)
234+
}`,
235+
{
236+
// Intentionally missing variable values.
237+
},
238+
);
239+
240+
expect(result).to.deep.equal({
241+
data: {
242+
fieldWithNullableStringInput: null,
243+
},
244+
});
245+
});
246+
247+
it('uses null when variable provided explicit null value', () => {
248+
const result = executeQuery(
249+
`
250+
query q($input: String) {
251+
fieldWithNullableStringInput(input: $input)
252+
}`,
253+
{ input: null },
254+
);
255+
256+
expect(result).to.deep.equal({
257+
data: {
258+
fieldWithNullableStringInput: 'null',
259+
},
260+
});
261+
});
262+
229263
it('uses default value when not provided', () => {
230264
const result = executeQuery(`
231265
query ($input: TestInputObject = {a: "foo", b: ["bar"], c: "baz"}) {
@@ -255,7 +289,7 @@ describe('Execute: Handles inputs', () => {
255289
});
256290
});
257291

258-
it('uses default value when explicit null value provided', () => {
292+
it('uses explicit null value instead of default value', () => {
259293
const result = executeQuery(
260294
`
261295
query q($input: String = "Default value") {
@@ -266,7 +300,7 @@ describe('Execute: Handles inputs', () => {
266300

267301
expect(result).to.deep.equal({
268302
data: {
269-
fieldWithNullableStringInput: "'Default value'",
303+
fieldWithNullableStringInput: 'null',
270304
},
271305
});
272306
});
@@ -919,24 +953,18 @@ describe('Execute: Handles inputs', () => {
919953
});
920954
});
921955

922-
it('not when argument type is non-null', async () => {
923-
const ast = parse(`query optionalVariable($optional: String) {
924-
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional)
925-
}`);
956+
it('when no runtime value is provided to a non-null argument', () => {
957+
const result = executeQuery(`
958+
query optionalVariable($optional: String) {
959+
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional)
960+
}
961+
`);
926962

927-
expect(await execute(schema, ast)).to.deep.equal({
963+
expect(result).to.deep.equal({
928964
data: {
929-
fieldWithNonNullableStringInputAndDefaultArgumentValue: null,
965+
fieldWithNonNullableStringInputAndDefaultArgumentValue:
966+
"'Hello World'",
930967
},
931-
errors: [
932-
{
933-
message:
934-
'Argument "input" of required type "String!" was provided the ' +
935-
'variable "$optional" which was not provided a runtime value.',
936-
locations: [{ line: 2, column: 71 }],
937-
path: ['fieldWithNonNullableStringInputAndDefaultArgumentValue'],
938-
},
939-
],
940968
});
941969
});
942970
});

‎src/execution/values.js

Lines changed: 93 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
import { GraphQLError } from '../error';
1111
import find from '../jsutils/find';
12-
import isInvalid from '../jsutils/isInvalid';
12+
import invariant from '../jsutils/invariant';
1313
import keyMap from '../jsutils/keyMap';
1414
import { coerceValue } from '../utilities/coerceValue';
1515
import { typeFromAST } from '../utilities/typeFromAST';
@@ -66,48 +66,44 @@ export function getVariableValues(
6666
);
6767
} else {
6868
const hasValue = hasOwnProperty(inputs, varName);
69-
const value = inputs[varName];
70-
if (!hasValue || value == null) {
71-
if (isNonNullType(varType)) {
72-
// If no value or a nullish value was provided to a variable with a
73-
// non-null type (required), produce an error.
74-
errors.push(
75-
new GraphQLError(
76-
hasValue
77-
? `Variable "$${varName}" of non-null type ` +
78-
`"${String(varType)}" must not be null.`
79-
: `Variable "$${varName}" of required type ` +
80-
`"${String(varType)}" was not provided.`,
81-
[varDefNode],
82-
),
83-
);
84-
} else if (varDefNode.defaultValue) {
85-
// If no value or a nullish value was provided to a variable with a
86-
// default value, use the default value.
87-
coercedValues[varName] = valueFromAST(
88-
varDefNode.defaultValue,
89-
varType,
90-
);
91-
} else if (hasValue && value == null) {
92-
// If the explcit value `null` was provided, an entry in the coerced
69+
const value = hasValue ? inputs[varName] : undefined;
70+
if (!hasValue && varDefNode.defaultValue) {
71+
// If no value was provided to a variable with a default value,
72+
// use the default value.
73+
coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType);
74+
} else if ((!hasValue || value === null) && isNonNullType(varType)) {
75+
// If no value or a nullish value was provided to a variable with a
76+
// non-null type (required), produce an error.
77+
errors.push(
78+
new GraphQLError(
79+
hasValue
80+
? `Variable "$${varName}" of non-null type ` +
81+
`"${String(varType)}" must not be null.`
82+
: `Variable "$${varName}" of required type ` +
83+
`"${String(varType)}" was not provided.`,
84+
[varDefNode],
85+
),
86+
);
87+
} else if (hasValue) {
88+
if (value === null) {
89+
// If the explicit value `null` was provided, an entry in the coerced
9390
// values must exist as the value `null`.
9491
coercedValues[varName] = null;
95-
}
96-
} else {
97-
// Otherwise, a non-null value was provided, coerce it to the expected
98-
// type or report an error if coercion fails.
99-
const coerced = coerceValue(value, varType, varDefNode);
100-
const coercionErrors = coerced.errors;
101-
if (coercionErrors) {
102-
const messagePrelude = `Variable "$${varName}" got invalid value ${JSON.stringify(
103-
value,
104-
)}; `;
105-
coercionErrors.forEach(error => {
106-
error.message = messagePrelude + error.message;
107-
});
108-
errors.push(...coercionErrors);
10992
} else {
110-
coercedValues[varName] = coerced.value;
93+
// Otherwise, a non-null value was provided, coerce it to the expected
94+
// type or report an error if coercion fails.
95+
const coerced = coerceValue(value, varType, varDefNode);
96+
const coercionErrors = coerced.errors;
97+
if (coercionErrors) {
98+
coercionErrors.forEach(error => {
99+
error.message =
100+
`Variable "$${varName}" got invalid ` +
101+
`value ${JSON.stringify(value)}; ${error.message}`;
102+
});
103+
errors.push(...coercionErrors);
104+
} else {
105+
coercedValues[varName] = coerced.value;
106+
}
111107
}
112108
}
113109
}
@@ -142,57 +138,71 @@ export function getArgumentValues(
142138
const name = argDef.name;
143139
const argType = argDef.type;
144140
const argumentNode = argNodeMap[name];
145-
const defaultValue = argDef.defaultValue;
146-
if (!argumentNode || argumentNode.value.kind === Kind.NULL) {
147-
if (isNonNullType(argType)) {
148-
if (!argumentNode) {
149-
throw new GraphQLError(
150-
`Argument "${name}" of required type "${String(argType)}" ` +
151-
'was not provided.',
152-
[node],
153-
);
154-
} else {
155-
throw new GraphQLError(
156-
`Argument "${name}" of non-null type "${String(argType)}" ` +
157-
'must not be null.',
158-
[argumentNode.value],
159-
);
160-
}
161-
} else if (!isInvalid(defaultValue)) {
162-
coercedValues[name] = defaultValue;
163-
} else if (argumentNode && argumentNode.value.kind === Kind.NULL) {
164-
coercedValues[name] = null;
165-
}
166-
} else if (argumentNode.value.kind === Kind.VARIABLE) {
141+
let hasValue;
142+
let isNull;
143+
if (argumentNode && argumentNode.value.kind === Kind.VARIABLE) {
167144
const variableName = argumentNode.value.name.value;
168-
if (variableValues && hasOwnProperty(variableValues, variableName)) {
169-
// Note: this does not check that this variable value is correct.
170-
// This assumes that this query has been validated and the variable
171-
// usage here is of the correct type.
172-
coercedValues[name] = variableValues[variableName];
173-
} else if (isNonNullType(argType)) {
145+
hasValue = variableValues && hasOwnProperty(variableValues, variableName);
146+
isNull = variableValues && variableValues[variableName] === null;
147+
} else {
148+
hasValue = argumentNode != null;
149+
isNull = argumentNode && argumentNode.value.kind === Kind.NULL;
150+
}
151+
152+
if (!hasValue && argDef.defaultValue !== undefined) {
153+
// If no argument was provided where the definition has a default value,
154+
// use the default value.
155+
coercedValues[name] = argDef.defaultValue;
156+
} else if ((!hasValue || isNull) && isNonNullType(argType)) {
157+
// If no argument or a null value was provided to an argument with a
158+
// non-null type (required), produce a field error.
159+
if (isNull) {
174160
throw new GraphQLError(
175-
`Argument "${name}" of required type "${String(argType)}" was ` +
176-
`provided the variable "$${variableName}" which was not provided ` +
177-
'a runtime value.',
161+
`Argument "${name}" of non-null type "${String(argType)}" ` +
162+
'must not be null.',
178163
[argumentNode.value],
179164
);
180-
} else if (!isInvalid(defaultValue)) {
181-
coercedValues[name] = defaultValue;
182-
}
183-
} else {
184-
const valueNode = argumentNode.value;
185-
const coercedValue = valueFromAST(valueNode, argType, variableValues);
186-
if (isInvalid(coercedValue)) {
187-
// Note: ValuesOfCorrectType validation should catch this before
188-
// execution. This is a runtime check to ensure execution does not
189-
// continue with an invalid argument value.
165+
} else if (argumentNode && argumentNode.value.kind === Kind.VARIABLE) {
166+
const variableName = argumentNode.value.name.value;
190167
throw new GraphQLError(
191-
`Argument "${name}" has invalid value ${print(valueNode)}.`,
168+
`Argument "${name}" of required type "${String(argType)}" ` +
169+
`was provided the variable "$${variableName}" ` +
170+
'which was not provided a runtime value.',
192171
[argumentNode.value],
193172
);
173+
} else {
174+
throw new GraphQLError(
175+
`Argument "${name}" of required type "${String(argType)}" ` +
176+
'was not provided.',
177+
[node],
178+
);
179+
}
180+
} else if (hasValue) {
181+
if (argumentNode.value.kind === Kind.NULL) {
182+
// If the explicit value `null` was provided, an entry in the coerced
183+
// values must exist as the value `null`.
184+
coercedValues[name] = null;
185+
} else if (argumentNode.value.kind === Kind.VARIABLE) {
186+
const variableName = argumentNode.value.name.value;
187+
invariant(variableValues, 'Must exist for hasValue to be true.');
188+
// Note: This does no further checking that this variable is correct.
189+
// This assumes that this query has been validated and the variable
190+
// usage here is of the correct type.
191+
coercedValues[name] = variableValues[variableName];
192+
} else {
193+
const valueNode = argumentNode.value;
194+
const coercedValue = valueFromAST(valueNode, argType, variableValues);
195+
if (coercedValue === undefined) {
196+
// Note: ValuesOfCorrectType validation should catch this before
197+
// execution. This is a runtime check to ensure execution does not
198+
// continue with an invalid argument value.
199+
throw new GraphQLError(
200+
`Argument "${name}" has invalid value ${print(valueNode)}.`,
201+
[argumentNode.value],
202+
);
203+
}
204+
coercedValues[name] = coercedValue;
194205
}
195-
coercedValues[name] = coercedValue;
196206
}
197207
}
198208
return coercedValues;

‎src/index.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ export {
285285
NoUnusedVariablesRule,
286286
OverlappingFieldsCanBeMergedRule,
287287
PossibleFragmentSpreadsRule,
288-
ProvidedNonNullArgumentsRule,
288+
ProvidedRequiredArgumentsRule,
289289
ScalarLeafsRule,
290290
SingleFieldSubscriptionsRule,
291291
UniqueArgumentNamesRule,
@@ -296,7 +296,6 @@ export {
296296
UniqueVariableNamesRule,
297297
ValuesOfCorrectTypeRule,
298298
VariablesAreInputTypesRule,
299-
VariablesDefaultValueAllowedRule,
300299
VariablesInAllowedPositionRule,
301300
} from './validation';
302301

0 commit comments

Comments
 (0)