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

Don't limit amount of reported errors #309

Merged
merged 1 commit into from
May 21, 2022

Conversation

vojtech-dobes
Copy link
Contributor

@vojtech-dobes vojtech-dobes commented May 16, 2022

Fixes #308

When our schema produced too many errors, graphql scream with following error:

Too many validation errors, error limit reached. Validation aborted.

Subsequent errors sorting trips over missing location in this error :).

maxErrors is documented specifically to not explode on crazy malicious queries, but considering purpose of this tool, raising maxErrors effectively to infinity should be valid.

@cjoudrey
Copy link
Owner

Thanks for the contribution @vojtech-dobes! Great find! This should help close #308.

I see some tests are failing. We might want to address those.

@vojtech-dobes vojtech-dobes force-pushed the fix-too-many-errors branch from 974d320 to 24fad28 Compare May 19, 2022 20:28
@vojtech-dobes vojtech-dobes force-pushed the fix-too-many-errors branch from 24fad28 to 744e54d Compare May 19, 2022 20:50
@vojtech-dobes
Copy link
Contributor Author

@cjoudrey I think I found it, there is difference in validate arguments in graphql@15.x and graphql@16.x. Let me know if solving it by checking import { version } from 'graphql' is okay.

@cjoudrey
Copy link
Owner

Good catch!

Yeah, I am fine with changing logic based on version. I'd make the default behaviour the v16 method signature. That way things continue to work for v17 and so on.

@vojtech-dobes
Copy link
Contributor Author

vojtech-dobes commented May 20, 2022

@cjoudrey right - does it mean you want it further tweaked :)? I am not sure.

@cjoudrey
Copy link
Owner

I think what you did is fine. Thanks again for the contribution. I'll get this merged and released shortly. 😄

@cjoudrey cjoudrey merged commit 9052872 into cjoudrey:master May 21, 2022
@vojtech-dobes
Copy link
Contributor Author

@cjoudrey Thanks for accepting :)!

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.

TypeError: Cannot read property '0' of undefined
2 participants