-
Notifications
You must be signed in to change notification settings - Fork 237
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
feat: support passing options to graphql validate and parse #1050
Conversation
@mcollina Can we get this on a reviewers queue? I wouldn't usually poke maintainers like this, but previous PRs seem to be getting reviewed within a few days. I wanted to make sure this didn't fall through the cracks. |
@simoneb Any chance this is something you can review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
docs/api/options.md
Outdated
- `parseOptions`: Object. [GraphQL's parse function options](https://github.com/graphql/graphql-js/blob/main/src/language/parser.ts#L77-L133) | ||
- `validateOptions`: Object. [GraphQL's validate function options](https://github.com/graphql/graphql-js/blob/main/src/validation/validate.ts#L44) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid deep links to source code, as it can change any time. The alternative would be to use permalinks, but I'm not sure that's a good option either. Is there official documentation that we can point to instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The published graphql docs do not contain option properties sadly. I could update them to a specific tag if that works for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not many compelling options then. I'm not sure. if we point at master, those links may refer to wrong lines when those files change. if we use permalinks, the reference will keep existing but the real code might have changed. not sure what's best really. shall we link to the files without any line number perhaps? that would give us some confidence that those files will keep existing and leave it to the reader to figure out where to find the types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just enumerate the literal properties, rather than linking off. It does mean our future selfs will need to go back to the docs and confirm when things have been added/removed.
Not sure there is a good option, other than pushing for these functions to be documented more fully in GraphQL library.
Happy to change to without line numbers if that makes anyone feel more comfortable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No line numbers is the compromise I'd go with, but happy for somebody else to chime in too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this!
Support passing options to GraphQL's
validate
andparse
functions.Reasoning to allow configuration of these functions topics is fine tuning to prevent DDOS attacks.
This allows developers to set:
validate
's options ofmaxErrors
to something more/less than the 100 default. This prevents an attacker from specifying 100 invalid fields, requiring CPU time generating validation errors.parse
's options ofmaxTokens
to a value. It has no default. This prevent an attacker from producing a overly high complexity query that requires a high amount of CPU time.