Exception if the GraphQL query has validation errors

[jaydenseric]

If the GraphQL query has validation errors such as variables not being used, or referencing unknown fragments, normally a GraphQL server can respond with a 400 status and a JSON response containing the GraphQL errors.

Unfortunately the GraphQL query complexity validator errors if the GraphQL query has validation issues, causing a 500 response that doesn't contain the detailed GraphQL errors. Ideally the complexity analysis would account for the possibility the query is invalid, and not cause an exception so that the normal GraphQL query validation errors can be reported.

Example correct 400 response for an invalid query:

{
  "errors": [
    {
      "message": "Unknown fragment \"galleryExhibitConnection\".",
      "locations": [{ "line": 1, "column": 1532 }]
    },
    {
      "message": "Variable \"$devicePixelRatio\" is never used.",
      "locations": [{ "line": 1, "column": 623 }]
    },
    {
      "message": "Variable \"$cardPaginationLimit\" is never used.",
      "locations": [{ "line": 1, "column": 648 }]
    },
    {
      "message": "Variable \"$cardExhibitImageWidth\" is never used.",
      "locations": [{ "line": 1, "column": 757 }]
    },
    {
      "message": "Variable \"$cardExhibitImageHeight\" is never used.",
      "locations": [{ "line": 1, "column": 785 }]
    },
    {
      "message": "Variable \"$galleryExhibitsPageCursor\" is never used.",
      "locations": [{ "line": 1, "column": 894 }]
    },
    {
      "message": "Fragment \"GalleryExhibits_GalleryExhibitConnection\" is never used.",
      "locations": [{ "line": 1, "column": 1 }]
    }
  ]
}

With query complexity setup, the actual response has a 500 status without the GraphQL errors:

{
  "errors": [{ "message": "Internal Server Error" }]
}

Here is the error logged in the server:

TypeError: Cannot read property 'typeCondition' of undefined
    at /[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:155:118
    at Array.reduce (<anonymous>)
    at QueryComplexity.nodeComplexity (/[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:88:75)
    at /[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:129:52
    at Array.reduce (<anonymous>)
    at QueryComplexity.nodeComplexity (/[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:88:75)
    at /[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:172:53
    at Array.reduce (<anonymous>)
    at QueryComplexity.nodeComplexity (/[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:88:75)
    at /[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:129:52
    at Array.reduce (<anonymous>)
    at QueryComplexity.nodeComplexity (/[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:88:75)
    at QueryComplexity.onOperationDefinitionEnter (/[redacted]/node_modules/graphql-query-complexity/dist/QueryComplexity.js:50:41)
    at Object.enter (/[redacted]/node_modules/graphql/language/visitor.js:323:29)
    at Object.enter (/[redacted]/node_modules/graphql/utilities/TypeInfo.js:370:25)
    at visit (/[redacted]/node_modules/graphql/language/visitor.js:243:26)

I'm using graphql-api-koa, but I don't think that's relevant as it's GraphQL.js based.

[ivome]

Thanks for bringing this up! The specific error you mentioned was caused by an unknown fragment. The query complexity validation rule tries to determine the complexity of a non-existent fragment, which fails and causes the error.
I now added handling that unknown fragments are just ignored. They should be caught by other validation rules.
I similarly added handling for fragments on unknown types. They will also be ignored.

Fix in PR #36

Are there any other cases we need to cover?

[jaydenseric]

Nice work! Looking forward to trying the new version out 🙌

Regarding cases to cover; theoretically the entire list of specified rules:

https://github.com/graphql/graphql-js/blob/cc146bc7d4c7d46950d4b43dcc051bc9eb058bdb/src/validation/specifiedRules.js#L94-L127

But, I'm not as familiar as you with what specific rules might be relevant to the work the query complexity analysis does. Maybe only the ones covered in your PR are necessary to test.

[ivome]

I merged the PR and published v0.7.2. The most robust solution would probably be to create a custom validation function that first runs the validation on all the default validation rules and only run the query complexity validation rule when we have a valid GraphQL query. That way we catch all current and future cases, but this is better done outside of this library. Plus we don't have to replicate the validation rules in the complexity analysis.

[jaydenseric]

Thank you, love your work 🙌