-
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
Input Value Validation #3813
base: main
Are you sure you want to change the base?
Input Value Validation #3813
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
1d1b883
to
c10741a
Compare
c7f677b
to
2f37daa
Compare
@leebyron @erikkessler1 @benjie et al I hit a wrinkle rebasing this on main considering the addition of The spec proposal for
This validation logic was implemented in #3513 within the This /**
* Validate that the provided input literal is allowed for this type, collecting
* all errors via a callback function.
*
* If variable values are not provided, the literal is validated statically
* (not assuming that those variables are missing runtime values).
*/
export function validateInputLiteral(
valueNode: ValueNode,
type: GraphQLInputType,
variables: Maybe<VariableValues>,
onError: (error: GraphQLError, path: ReadonlyArray<string | number>) => void,
path?: Path,
): void To implement the additional validation for OneOf Input Types mentioned above , we need to pass the operation's variable definitions to It might seem that this is very doable, as because of #3811, further up in this PR stack, Validation for We have some options!
All input (pun-intended) welcome! |
Current: export interface VariableValues {
readonly sources: ReadOnlyObjMap<VariableValueSource>;
readonly coerced: ReadOnlyObjMap<unknown>;
}
interface VariableValueSource {
readonly variable: VariableDefinitionNode;
readonly type: GraphQLInputType;
readonly value: unknown;
} Proposal: export interface VariableValues {
readonly definitions: ReadOnlyObjMap<VariableDefinitionNode>;
readonly values?: ReadOnlyObjMap<VariableValueValues> | undefined;
}
export interface VariableValueValues {
readonly sources: ReadOnlyObjMap<VariableValueSource>;
readonly coerced: ReadOnlyObjMap<unknown>;
}
interface VariableValueSource {
readonly type: GraphQLInputType;
readonly value: unknown;
} |
Correction: this is definitely not the case, there is a wholly separate validation rule specifying that variables should be of the correct type. My current suggestion actually is to pull the variable validation from one-of altogether, and let whether we allow nullable variables in non-nullable positions re: oneOf be decided by the general rule. See: https://github.com/graphql/graphql-spec/pull/825/files#r1538875839 |
2f37daa
to
da558c0
Compare
b9d35d4
to
2300b66
Compare
Fixes graphql#3051 This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions `defaultValue` field from just the "value" to a new `GraphQLDefaultValueUsage` type which contains either -- but not both -- "value" and "literal" fields. Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL: **Before this change:** ``` (SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL) ``` `coerceInputLiteral` performs coercion which is a one-way function, and `valueToAST` is unsafe and set to be deprecated in graphql#3049. **After this change:** ``` (SDL) --parse-> (defaultValue literal config) --print --> (SDL) ```
By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.
* Adds `valueToLiteral()` which takes an external value and translates it to a literal, allowing for custom scalars to define this behavior. This also adds important changes to Input Coercion, especially for custom scalars: * Addition of `parseConstLiteral()` to leaf types which operates in parallel to `parseLiteral()` but take `ConstValueNode` instead of `ValueNode` -- the second `variables` argument has been removed. For all built-in scalars this has no effect, but any custom scalars which use complex literals no longer need to do variable reconciliation manually (in fact most do not -- this has been an easy subtle bug to miss). This behavior is possible with the addition of `replaceVariables()`. `parseLiteral()` is no longer used internally and has been marked for deprecation.
Factors out input validation to reusable functions: * Introduces `validateInputLiteral` by extracting this behavior from `ValuesOfCorrectTypeRule`. * Introduces `validateInputValue` by extracting this behavior from `coerceInputValue` * Simplifies `coerceInputValue` to return early on validation error * Unifies error reporting between `validateInputValue` and `validateInputLiteral`, causing some error message strings to change, but error data (eg locations) are preserved. These two parallel functions will be used to validate default values in graphql#3049 Potentially breaking if you rely on the existing behavior of `coerceInputValue` to call a callback function, as the call signature has changed, or to throw with the default callback function. Grossly similar behavior is available with `validateInputValue()`, but with a separate function. GraphQL behavior should not change, though error messages are now slightly different.
2300b66
to
e4860de
Compare
#3086 rebased on main.
Depends on #3812
@leebyron comments from original PR:
Note: also breaking if you rely on the default callback function to throw. Grossly similar behavior is available with
validateInputValue()
.