Skip to content
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

Conversation

SoyYoRafa
Copy link

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.

@SoyYoRafa SoyYoRafa changed the title Adds ability to limit errors in getVariableValues() and coerceValue(). Limits errors in getVariableValues() and coerceValue(). Jul 14, 2019
…es() and coerceValue(). Errors are statically capped at 50
@SoyYoRafa SoyYoRafa force-pushed the bugfix/SoyYoRafa/getvariablevalues-max-errors branch from 597e4f5 to 62f5356 Compare July 14, 2019 16:30
@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jul 16, 2019

@SoyYoRafa Thanks for PR 👍
BTW, you don't need fieldWithOneHundredInputs to create multiple query variables you can use fields aliases to do the same even for the fields with a single argument:

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:

[
  [... 50 bad values],
  [... another 50 bad values]
]

I will try to finish it this week.

@SoyYoRafa
Copy link
Author

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.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jul 17, 2019

Could we please release the final merge result as part of version 14.x?

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 15.x.x just for this fix.

@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 execute and there are no plans to limit errors from validate.

@IvanGoncharov
Copy link
Member

@SoyYoRafa Just as an update I'm still working on it.
Taking more time than I initially estimated since ATM I'm focusing on code & tests clean up.
But I'm pretty confident I will have my PR ready for review in a couple of days.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jul 29, 2019
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jul 31, 2019
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 6, 2019
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 6, 2019
@mjmahone
Copy link
Contributor

mjmahone commented Aug 6, 2019

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.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 7, 2019
IvanGoncharov added a commit that referenced this pull request Aug 7, 2019
@IvanGoncharov
Copy link
Member

@SoyYoRafa Fixed in #2062. Thank you for this prototype I borrow a lot from it and this significantly speed up work on #2037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants