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

Enforces input coercion rules. #553

Merged
merged 7 commits into from
Nov 3, 2016
Merged

Enforces input coercion rules. #553

merged 7 commits into from
Nov 3, 2016

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Nov 1, 2016

Before this diff, bad input to arguments and variables was often ignored and replaced with null rather than rejected. Now that null has a semantic meaning, and thanks to some recent changes to the spec (graphql/graphql-spec#221) - changes are necessary in order to enforce the stricter coercion rules.

This diff does the following:

  • Implements the CoerceArgumentValues as described in the spec.
  • Implements the CoerceVariablesValues as described in the spec.
  • Alters valueFromAST and coerceValue (dual functions) to strictly enforce coercion, returning undefined implicitly when they fail to do so. It also fixes issues where undefined returns were being ignored as items in a list or fields in an input object.

Before this diff, bad input to arguments and variables was often ignored and replaced with `null` rather than rejected. Now that `null` has a semantic meaning, and thanks to some recent changes to the spec (graphql/graphql-spec#221) - changes are necessary in order to enforce the stricter coercion rules.

This diff does the following:

* Implements the CoerceArgumentValues as described in the spec.
* Implements the CoerceVariablesValues as described in the spec.
* Alters valueFromAST and coerceValue (dual functions) to strictly enforce coercion, returning `undefined` implicitly when they fail to do so. It also fixes issues where undefined returns were being ignored as items in a list or fields in an input object.
@leebyron
Copy link
Contributor Author

leebyron commented Nov 1, 2016

Some of this ends up being follow-up to @langpavel's #544 as edges cases with support for null interrelates to the strict coercion rules.

@langpavel
Copy link
Contributor

Well done!

@thebigredgeek
Copy link

@leebyron it appears that errors thrown from parseValue in scalar resolvers are now completely swallowed, with their messages being appended to the invalid input message. This is a considerable breaking change, as many people are using scalars for form validation (myself included) and this makes it impossible to propagate back custom error text for presentation on the client. Is there any way that the error could retain a reference to the originalError as is the case with most other errors?

@leebyron
Copy link
Contributor Author

Certainly retaining a reference to originalError is intended. I'd support a PR which fixed this issue

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