-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Limits errors in getVariableValues() and coerceValue(). #2037
Limits errors in getVariableValues() and coerceValue(). #2037
Conversation
…es() and coerceValue(). Errors are statically capped at 50
597e4f5
to
62f5356
Compare
@SoyYoRafa Thanks for PR 👍 query($a:String!,$b:String!,$c:String!) {
a: __type(name: $a) { __typename }
b: __type(name: $b) { __typename }
c: __type(name: $c) { __typename }
} But before merging it I want to try to implemente same functionality with less code duplication and also try to fix some edge cases. For example in current implementetion this array will produce 100 of errors that will latter be dropped to 50:
I will try to finish it this week. |
Ah, good to know! Could we please release the final merge result as part of version 14.x? We are keen on getting this issue fixed. |
It can be a breaking change if someone using graphql errors for client-side validation but it's something that we always discouraged, so I think it safe. Also, I think we need to publish CVE (low priority but still) after publishing this fix and I think it's bad to force users to update to @mjmahone @martijnwalraven @abernix Can it a be breaking change if we will start limiting maximum number errors produced by validating/coercing query variables? Note: We discuss only validation performed during |
@SoyYoRafa Just as an update I'm still working on it. |
I don't have a strict problem with this, and it's unclear to me what a reference server implementation should do. I think the server ending up in an error state if there are too many GraphQL errors is acceptable, though. |
@SoyYoRafa Fixed in #2062. Thank you for this prototype I borrow a lot from it and this significantly speed up work on #2037 |
Fix for issue #1753. Errors are statically capped at 50. Unit tests added.
There is a bit of code duplication in checking whether the error limit is capped. The reason for this is that we want the cost of checking whether the limit is reached only when errors are being created and not during the execution and coercion of good values.