Skip to content

Allow buildASTSchema to throw errors with source locations. #722

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

Closed
wants to merge 4 commits into from
Closed
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
35 changes: 27 additions & 8 deletions src/error/GraphQLError.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,33 @@ export function GraphQLError( // eslint-disable-line no-redeclare
path?: ?Array<string | number>,
originalError?: ?Error
) {
// Define message so it can be captured in stack trace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I misunderstood, but any reason why this was moved to the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asiandrummer Ah, yeah this is the rebase over #718. I needed this to add the test case for the validation error—otherwise the message reports as just "GraphQLError" and we can't meaningfully test it.

Object.defineProperty(this, 'message', {
value: message,
// By being enumerable, JSON.stringify will include `message` in the
// resulting output. This ensures that the simplest possible GraphQL
// service adheres to the spec.
enumerable: true,
writable: true
});

// Include (non-enumerable) stack trace.
if (originalError && originalError.stack) {
Object.defineProperty(this, 'stack', {
value: originalError.stack,
writable: true,
configurable: true
});
} else if (Error.captureStackTrace) {
Error.captureStackTrace(this, GraphQLError);
} else {
Object.defineProperty(this, 'stack', {
value: Error().stack,
writable: true,
configurable: true
});
}

// Compute locations in the source for the given nodes/positions.
let _source = source;
if (!_source && nodes && nodes.length > 0) {
Expand All @@ -109,14 +136,6 @@ export function GraphQLError( // eslint-disable-line no-redeclare
}

Object.defineProperties(this, {
message: {
value: message,
// By being enumerable, JSON.stringify will include `message` in the
// resulting output. This ensures that the simplest possible GraphQL
// service adheres to the spec.
enumerable: true,
writable: true
},
locations: {
// Coercing falsey values to undefined ensures they will not be included
// in JSON.stringify() when not provided.
Expand Down
51 changes: 44 additions & 7 deletions src/error/syntaxError.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,29 @@
import { getLocation } from '../language/location';
import type { Source } from '../language/source';
import { GraphQLError } from './GraphQLError';
import type { ASTNode } from '../language/ast';

/**
* Produces a string for formatting a syntax or validation error with an
* embedded location.
*/
export function printLocatedError(
source: Source,
position: number,
message: string,
description?: string,
): GraphQLError {
const location = getLocation(source, position);
const body = `${message} (${location.line}:${location.column})` +
(description ? ' ' + description : '') +
'\n\n' + highlightSourceAtLocation(source, location);
return new GraphQLError(
body,
undefined,
source,
[ position ]
);
}

/**
* Produces a GraphQLError representing a syntax error, containing useful
Expand All @@ -21,15 +44,29 @@ export function syntaxError(
position: number,
description: string
): GraphQLError {
const location = getLocation(source, position);
const error = new GraphQLError(
`Syntax Error ${source.name} (${location.line}:${location.column}) ` +
description + '\n\n' + highlightSourceAtLocation(source, location),
undefined,
return printLocatedError(
source,
[ position ]
position,
`Syntax Error ${source.name}`,
description,
);
return error;
}

/**
* Produces a GraphQLError for the invariant(...) function that renders the
* location where a validation error occurred. If no source is passed in, it
* returns an error containing the message, without context.
*/
export function validationError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used from buildASTSchema, so perhaps it should live there? - Also perhaps this should be "Schema Error" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, in the rebased diff it is now used in buildASTSchema.js and definition.js.

source: ?Source,
node: ?ASTNode,
message: string,
): GraphQLError {
const position = node ? (node.loc ? node.loc.start : null) : null;
if (position == null || source == null) {
return new GraphQLError(message);
}
return printLocatedError(source, position, `Validation Error: ${message}`);
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/jsutils/invariant.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
* of patent rights can be found in the PATENTS file in the same directory.
*/

export default function invariant(condition: mixed, message: string) {
export default function invariant(condition: mixed, message: string | Error) {
if (!condition) {
if (message instanceof Error) {
throw message;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit is unnecessary - no need for this change, then you can change the invariants below to just be if (!condition) { throw validationError() }

throw new Error(message);
}
}
33 changes: 23 additions & 10 deletions src/type/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import type {
ValueNode,
} from '../language/ast';
import type { GraphQLSchema } from './schema';
import { validationError } from '../error/syntaxError';

import type { Source } from '../language/source';
import type { ASTNode } from '../language/ast';

// Predicates & Assertions

Expand Down Expand Up @@ -82,11 +85,16 @@ export function isInputType(type: ?GraphQLType): boolean %checks {
);
}

export function assertInputType(type: ?GraphQLType): GraphQLInputType {
invariant(
isInputType(type),
`Expected ${String(type)} to be a GraphQL input type.`
);
export function assertInputType(
type: ?GraphQLType,
typeNode?: ASTNode,
source?: Source
): GraphQLInputType {
invariant(isInputType(type),
validationError(
source,
typeNode,
`Expected ${String(type)} to be a GraphQL input type.`));
return type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods are used throughout and not only in validation, so instead anywhere doing schema validation should be using isInputType and throwing the appropriate error instead of this general purpose function changing

}

Expand Down Expand Up @@ -121,11 +129,16 @@ export function isOutputType(type: ?GraphQLType): boolean %checks {
);
}

export function assertOutputType(type: ?GraphQLType): GraphQLOutputType {
invariant(
isOutputType(type),
`Expected ${String(type)} to be a GraphQL output type.`,
);
export function assertOutputType(
type: ?GraphQLType,
typeNode?: ASTNode,
source?: Source
): GraphQLOutputType {
invariant(isOutputType(type),
validationError(
source,
typeNode,
`Expected ${String(type)} to be a GraphQL output type.`));
return type;
}

Expand Down
26 changes: 26 additions & 0 deletions src/utilities/__tests__/buildASTSchema-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
GraphQLSkipDirective,
GraphQLIncludeDirective,
GraphQLDeprecatedDirective,
Source,
} from '../../';

/**
Expand Down Expand Up @@ -819,5 +820,30 @@ type Repeated {
const doc = parse(body);
expect(() => buildASTSchema(doc))
.to.throw('Type "Repeated" was defined more than once.');

it('warns with a location where validation errors occur', () => {
const body = new Source(`type OutputType {
value: String
}

input InputType {
value: String
}

type Query {
output: [OutputType]
}

type Mutation {
signup (password: OutputType): OutputType
}`);
const doc = parse(body);
expect(() => buildASTSchema(doc, body))
.to.throw(`GraphQLError: Validation Error: Expected Input type (14:25)

13: type Mutation {
14: signup (password: OutputType): OutputType
^
15: }`);
});
});
21 changes: 14 additions & 7 deletions src/utilities/buildASTSchema.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ import keyValMap from '../jsutils/keyValMap';
import { valueFromAST } from './valueFromAST';
import { TokenKind } from '../language/lexer';
import { parse } from '../language/parser';
import type { Source } from '../language/source';
import { Source } from '../language/source';
import { getArgumentValues } from '../execution/values';
import { validationError } from '../error/syntaxError';

import {
LIST_TYPE,
Expand Down Expand Up @@ -135,7 +136,10 @@ function getNamedTypeNode(typeNode: TypeNode): NamedTypeNode {
* Given that AST it constructs a GraphQLSchema. The resulting schema
* has no resolve methods, so execution will use default resolvers.
*/
export function buildASTSchema(ast: DocumentNode): GraphQLSchema {
export function buildASTSchema(
ast: DocumentNode,
source?: Source
): GraphQLSchema {
if (!ast || ast.kind !== DOCUMENT) {
throw new Error('Must provide a document ast.');
}
Expand Down Expand Up @@ -303,22 +307,24 @@ export function buildASTSchema(ast: DocumentNode): GraphQLSchema {
}

function produceInputType(typeNode: TypeNode): GraphQLInputType {
return assertInputType(produceType(typeNode));
return assertInputType(produceType(typeNode), typeNode, source);
}

function produceOutputType(typeNode: TypeNode): GraphQLOutputType {
return assertOutputType(produceType(typeNode));
return assertOutputType(produceType(typeNode), typeNode, source);
}

function produceObjectType(typeNode: TypeNode): GraphQLObjectType {
const type = produceType(typeNode);
invariant(type instanceof GraphQLObjectType, 'Expected Object type.');
invariant(type instanceof GraphQLObjectType,
validationError(source, typeNode, 'Expected Object type'));
return type;
}

function produceInterfaceType(typeNode: TypeNode): GraphQLInterfaceType {
const type = produceType(typeNode);
invariant(type instanceof GraphQLInterfaceType, 'Expected Interface type.');
invariant(type instanceof GraphQLInterfaceType,
validationError(source, typeNode, 'Expected Interface type'));
return type;
}

Expand Down Expand Up @@ -524,7 +530,8 @@ export function getDescription(node: { loc?: Location }): ?string {
* document.
*/
export function buildSchema(source: string | Source): GraphQLSchema {
return buildASTSchema(parse(source));
const sourceObj = typeof source === 'string' ? new Source(source) : source;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid recreating the Source object many times in invariantError, buildASTSchema now does so explicitly. But is this necessary?

Maybe we could construct a custom function globally to do this. I don't have a good answer for this as well, but yeah, passing an argument just for invariantError (or whatever we'll end up with) seems a bit unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that ast nodes include a reference to the source they come from, so you shouldn't need to pass the second argument around

return buildASTSchema(parse(sourceObj), sourceObj);
}

// Count the number of spaces on the starting side of a string.
Expand Down