Skip to content

Enforces input coercion rules. #553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 3, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/error/locatedError.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function locatedError(
): GraphQLError {
// Note: this uses a brand-check to support GraphQL errors originating from
// other contexts.
if (originalError && originalError.locations) {
if (originalError && originalError.path) {
return (originalError: any);
}

Expand All @@ -32,9 +32,9 @@ export function locatedError(
'An unknown error occurred.';
return new GraphQLError(
message,
nodes,
undefined,
undefined,
originalError && (originalError: any).nodes || nodes,
originalError && (originalError: any).source,
originalError && (originalError: any).positions,
path,
originalError
);
Expand Down
6 changes: 4 additions & 2 deletions src/execution/__tests__/abstract-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,8 @@ describe('Execute: Handles execution of abstract types', () => {
{
message:
'Runtime Object type "Human" is not a possible type for "Pet".',
locations: [ { line: 2, column: 7 } ]
locations: [ { line: 2, column: 7 } ],
path: [ 'pets', 2 ]
}
]
});
Expand Down Expand Up @@ -347,7 +348,8 @@ describe('Execute: Handles execution of abstract types', () => {
{
message:
'Runtime Object type "Human" is not a possible type for "Pet".',
locations: [ { line: 2, column: 7 } ]
locations: [ { line: 2, column: 7 } ],
path: [ 'pets', 2 ]
}
]
});
Expand Down
94 changes: 78 additions & 16 deletions src/execution/__tests__/variables-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,24 +199,30 @@ describe('Execute: Handles inputs', () => {

const result = await execute(schema, ast);

expect(result).to.deep.equal({
expect(result).to.containSubset({
data: {
fieldWithObjectInput: null
}
},
errors: [ {
message:
'Argument "input" got invalid value ["foo", "bar", "baz"].\n' +
'Expected "TestInputObject", found not an object.',
path: [ 'fieldWithObjectInput' ]
} ]
});
});

it('properly runs parseLiteral on complex scalar types', async () => {
const doc = `
{
fieldWithObjectInput(input: {a: "foo", d: "SerializedValue"})
fieldWithObjectInput(input: {c: "foo", d: "SerializedValue"})
}
`;
const ast = parse(doc);

return expect(await execute(schema, ast)).to.deep.equal({
data: {
fieldWithObjectInput: '{"a":"foo","d":"DeserializedValue"}',
fieldWithObjectInput: '{"c":"foo","d":"DeserializedValue"}',
}
});
});
Expand Down Expand Up @@ -482,7 +488,21 @@ describe('Execute: Handles inputs', () => {
});
});

describe('Handles non-nullable scalars', () => {
describe('Handles non-nullable scalars', async () => {
it('allows non-nullable inputs to be omitted given a default', async () => {
const doc = `
query SetsNonNullable($value: String = "default") {
fieldWithNonNullableStringInput(input: $value)
}
`;

expect(await execute(schema, parse(doc))).to.deep.equal({
data: {
fieldWithNonNullableStringInput: '"default"'
}
});
});

it('does not allow non-nullable inputs to be omitted in a variable', async () => {
const doc = `
query SetsNonNullable($value: String!) {
Expand Down Expand Up @@ -522,7 +542,8 @@ describe('Execute: Handles inputs', () => {
expect(caughtError).to.containSubset({
locations: [ { line: 2, column: 31 } ],
message:
'Variable "$value" of required type "String!" was not provided.'
'Variable "$value" got invalid value null.\n' +
'Expected "String!", found null.'
});
});

Expand Down Expand Up @@ -558,7 +579,7 @@ describe('Execute: Handles inputs', () => {
});
});

it('passes along null for non-nullable inputs if explcitly set in the query', async () => {
it('reports error for missing non-nullable inputs', async () => {
const doc = `
{
fieldWithNonNullableStringInput
Expand All @@ -569,7 +590,39 @@ describe('Execute: Handles inputs', () => {
return expect(await execute(schema, ast)).to.deep.equal({
data: {
fieldWithNonNullableStringInput: null
}
},
errors: [ {
message: 'Argument "input" of required type "String!" was not provided.',
locations: [ { line: 3, column: 9 } ],
path: [ 'fieldWithNonNullableStringInput' ]
} ]
});
});

it('reports error for non-provided variables for non-nullable inputs', async () => {
// Note: this test would typically fail validation before encountering
// this execution error, however for queries which previously validated
// and are being run against a new schema which have introduced a breaking
// change to make a formerly non-required argument required, this asserts
// failure before allowing the underlying code to receive a non-null value.
const doc = `
{
fieldWithNonNullableStringInput(input: $foo)
}
`;
const ast = parse(doc);

return expect(await execute(schema, ast)).to.deep.equal({
data: {
fieldWithNonNullableStringInput: null
},
errors: [ {
message:
'Argument "input" of required type "String!" was provided the ' +
'variable "$foo" which was not provided a runtime value.',
locations: [ { line: 3, column: 48 } ],
path: [ 'fieldWithNonNullableStringInput' ]
} ]
});
});
});
Expand Down Expand Up @@ -644,7 +697,8 @@ describe('Execute: Handles inputs', () => {
expect(caughtError).to.containSubset({
locations: [ { line: 2, column: 17 } ],
message:
'Variable "$input" of required type "[String]!" was not provided.'
'Variable "$input" got invalid value null.\n' +
'Expected "[String]!", found null.'
});
});

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

Expand Down Expand Up @@ -820,7 +875,7 @@ describe('Execute: Handles inputs', () => {
}

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

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

it('when nullable variable provided', async () => {
it('when omitted variable provided', async () => {
const ast = parse(`query optionalVariable($optional: String) {
fieldWithDefaultArgumentValue(input: $optional)
}`);
Expand All @@ -879,15 +934,22 @@ describe('Execute: Handles inputs', () => {
});
});

it('when argument provided cannot be parsed', async () => {
it('not when argument cannot be coerced', async () => {
const ast = parse(`{
fieldWithDefaultArgumentValue(input: WRONG_TYPE)
}`);

return expect(await execute(schema, ast)).to.deep.equal({
data: {
fieldWithDefaultArgumentValue: '"Hello World"'
}
fieldWithDefaultArgumentValue: null
},
errors: [ {
message:
'Argument "input" got invalid value WRONG_TYPE.\n' +
'Expected type "String", found WRONG_TYPE.',
locations: [ { line: 2, column: 46 } ],
path: [ 'fieldWithDefaultArgumentValue' ]
} ]
});
});

Expand Down
40 changes: 25 additions & 15 deletions src/execution/execute.js
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,8 @@ function shouldIncludeNode(
);
if (skipAST) {
const { if: skipIf } = getArgumentValues(
GraphQLSkipDirective.args,
skipAST.arguments,
GraphQLSkipDirective,
skipAST,
exeContext.variableValues
);
if (skipIf === true) {
Expand All @@ -473,8 +473,8 @@ function shouldIncludeNode(
);
if (includeAST) {
const { if: includeIf } = getArgumentValues(
GraphQLIncludeDirective.args,
includeAST.arguments,
GraphQLIncludeDirective,
includeAST,
exeContext.variableValues
);
if (includeIf === false) {
Expand Down Expand Up @@ -559,15 +559,6 @@ function resolveField(
const returnType = fieldDef.type;
const resolveFn = fieldDef.resolve || defaultResolveFn;

// Build a JS object of arguments from the field.arguments AST, using the
// variables scope to fulfill any variable references.
// TODO: find a way to memoize, in case this field is within a List type.
const args = getArgumentValues(
fieldDef.args,
fieldAST.arguments,
exeContext.variableValues
);

// The resolve function's optional third argument is a context value that
// is provided to every resolve function within an execution. It is commonly
// used to represent an authenticated user, or request-specific caches.
Expand All @@ -590,7 +581,15 @@ function resolveField(

// Get the resolve function, regardless of if its result is normal
// or abrupt (error).
const result = resolveOrError(resolveFn, source, args, context, info);
const result = resolveOrError(
exeContext,
fieldDef,
fieldAST,
resolveFn,
source,
context,
info
);

return completeValueCatchingError(
exeContext,
Expand All @@ -605,13 +604,24 @@ function resolveField(
// Isolates the "ReturnOrAbrupt" behavior to not de-opt the `resolveField`
// function. Returns the result of resolveFn or the abrupt-return Error object.
function resolveOrError(
exeContext: ExecutionContext,
fieldDef: GraphQLFieldDefinition,
fieldAST: Field,
resolveFn: GraphQLFieldResolveFn<*>,
source: mixed,
args: { [key: string]: mixed },
context: mixed,
info: GraphQLResolveInfo
): Error | mixed {
try {
// Build a JS object of arguments from the field.arguments AST, using the
// variables scope to fulfill any variable references.
// TODO: find a way to memoize, in case this field is within a List type.
const args = getArgumentValues(
fieldDef,
fieldAST,
exeContext.variableValues
);

return resolveFn(source, args, context, info);
} catch (error) {
// Sometimes a non-error is thrown, wrap it as an Error for a
Expand Down
Loading