graphql-schema-linter icon indicating copy to clipboard operation
graphql-schema-linter copied to clipboard

Cannot read property '0' of undefined

Open jbruni opened this issue 5 years ago • 4 comments

Similar to #170... but I have the "schema" I tried here.

First, the error message:

$ graphql-schema-linter schema.graphql
It looks like you may have hit a bug in graphql-schema-linter.

It would be super helpful if you could report this here: https://github.com/cjoudrey/graphql-schema-linter/issues/new

TypeError: Cannot read property '0' of undefined
    at /home/jbruni/.nvm/versions/node/v8.12.0/lib/node_modules/graphql-schema-linter/lib/runner.js:76:70
    at Array.reduce (<anonymous>)
    at groupErrorsBySchemaFilePath (/home/jbruni/.nvm/versions/node/v8.12.0/lib/node_modules/graphql-schema-linter/lib/runner.js:75:17)
    at run (/home/jbruni/.nvm/versions/node/v8.12.0/lib/node_modules/graphql-schema-linter/lib/runner.js:67:23)
    at Object.<anonymous> (/home/jbruni/.nvm/versions/node/v8.12.0/lib/node_modules/graphql-schema-linter/lib/cli.js:15:32)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)

Now, the schema.graphql contents:

type User {
  id: ID!
  email: String!
}

This is my very first time touching GraphQL... I've started small and simple... I'm sure there is something wrong here. :smile: But... Cannot read property '0' of undefined :thinking: What exactly is wrong with it?

Thanks!

jbruni avatar Mar 17 '19 06:03 jbruni

FYI... this is a snippet of a bigger schema, which I've taken from here: https://github.com/apollographql/fullstack-tutorial/blob/master/final/server/src/schema.js

And if I try linting the full schema, graphql-schema-linter provides its output without breaking.

jbruni avatar Mar 17 '19 06:03 jbruni

@jbruni Thanks for opening the issue and providing a schema to reproduce the issue. :smile:

type User {
  id: ID!
  email: String!
}

The problem with the schema you pasted above is that there is missing a Query type or a schema definition.

For example:

# A query root:

type Query {
  # ...
}

type User {
  id: ID!
  email: String!
}

# Or, a schema definition:

schema {
  query: MyCustomQueryRoot
}

type MyCustomQueryRoot {
  # ...
}

type User {
  id: ID!
  email: String!
}

I'll take a look at fixing this issue in the linter as it shouldn't be crashing.

cjoudrey avatar Mar 17 '19 17:03 cjoudrey

Hi mr @jbruni :) I've ran in the same problem, and after a little debugging it seems that there are some errors that have no locations in them, so this will break at any part of the code that have locations[0]. I was thinking at least 2 possibilities to handle this:

  1. Always test for locations ? locations[0] : location and if its undefined it will not be printed

  2. Normalized it to another property before the sort, with something like:

function normalizeLocation(errors) {
  return errors.map(error => {
    error.normalizedLocation = error.locations
      ? error.locations[0]
      : error.location || {};
    return error;
  });
}

And after that always test for the line property of this normalizedLocation.

I can try to create a PR to handle this issue, if you are ok with that, btw :)

clovislima avatar Mar 19 '19 13:03 clovislima

I believe you are correct @clovislima. :smile:

Most errors should have a location, but there are some exceptional cases where a location won't be present for example:

https://github.com/graphql/graphql-js/blob/c83412f19e88fda4815a26340127d938fd4f3b2f/src/type/validate.js#L117-L121

If you omit schema { .. } and type Query, then schema.astNode will be undefined.

In this case, I believe the linter should still display the error. Since we have no line/column information, I would default it to line 1 column 1 of the first file being linted.

I also realized when looking into this that GraphQLError.locations is a readonly array:

https://github.com/graphql/graphql-js/blob/c83412f19e88fda4815a26340127d938fd4f3b2f/src/error/GraphQLError.js#L52

This means we can't naively do: error.locations = { ... }.

The linter has its own error class ValidationError that extends GraphQLError, perhaps we can have a method in that class that normalizes the location when it is missing.

One last thing I noticed is that when validating the schema syntax, we aren't wrapping the GraphQLError with ValidationError:

https://github.com/cjoudrey/graphql-schema-linter/blob/f49a41a9a1273f66238db2640db3bbebe18161df/src/validator.js#L32-L59

This will no doubt lead to other problems down the line. We might be able to fix this by wrapping the error in the map, i.e. return new ValidationError( ... ) instead of return error.

We should probably also include a test for this edge case in test/runner.js.

Let me know if you need help with any of this. :smile:

cjoudrey avatar Mar 19 '19 15:03 cjoudrey