TypeError from passing invalid arguments when there's more than one query present

[dburles]

A query such as:

query {
  requiredArgs(count: x) {
    scalar
    complexScalar
  }
  nonNullItem {
    scalar
    complexScalar
    variableScalar(count: 10)
  }
}

results in complexityMap, being undefined, and the error TypeError: Cannot convert undefined or null to object arising from this call.

Here's a reproduction https://github.com/dburles/graphql-query-complexity/blob/bug/src/__tests__/QueryComplexity-test.ts#L800-L821. Running this test will result in the error being thrown.

GraphQL should be reporting "Argument \"count\" has invalid value x.".

[ivome]

Thanks for the repro. This library does not do any query validation, it expects a valid GraphQL query and variables. Otherwise, we'd have to duplicate the entire validation logic inside of the rule, which would be unnecessary overhead and redundant in a lot of cases. I would suggest to validate the query using the graphql library before passing it to the rule (or use it as the last rule in the chain). Maybe we could add a note to the README, I feel like this has come up before.

[dburles]

Hey @ivome, I've done some digging this morning, and I believe the underlying cause of this problem is that validation occurs in parallel. A query complexity rule cannot be used as if it were the last rule in the chain, as the rules are not ordered.

// Results in `TypeError: Cannot convert undefined or null to object`
validate(schema, document, [
  ...specifiedRules,
  createComplexityRule({ ... })
]);

I've updated the reproduction to show this occurring: https://github.com/dburles/graphql-query-complexity/blob/bd05756ddf52c57ddfa2ca4e65a528b05fa3480c/src/__tests__/QueryComplexity-test.ts#L822-L832

I believe then that you may need to make changes to accomodate complexityMap being undefined.

[dburles]

I came across your reply here mentioning perhaps first validating with default and/or custom rules, before then validating with a query complexity rule, and I was actually going to suggest the same thing and it seems reasonable, however there a couple of downsides:

1. A validation function should work by itself, and not have an implicit dependency on other validation functions (in this case, the GraphQL defaults).
2. It will make adding complexity rules more complicated, since it will require users to define a custom validation function.

So, it seems correct for the library to handle cases like this and account for missing values. As mentioned here, it may be worthwhile writing test cases covering the relevant rules. I'd be happy to help with that.

[ivome]

@dburles, thanks for digging into this. I took a closer look today and found the issue that was causing this. Luckily that was not caused by missing query validation but rather a wrong return value, which means we might not have to add all the validation rules. Here was the culprit:

https://github.com/slicknode/graphql-query-complexity/pull/62/files#diff-720574529a576948113b0f865801527a139a756ce25203d58f98f714bfd59b76R306-R309

I added your reproduction to the test suite to avoid regression. While working on this I also found that the error reporting in getComplexity was not implemented properly and errors are silently swallowed. This is now fixed as well.

If you want to help add more test cases for invalid queries that fail the general GraphQL query validation, that would be awesome to make this more bulletproof. I believe most of the cases are already handled, but there might be a few edge cases that might have slipped through (like the one you found).

[dburles]

Thanks @ivome, this looks great! I'll have a look and see if there may be any additional test cases worth adding, in the meantime I am happy for this fix to be released.

[ivome]

@dburles The initial fix is now published in v0.10.0. Did you have a chance yet to look into other test cases that might be worth adding?

[dburles]

I'll take a look in the coming week.

[dburles]

I've been thinking about adapting some test cases from the core GraphQL validator tests, and in reality I don't think it's scalable. Instead, the rule should be defensive and ensure that it's only acting on values that are present and are the expected type. There's an infinite variety of incorrectly formatted queries.

[ivome]

This is in a way what happens now. With the implemented fix, invalid values should raise a meaningful error message at least. This should be identical to the one that would be raised by the GraphQL library, since it uses the same function internally and just passes the error.

If you find any other invalid queries or edge cases that we need to address, feel free to open another ticket or PR. I'll close this here as the original issue is fixed.