Skip to content

Commit b93f049

Browse files
authored
Enforces input coercion rules. (#553)
* Enforces input coercion rules. Before this diff, bad input to arguments and variables was often ignored and replaced with `null` rather than rejected. Now that `null` has a semantic meaning, and thanks to some recent changes to the spec (graphql/graphql-spec#221) - changes are necessary in order to enforce the stricter coercion rules. This diff does the following: * Implements the CoerceArgumentValues as described in the spec. * Implements the CoerceVariablesValues as described in the spec. * Alters valueFromAST and coerceValue (dual functions) to strictly enforce coercion, returning `undefined` implicitly when they fail to do so. It also fixes issues where undefined returns were being ignored as items in a list or fields in an input object. * Fix most of the failing tests * Fix missing data from errors * fix lint issues * Add full behavior test cases for valueFromAST * additional value coercion tests * Add appropriate list item behavior for missing vars
1 parent 6cdb68c commit b93f049

File tree

10 files changed

+538
-157
lines changed

10 files changed

+538
-157
lines changed

src/error/locatedError.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ export function locatedError(
2323
): GraphQLError {
2424
// Note: this uses a brand-check to support GraphQL errors originating from
2525
// other contexts.
26-
if (originalError && originalError.locations) {
26+
if (originalError && originalError.path) {
2727
return (originalError: any);
2828
}
2929

@@ -32,9 +32,9 @@ export function locatedError(
3232
'An unknown error occurred.';
3333
return new GraphQLError(
3434
message,
35-
nodes,
36-
undefined,
37-
undefined,
35+
originalError && (originalError: any).nodes || nodes,
36+
originalError && (originalError: any).source,
37+
originalError && (originalError: any).positions,
3838
path,
3939
originalError
4040
);

src/execution/__tests__/abstract-test.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,8 @@ describe('Execute: Handles execution of abstract types', () => {
258258
{
259259
message:
260260
'Runtime Object type "Human" is not a possible type for "Pet".',
261-
locations: [ { line: 2, column: 7 } ]
261+
locations: [ { line: 2, column: 7 } ],
262+
path: [ 'pets', 2 ]
262263
}
263264
]
264265
});
@@ -347,7 +348,8 @@ describe('Execute: Handles execution of abstract types', () => {
347348
{
348349
message:
349350
'Runtime Object type "Human" is not a possible type for "Pet".',
350-
locations: [ { line: 2, column: 7 } ]
351+
locations: [ { line: 2, column: 7 } ],
352+
path: [ 'pets', 2 ]
351353
}
352354
]
353355
});

src/execution/__tests__/variables-test.js

Lines changed: 78 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -199,24 +199,30 @@ describe('Execute: Handles inputs', () => {
199199

200200
const result = await execute(schema, ast);
201201

202-
expect(result).to.deep.equal({
202+
expect(result).to.containSubset({
203203
data: {
204204
fieldWithObjectInput: null
205-
}
205+
},
206+
errors: [ {
207+
message:
208+
'Argument "input" got invalid value ["foo", "bar", "baz"].\n' +
209+
'Expected "TestInputObject", found not an object.',
210+
path: [ 'fieldWithObjectInput' ]
211+
} ]
206212
});
207213
});
208214

209215
it('properly runs parseLiteral on complex scalar types', async () => {
210216
const doc = `
211217
{
212-
fieldWithObjectInput(input: {a: "foo", d: "SerializedValue"})
218+
fieldWithObjectInput(input: {c: "foo", d: "SerializedValue"})
213219
}
214220
`;
215221
const ast = parse(doc);
216222

217223
return expect(await execute(schema, ast)).to.deep.equal({
218224
data: {
219-
fieldWithObjectInput: '{"a":"foo","d":"DeserializedValue"}',
225+
fieldWithObjectInput: '{"c":"foo","d":"DeserializedValue"}',
220226
}
221227
});
222228
});
@@ -482,7 +488,21 @@ describe('Execute: Handles inputs', () => {
482488
});
483489
});
484490

485-
describe('Handles non-nullable scalars', () => {
491+
describe('Handles non-nullable scalars', async () => {
492+
it('allows non-nullable inputs to be omitted given a default', async () => {
493+
const doc = `
494+
query SetsNonNullable($value: String = "default") {
495+
fieldWithNonNullableStringInput(input: $value)
496+
}
497+
`;
498+
499+
expect(await execute(schema, parse(doc))).to.deep.equal({
500+
data: {
501+
fieldWithNonNullableStringInput: '"default"'
502+
}
503+
});
504+
});
505+
486506
it('does not allow non-nullable inputs to be omitted in a variable', async () => {
487507
const doc = `
488508
query SetsNonNullable($value: String!) {
@@ -522,7 +542,8 @@ describe('Execute: Handles inputs', () => {
522542
expect(caughtError).to.containSubset({
523543
locations: [ { line: 2, column: 31 } ],
524544
message:
525-
'Variable "$value" of required type "String!" was not provided.'
545+
'Variable "$value" got invalid value null.\n' +
546+
'Expected "String!", found null.'
526547
});
527548
});
528549

@@ -558,7 +579,7 @@ describe('Execute: Handles inputs', () => {
558579
});
559580
});
560581

561-
it('passes along null for non-nullable inputs if explcitly set in the query', async () => {
582+
it('reports error for missing non-nullable inputs', async () => {
562583
const doc = `
563584
{
564585
fieldWithNonNullableStringInput
@@ -569,7 +590,39 @@ describe('Execute: Handles inputs', () => {
569590
return expect(await execute(schema, ast)).to.deep.equal({
570591
data: {
571592
fieldWithNonNullableStringInput: null
572-
}
593+
},
594+
errors: [ {
595+
message: 'Argument "input" of required type "String!" was not provided.',
596+
locations: [ { line: 3, column: 9 } ],
597+
path: [ 'fieldWithNonNullableStringInput' ]
598+
} ]
599+
});
600+
});
601+
602+
it('reports error for non-provided variables for non-nullable inputs', async () => {
603+
// Note: this test would typically fail validation before encountering
604+
// this execution error, however for queries which previously validated
605+
// and are being run against a new schema which have introduced a breaking
606+
// change to make a formerly non-required argument required, this asserts
607+
// failure before allowing the underlying code to receive a non-null value.
608+
const doc = `
609+
{
610+
fieldWithNonNullableStringInput(input: $foo)
611+
}
612+
`;
613+
const ast = parse(doc);
614+
615+
return expect(await execute(schema, ast)).to.deep.equal({
616+
data: {
617+
fieldWithNonNullableStringInput: null
618+
},
619+
errors: [ {
620+
message:
621+
'Argument "input" of required type "String!" was provided the ' +
622+
'variable "$foo" which was not provided a runtime value.',
623+
locations: [ { line: 3, column: 48 } ],
624+
path: [ 'fieldWithNonNullableStringInput' ]
625+
} ]
573626
});
574627
});
575628
});
@@ -644,7 +697,8 @@ describe('Execute: Handles inputs', () => {
644697
expect(caughtError).to.containSubset({
645698
locations: [ { line: 2, column: 17 } ],
646699
message:
647-
'Variable "$input" of required type "[String]!" was not provided.'
700+
'Variable "$input" got invalid value null.\n' +
701+
'Expected "[String]!", found null.'
648702
});
649703
});
650704

@@ -758,7 +812,8 @@ describe('Execute: Handles inputs', () => {
758812
expect(caughtError).to.containSubset({
759813
locations: [ { line: 2, column: 17 } ],
760814
message:
761-
'Variable "$input" of required type "[String!]!" was not provided.'
815+
'Variable "$input" got invalid value null.\n' +
816+
'Expected "[String!]!", found null.'
762817
});
763818
});
764819

@@ -820,7 +875,7 @@ describe('Execute: Handles inputs', () => {
820875
}
821876

822877
expect(caughtError).to.containSubset({
823-
locations: [ { line: 2, column: 17 } ],
878+
locations: [ { line: 2, column: 25 } ],
824879
message:
825880
'Variable "$input" expected value of type "TestType!" which cannot ' +
826881
'be used as an input type.'
@@ -844,7 +899,7 @@ describe('Execute: Handles inputs', () => {
844899
}
845900

846901
expect(caughtError).to.containSubset({
847-
locations: [ { line: 2, column: 17 } ],
902+
locations: [ { line: 2, column: 25 } ],
848903
message:
849904
'Variable "$input" expected value of type "UnknownType!" which ' +
850905
'cannot be used as an input type.'
@@ -867,7 +922,7 @@ describe('Execute: Handles inputs', () => {
867922
});
868923
});
869924

870-
it('when nullable variable provided', async () => {
925+
it('when omitted variable provided', async () => {
871926
const ast = parse(`query optionalVariable($optional: String) {
872927
fieldWithDefaultArgumentValue(input: $optional)
873928
}`);
@@ -879,15 +934,22 @@ describe('Execute: Handles inputs', () => {
879934
});
880935
});
881936

882-
it('when argument provided cannot be parsed', async () => {
937+
it('not when argument cannot be coerced', async () => {
883938
const ast = parse(`{
884939
fieldWithDefaultArgumentValue(input: WRONG_TYPE)
885940
}`);
886941

887942
return expect(await execute(schema, ast)).to.deep.equal({
888943
data: {
889-
fieldWithDefaultArgumentValue: '"Hello World"'
890-
}
944+
fieldWithDefaultArgumentValue: null
945+
},
946+
errors: [ {
947+
message:
948+
'Argument "input" got invalid value WRONG_TYPE.\n' +
949+
'Expected type "String", found WRONG_TYPE.',
950+
locations: [ { line: 2, column: 46 } ],
951+
path: [ 'fieldWithDefaultArgumentValue' ]
952+
} ]
891953
});
892954
});
893955

src/execution/execute.js

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -458,8 +458,8 @@ function shouldIncludeNode(
458458
);
459459
if (skipAST) {
460460
const { if: skipIf } = getArgumentValues(
461-
GraphQLSkipDirective.args,
462-
skipAST.arguments,
461+
GraphQLSkipDirective,
462+
skipAST,
463463
exeContext.variableValues
464464
);
465465
if (skipIf === true) {
@@ -473,8 +473,8 @@ function shouldIncludeNode(
473473
);
474474
if (includeAST) {
475475
const { if: includeIf } = getArgumentValues(
476-
GraphQLIncludeDirective.args,
477-
includeAST.arguments,
476+
GraphQLIncludeDirective,
477+
includeAST,
478478
exeContext.variableValues
479479
);
480480
if (includeIf === false) {
@@ -559,15 +559,6 @@ function resolveField(
559559
const returnType = fieldDef.type;
560560
const resolveFn = fieldDef.resolve || defaultResolveFn;
561561

562-
// Build a JS object of arguments from the field.arguments AST, using the
563-
// variables scope to fulfill any variable references.
564-
// TODO: find a way to memoize, in case this field is within a List type.
565-
const args = getArgumentValues(
566-
fieldDef.args,
567-
fieldAST.arguments,
568-
exeContext.variableValues
569-
);
570-
571562
// The resolve function's optional third argument is a context value that
572563
// is provided to every resolve function within an execution. It is commonly
573564
// used to represent an authenticated user, or request-specific caches.
@@ -590,7 +581,15 @@ function resolveField(
590581

591582
// Get the resolve function, regardless of if its result is normal
592583
// or abrupt (error).
593-
const result = resolveOrError(resolveFn, source, args, context, info);
584+
const result = resolveOrError(
585+
exeContext,
586+
fieldDef,
587+
fieldAST,
588+
resolveFn,
589+
source,
590+
context,
591+
info
592+
);
594593

595594
return completeValueCatchingError(
596595
exeContext,
@@ -605,13 +604,24 @@ function resolveField(
605604
// Isolates the "ReturnOrAbrupt" behavior to not de-opt the `resolveField`
606605
// function. Returns the result of resolveFn or the abrupt-return Error object.
607606
function resolveOrError(
607+
exeContext: ExecutionContext,
608+
fieldDef: GraphQLFieldDefinition,
609+
fieldAST: Field,
608610
resolveFn: GraphQLFieldResolveFn<*>,
609611
source: mixed,
610-
args: { [key: string]: mixed },
611612
context: mixed,
612613
info: GraphQLResolveInfo
613614
): Error | mixed {
614615
try {
616+
// Build a JS object of arguments from the field.arguments AST, using the
617+
// variables scope to fulfill any variable references.
618+
// TODO: find a way to memoize, in case this field is within a List type.
619+
const args = getArgumentValues(
620+
fieldDef,
621+
fieldAST,
622+
exeContext.variableValues
623+
);
624+
615625
return resolveFn(source, args, context, info);
616626
} catch (error) {
617627
// Sometimes a non-error is thrown, wrap it as an Error for a

0 commit comments

Comments
 (0)