Input Value Validation

[yaacovCR]

#3086 rebased on main.

Depends on #3812

@leebyron comments from original PR:

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

Potentially breaking if you rely on the existing behavior of coerceInputValue to call a callback function, as the call signature has changed. GraphQL behavior should not change, though error messages are now slightly different.

Note: also breaking if you rely on the default callback function to throw. Grossly similar behavior is available with validateInputValue().

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	7ee722c
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/67121b0ba121e800085537d7
😎 Deploy Preview	https://deploy-preview-3813--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[yaacovCR]

@leebyron @erikkessler1 @benjie et al

I hit a wrinkle rebasing this on main considering the addition of @oneOf.

The spec proposal for @oneOf appropriately adds the following:

- If {value} is a variable:
  - Let {variableName} be the name of {variable}.
  - Let {variableDefinition} be the {VariableDefinition} named
    {variableName} defined within {operation}.
  - Let {variableType} be the expected type of {variableDefinition}.
  - {variableType} must be a non-null type.

This validation logic was implemented in #3513 within the ValuesOfCorrectTypeRule.

This Input Validation PR extracts the logic for ValuesOfCorrectTypeRule into a separate function validateInputLiteral with TS signature:

/**
 * 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 validateInputLiteral.

It might seem that this is very doable, as because of #3811, further up in this PR stack, variables is now of new type VariableValues that preserves the variable source -- and its definition. However, variables is not supplied by the implementation of ValuesForCorrectType, and in fact the presence of absence of variables is what changes the behavior above in terms of the difference between static or runtime validation referenced in the comment above.

Validation for @oneOf seems to be the first case where we can statically check something about the variable definition without actually having a variable value.

We have some options!

1. We can validate the variable definition when used with @oneOf at runtime (Ed: I don't think this is a great option).
2. We can collect and pass the variableDefinitions separately to validateInputLiteral as an additional parameter.
3. We can reorganize the VariableValues interface such that the definitions are stored separately from the actual values. See next comment for a potential option:

All input (pun-intended) welcome!

[yaacovCR]

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;
}

[yaacovCR]

Validation for @oneOf seems to be the first case where we can statically check something about the variable definition without actually having a variable value.

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

[yaacovCR]

This PR relies on #4194 to perform @oneOf variable validation, moving it to VariablesInAllowedPositionRule, a fourth option besides the ones above, and one that has independent appeal for me, as suggested in the later comment above => @oneOf variable validation should be treated the same as variables of other values.

Hopefully, #4194 will be discussed at an upcoming GraphQL WG meeting as it moves closer to Stage 3. If #4194 is not accepted as part of this process, this PR will require greater refiguring.

[benjie]

Hi @yaacovCR; are a lot of the comments above dealt with now via:

• move variable position validation for OneOf to VariablesInAllowedPositionRule #4194
• Clarify 'Values of Correct Type' rule relates to literals graphql-spec#1118
• OneOf validation rule location suggestions benjie/graphql-spec#1

If so, are there remaining complexities?

[yaacovCR]

If so, are there remaining complexities?

I don't think so! Just need some approvals :)