Allow buildASTSchema to throw errors with source locations. #722

tcr · 2017-02-17T16:58:03Z

See #546 for context. This patch is one possible implementation of having type errors in a schema list their source location. Given the following code:

const { graphql, buildSchema } = require('graphql');

const schema = buildSchema(`
  type OutputType {
    value: String
  }

  input InputType {
    value: String
  }

  type Query {
    output: [OutputType]
  }

  type Mutation {
    signup (password: OutputType): OutputType
  }
`)

Instead of this error:

$ node schema.js
/Users/trim/github/graphql-js/dist/jsutils/invariant.js:19
    throw new Error(message);
    ^

Error: Expected Input type
    at invariant (/Users/trim/github/graphql-js/dist/jsutils/invariant.js:19:11)
    ...

It would throw this:

$ node schema.js
/Users/trim/github/graphql-js/dist/jsutils/invariant.js:19
    throw new Error(message);
    ^

Error: Expected Input type (15:23)

14:   type Mutation {
15:     signup (password: OutputType): OutputType
                          ^
16:   }

    at invariant (/Users/trim/github/graphql-js/dist/jsutils/invariant.js:19:11)
    ....

Caveats:

We'd have to thread the Source object through buildASTSchema in order to have content for line numbers and the source error itself. In this implementation, I made this optional so buildASTSchema just uses it as context for the error if it's provided.
In order to avoid recreating the Source object many times in invariantError, buildASTSchema now does so explicitly. But is this necessary?
This makes our invariant calls more costly, because it constructs the error message and deduces the line numbers & syntax highlighting whether or not it's thrown.
We have to give the syntaxError module knowledge of the AST, though with a bit of work this implementation could pass in a position instead.
The implementation invariantError I chose here is pretty weak, there's probably a better pattern for wrapping invariant(...) we could use.
Can we merge the formatting logic of syntaxError to use invariantError?
Lastly, I didn't apply the invariantError to all functions, just a handful. That would have to be finished before merge.

asiandrummer

@tcr I think this is great. Lee and I just chatted about having a general util function for handling type errors with source location, used in extending/building the schema - and I think this is very close to (or exactly) what we need.

asiandrummer · 2017-03-15T23:09:47Z

src/error/syntaxError.js

@@ -33,6 +34,25 @@ export function syntaxError(
 }

 /**
+ * Produces a string for the invarant(...) function that renders the location


Maybe we can even refactor these functions to be more generic:

export function formatError(errorType: string) { ... } export function syntaxError() { return formatError('syntax'); } export function validationError() { return formatError('validation'); }

Not exactly like that, but you get the gist

asiandrummer · 2017-03-15T23:10:16Z

src/error/syntaxError.js

+    return message;
+  }
+  const location = getLocation(source, position);
+  return `${message} (${location.line}:${location.column}) ` +


I think this can actually return new GraphQLError(...)

@asiandrummer This prints strange output from the invariant function, which inlined would be:

if (!condition) { throw new Error(new GraphQLError(...)); }

But, I could change invariant(...) to check first if its message argument is a string, and if it's actually an error, just rethrow. Then I could make validationError return a GraphQLError. Sensible?

asiandrummer · 2017-03-15T23:27:55Z

src/utilities/buildASTSchema.js

@@ -518,7 +526,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;


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.

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

asiandrummer · 2017-03-15T23:31:52Z

src/error/syntaxError.js

+ * where an error occurred. If no source is passed in, it renders the message
+ * without context.
+ */
+export function invariantError(


eh, maybe typeError/validationError?

tcr · 2017-03-20T15:53:08Z

Updated this PR:

This now rolls up Adds message to stack trace of GraphQLError. #718 to allow the GraphQLError test to function properly.
Allows invariant(...) to rethrow errors if they are passed in. This change can be reverted if we want validationError() to simply return a string.
Broke out formatError into its own method.

The caveat still exists that buildASTSchema requires both "Source" and "ast" to return validation errors with full source body.

asiandrummer · 2017-03-20T17:58:06Z

src/error/GraphQLError.js

@@ -77,6 +77,16 @@ export function GraphQLError( // eslint-disable-line no-redeclare
  path?: ?Array<string | number>,
  originalError?: ?Error
 ) {
+  // Define message so it can be captured in stack trace.


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

@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.

asiandrummer · 2017-03-20T17:59:23Z

src/error/syntaxError.js

+ * returns an error containing the message, without context.
+ */
+export function validationError(
+  message: string,


nit, but should we order the argument similar to the functions above? That probably means to move source to the top.

asiandrummer · 2017-03-20T18:00:08Z

This looks good to me - maybe @leebyron should have another look at this

leebyron · 2017-05-18T14:48:15Z

src/jsutils/invariant.js

  if (!condition) {
+    if (message instanceof Error) {
+      throw message;
+    }


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

leebyron · 2017-05-18T14:59:20Z

src/error/syntaxError.js

+ * Produces a string for formatting a syntax or validation error with an
+ * embedded location.
+ */
+export function formatError(


We already have another function called formatError which does something slightly different.

How about printLocatedError?

leebyron · 2017-05-18T15:00:21Z

src/error/syntaxError.js

+ * location where a validation error occurred. If no source is passed in, it
+ * returns an error containing the message, without context.
+ */
+export function validationError(


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

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

leebyron · 2017-05-18T15:01:43Z

Sorry for not seeing this sooner! This is great! There is also some discussion in #746 which might lead to even more coverage of schema validation improvements like this one.

Do you mind rebasing?

tcr · 2017-05-18T17:20:50Z

Rebased—let me know if anything got lost in the transition.

cjoudrey · 2017-12-01T00:31:03Z

@tcr was your intention to replace all other cases of throw new Error with an error class that includes location of the mistake?

graphql-js/src/utilities/buildASTSchema.js

Lines 153 to 159 in 37717b8

    
           switch (d.kind) { 
        
             case Kind.SCHEMA_DEFINITION: 
        
               if (schemaDef) { 
        
                 throw new Error('Must provide only one schema definition.'); 
        
               } 
        
               schemaDef = d; 
        
               break;

For example, in the above snippet it would be great that the error that is thrown includes the location of both schema definition AST nodes.

This would allow tools (like schema linters) to surface this information to the user. At the moment this isn't possible because a generic Error with only a message is thrown.

Happy to help if you don't have time to ship this.

leebyron

I really want to have this, but perhaps we could make this more generic to apply to all schema, not just those created in buildASTSchema?

leebyron · 2017-12-04T20:46:34Z

src/type/definition.js

+    validationError(
+      source,
+      typeNode,
+      `Expected ${String(type)} to be a GraphQL input type.`));
  return type;


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

leebyron · 2017-12-04T20:47:14Z

src/utilities/buildASTSchema.js

@@ -518,7 +526,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;


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

Lifted from / inspired by a similar change in #722, this creates a new function `printError()` (and uses it as the implementation for `GraphQLError#toString()`) which prints location information in the context of an error. This is moved from the syntax error where it used to be hard-coded, so it may now be used to format validation errors, value coercion errors, or any other error which may be associated with a location.

facebook-github-bot added the CLA Signed label Feb 17, 2017

tcr force-pushed the tcr-astschemasource branch from fa6ee06 to f5d48d7 Compare February 17, 2017 17:03

asiandrummer reviewed Mar 15, 2017

View reviewed changes

tcr force-pushed the tcr-astschemasource branch 3 times, most recently from d86f752 to be5244f Compare March 20, 2017 15:50

asiandrummer reviewed Mar 20, 2017

View reviewed changes

wincent mentioned this pull request Mar 20, 2017

Adds message to stack trace of GraphQLError. #718

Merged

asiandrummer reviewed Mar 20, 2017

View reviewed changes

leebyron reviewed May 18, 2017

View reviewed changes

tcr and others added 4 commits May 18, 2017 12:39

Adds message to stack trace of GraphQLError.

e54786d

Fix typo in comment

e53401c

Allows buildASTSchema to throw errors with source locations.

282b8c3

Corrects argument order in validationError.

e881215

tcr force-pushed the tcr-astschemasource branch from 8784d8c to e881215 Compare May 18, 2017 17:20

leebyron force-pushed the master branch from fda2afd to da8fe62 Compare May 26, 2017 16:58

leebyron requested changes Dec 4, 2017

View reviewed changes

leebyron mentioned this pull request Dec 7, 2017

Validate schema root types and directives #1124

Merged

leebyron mentioned this pull request Dec 8, 2017

New: printError() #1129

Merged

leebyron closed this in #1124 Dec 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow buildASTSchema to throw errors with source locations. #722

Allow buildASTSchema to throw errors with source locations. #722

tcr commented Feb 17, 2017 •

edited

Loading

asiandrummer left a comment

asiandrummer Mar 15, 2017

asiandrummer Mar 15, 2017

tcr Mar 20, 2017

asiandrummer Mar 15, 2017

leebyron Dec 4, 2017

asiandrummer Mar 15, 2017

tcr commented Mar 20, 2017

asiandrummer Mar 20, 2017

tcr Mar 20, 2017

asiandrummer Mar 20, 2017

asiandrummer commented Mar 20, 2017

leebyron May 18, 2017

leebyron May 18, 2017

leebyron May 18, 2017

tcr May 18, 2017

leebyron commented May 18, 2017

tcr commented May 18, 2017

cjoudrey commented Dec 1, 2017

leebyron left a comment

leebyron Dec 4, 2017

leebyron Dec 4, 2017

Allow buildASTSchema to throw errors with source locations. #722

Allow buildASTSchema to throw errors with source locations. #722

Conversation

tcr commented Feb 17, 2017 • edited Loading

asiandrummer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcr commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asiandrummer commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented May 18, 2017

tcr commented May 18, 2017

cjoudrey commented Dec 1, 2017

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcr commented Feb 17, 2017 •

edited

Loading