Skip to content

Commit c1076c3

Browse files
committed
SPEC/BUG: Ambiguity with null variable values and default values
This change corresponds to a spec proposal which solves an ambiguity in how variable values and default values behave with explicit null values. Otherwise, this ambiguity allows for null values to appear in non-null argument values, which may result in unforseen null-pointer-errors. This appears in three distinct but related issues: **VariablesInAllowedPosition validation rule** The explicit value `null` may be used as a default value for a variable, however `VariablesInAllowedPositions` allowed a nullable type with a default value to flow into an argument expecting a non-null type. This validation rule must explicitly not allow `null` default values to flow in this manner. **Coercing Variable Values** coerceVariableValues allows the explicit `null` value to be used over a default value, which can result in flowing a null value to a non-null argument when paired with the validation rule mentioned above. Instead a default value must be used even when an explicit `null` value is provided. **Coercing Argument Values** coerceArgumentValues allows the explicit `null` default value to be used before checking for a non-null type. This could inadvertently allow a null value into a non-null typed argument.
1 parent 4dfb993 commit c1076c3

File tree

5 files changed

+343
-29
lines changed

5 files changed

+343
-29
lines changed

src/execution/__tests__/nonnull-test.js

+188
Original file line numberDiff line numberDiff line change
@@ -486,4 +486,192 @@ describe('Execute: handles non-nullable types', () => {
486486
],
487487
},
488488
);
489+
490+
describe('Handles non-null argument', () => {
491+
const schemaWithNonNullArg = new GraphQLSchema({
492+
query: new GraphQLObjectType({
493+
name: 'Query',
494+
fields: {
495+
withNonNullArg: {
496+
type: GraphQLString,
497+
args: {
498+
cannotBeNull: {
499+
type: GraphQLNonNull(GraphQLString),
500+
defaultValue: null,
501+
},
502+
},
503+
resolve: async (_, args) => {
504+
if (typeof args.cannotBeNull === 'string') {
505+
return 'Passed: ' + args.cannotBeNull;
506+
}
507+
},
508+
},
509+
},
510+
}),
511+
});
512+
513+
it('succeeds when passed non-null literal value', async () => {
514+
const result = await execute({
515+
schema: schemaWithNonNullArg,
516+
document: parse(`
517+
query {
518+
withNonNullArg (cannotBeNull: "literal value")
519+
}
520+
`),
521+
});
522+
523+
expect(result).to.deep.equal({
524+
data: {
525+
withNonNullArg: 'Passed: literal value',
526+
},
527+
});
528+
});
529+
530+
it('succeeds when passed non-null variable value', async () => {
531+
const result = await execute({
532+
schema: schemaWithNonNullArg,
533+
document: parse(`
534+
query ($testVar: String!) {
535+
withNonNullArg (cannotBeNull: $testVar)
536+
}
537+
`),
538+
variableValues: {
539+
testVar: 'variable value',
540+
},
541+
});
542+
543+
expect(result).to.deep.equal({
544+
data: {
545+
withNonNullArg: 'Passed: variable value',
546+
},
547+
});
548+
});
549+
550+
it('succeeds when missing variable has default value', async () => {
551+
const result = await execute({
552+
schema: schemaWithNonNullArg,
553+
document: parse(`
554+
query ($testVar: String = "default value") {
555+
withNonNullArg (cannotBeNull: $testVar)
556+
}
557+
`),
558+
variableValues: {
559+
// Intentionally missing variable
560+
},
561+
});
562+
563+
expect(result).to.deep.equal({
564+
data: {
565+
withNonNullArg: 'Passed: default value',
566+
},
567+
});
568+
});
569+
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+
590+
it('field error when missing non-null arg', async () => {
591+
// Note: validation should identify this issue first (missing args rule)
592+
// however execution should still protect against this.
593+
const result = await execute({
594+
schema: schemaWithNonNullArg,
595+
document: parse(`
596+
query {
597+
withNonNullArg
598+
}
599+
`),
600+
});
601+
602+
expect(result).to.deep.equal({
603+
data: {
604+
withNonNullArg: null,
605+
},
606+
errors: [
607+
{
608+
message:
609+
'Argument "cannotBeNull" of required type "String!" was not provided.',
610+
locations: [{ line: 3, column: 13 }],
611+
path: ['withNonNullArg'],
612+
},
613+
],
614+
});
615+
});
616+
617+
it('field error when non-null arg provided null', async () => {
618+
// Note: validation should identify this issue first (values of correct
619+
// type rule) however execution should still protect against this.
620+
const result = await execute({
621+
schema: schemaWithNonNullArg,
622+
document: parse(`
623+
query {
624+
withNonNullArg(cannotBeNull: null)
625+
}
626+
`),
627+
});
628+
629+
expect(result).to.deep.equal({
630+
data: {
631+
withNonNullArg: null,
632+
},
633+
errors: [
634+
{
635+
message:
636+
'Argument "cannotBeNull" of non-null type "String!" must ' +
637+
'not be null.',
638+
locations: [{ line: 3, column: 42 }],
639+
path: ['withNonNullArg'],
640+
},
641+
],
642+
});
643+
});
644+
645+
it('field error when non-null arg not provided variable value', async () => {
646+
// Note: validation should identify this issue first (variables in allowed
647+
// position rule) however execution should still protect against this.
648+
const result = await execute({
649+
schema: schemaWithNonNullArg,
650+
document: parse(`
651+
query ($testVar: String) {
652+
withNonNullArg(cannotBeNull: $testVar)
653+
}
654+
`),
655+
variableValues: {
656+
// Intentionally missing variable
657+
},
658+
});
659+
660+
expect(result).to.deep.equal({
661+
data: {
662+
withNonNullArg: null,
663+
},
664+
errors: [
665+
{
666+
message:
667+
'Argument "cannotBeNull" of required type "String!" was ' +
668+
'provided the variable "$testVar" which was not provided a ' +
669+
'runtime value.',
670+
locations: [{ line: 3, column: 42 }],
671+
path: ['withNonNullArg'],
672+
},
673+
],
674+
});
675+
});
676+
});
489677
});

src/execution/__tests__/variables-test.js

+77-6
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,10 @@ const TestType = new GraphQLObjectType({
8484
type: GraphQLString,
8585
defaultValue: 'Hello World',
8686
}),
87+
fieldWithNonNullableStringInputAndDefaultArgumentValue: fieldWithInputArg({
88+
type: GraphQLNonNull(GraphQLString),
89+
defaultValue: 'Unreachable',
90+
}),
8791
fieldWithNestedInputObject: fieldWithInputArg({
8892
type: TestNestedInputObject,
8993
defaultValue: 'Hello World',
@@ -236,6 +240,55 @@ describe('Execute: Handles inputs', () => {
236240
});
237241
});
238242

243+
it('does not use default value when provided', () => {
244+
const result = executeQuery(
245+
`query q($input: String = "Default value") {
246+
fieldWithNullableStringInput(input: $input)
247+
}`,
248+
{ input: 'Variable value' },
249+
);
250+
251+
expect(result).to.deep.equal({
252+
data: {
253+
fieldWithNullableStringInput: "'Variable value'",
254+
},
255+
});
256+
});
257+
258+
it('uses default value when explicit null value provided', () => {
259+
const result = executeQuery(
260+
`
261+
query q($input: String = "Default value") {
262+
fieldWithNullableStringInput(input: $input)
263+
}`,
264+
{ input: null },
265+
);
266+
267+
expect(result).to.deep.equal({
268+
data: {
269+
fieldWithNullableStringInput: "'Default value'",
270+
},
271+
});
272+
});
273+
274+
it('uses null default value when not provided', () => {
275+
const result = executeQuery(
276+
`
277+
query q($input: String = null) {
278+
fieldWithNullableStringInput(input: $input)
279+
}`,
280+
{
281+
// Intentionally missing variable values.
282+
},
283+
);
284+
285+
expect(result).to.deep.equal({
286+
data: {
287+
fieldWithNullableStringInput: 'null',
288+
},
289+
});
290+
});
291+
239292
it('properly parses single value to list', () => {
240293
const params = { input: { a: 'foo', b: 'bar', c: 'baz' } };
241294
const result = executeQuery(doc, params);
@@ -492,8 +545,7 @@ describe('Execute: Handles inputs', () => {
492545
errors: [
493546
{
494547
message:
495-
'Variable "$value" got invalid value null; ' +
496-
'Expected non-nullable type String! not to be null.',
548+
'Variable "$value" of non-null type "String!" must not be null.',
497549
locations: [{ line: 2, column: 16 }],
498550
path: undefined,
499551
},
@@ -653,8 +705,7 @@ describe('Execute: Handles inputs', () => {
653705
errors: [
654706
{
655707
message:
656-
'Variable "$input" got invalid value null; ' +
657-
'Expected non-nullable type [String]! not to be null.',
708+
'Variable "$input" of non-null type "[String]!" must not be null.',
658709
locations: [{ line: 2, column: 16 }],
659710
path: undefined,
660711
},
@@ -739,8 +790,7 @@ describe('Execute: Handles inputs', () => {
739790
errors: [
740791
{
741792
message:
742-
'Variable "$input" got invalid value null; ' +
743-
'Expected non-nullable type [String!]! not to be null.',
793+
'Variable "$input" of non-null type "[String!]!" must not be null.',
744794
locations: [{ line: 2, column: 16 }],
745795
path: undefined,
746796
},
@@ -868,5 +918,26 @@ describe('Execute: Handles inputs', () => {
868918
],
869919
});
870920
});
921+
922+
it('not when argument type is non-null', async () => {
923+
const ast = parse(`query optionalVariable($optional: String) {
924+
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional)
925+
}`);
926+
927+
expect(await execute(schema, ast)).to.deep.equal({
928+
data: {
929+
fieldWithNonNullableStringInputAndDefaultArgumentValue: null,
930+
},
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+
],
940+
});
941+
});
871942
});
872943
});

0 commit comments

Comments
 (0)