-
Notifications
You must be signed in to change notification settings - Fork 41
Add fallback for missing child complexity in simple estimator #58
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
Conversation
a39f6c0 to
7473ad3
Compare
Hi @kjarmicki, thanks for the PR! I am not sure if it's a good idea to suppress that error. This library assumes that a valid GraphQL query is passed with valid variable values. There are a lot of other cases where an error in the GraphQL query / variable values leads to unintended behavior. I don't think we should hide and suppress those errors. As a user of the simpleEstimator I would never expect that an invalid scalar in some deeply nested query essentially disables the query complexity calculation for that part of the query tree instead of raising an error.
If you are doing a standalone calculation of the query complexity, it's probably best to validate the query first using the validation methods from graphql-js.
Oh, ok. Sounds reasonable, thank you for the explanation 🙂
Hello,
In the docs it states that the simple estimator "can be used as the last estimator in the chain for a default value". Unfortunately, that's not always the case - if child node complexity computation fails, then
args.childComplexitywill beNaN, resulting inNaNvalue for the entire complexity estimation and an error being passed to the context here:graphql-query-complexity/src/QueryComplexity.ts
Lines 335 to 342 in a940f94
This PR fixes that by assuming
NaNvalue of child complexity calculation to be equal to 0 and falling back to the default complexity value.