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

Input Value Validation #3086

Closed
wants to merge 1 commit into from
Closed

Input Value Validation #3086

wants to merge 1 commit into from

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented May 13, 2021

Depends on #3065

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 #3049

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.

@leebyron leebyron force-pushed the value-to-literal branch from d4a59f5 to d3aaf3e Compare May 13, 2021 23:05
@leebyron leebyron force-pushed the input-validation branch 3 times, most recently from 77ffb26 to 026cdd2 Compare May 14, 2021 20:34
@leebyron leebyron force-pushed the value-to-literal branch from d3aaf3e to b56f8dd Compare May 14, 2021 23:28
@leebyron leebyron force-pushed the input-validation branch from 026cdd2 to 855af69 Compare May 14, 2021 23:29
@leebyron leebyron force-pushed the value-to-literal branch from b56f8dd to f958baa Compare May 15, 2021 05:41
@leebyron leebyron force-pushed the input-validation branch 2 times, most recently from 0ac6329 to 6bba95b Compare May 15, 2021 06:11
@leebyron leebyron force-pushed the value-to-literal branch from f958baa to 1a7b39a Compare May 15, 2021 06:35
@leebyron leebyron force-pushed the input-validation branch from 6bba95b to 7f31940 Compare May 15, 2021 06:36
@leebyron leebyron force-pushed the value-to-literal branch 2 times, most recently from 3f23ab5 to 01652d0 Compare May 15, 2021 06:55
@leebyron leebyron force-pushed the input-validation branch from 7f31940 to f7f624e Compare May 15, 2021 06:56
@andimarek
Copy link
Contributor

@leebyron question: are you going to validate default argument values of Directive definitions and arguments of applied Directives too?

For example:

# custom Scalar
scalar CacheKey

directive @cached(
  key: CacheKey = {key: "DEFAULT", duration=1000}
) on FIELD_DEFINITION 

type Query {
  hello: String @cached(key: {key: "hello", duration = 500})
}

@leebyron leebyron force-pushed the value-to-literal branch from 01652d0 to 56f78d8 Compare May 15, 2021 21:34
@leebyron leebyron force-pushed the input-validation branch 2 times, most recently from a30c9ca to 88fa4f9 Compare May 15, 2021 21:39
@leebyron
Copy link
Contributor Author

Yes both. Though one additional follow up might be necessary.

Arguments of applied directives in operations have always been validated in the ValuesOfCorrectType validator. In schema they never translate into something so they haven't been validated. This seems like an easy thing to fix.

Directive definition argument default value validation is in the RFC spec change and included in the last PR in this stack

@leebyron leebyron force-pushed the value-to-literal branch 3 times, most recently from 13d9314 to 30deb95 Compare June 1, 2021 22:11
@leebyron leebyron force-pushed the input-validation branch 2 times, most recently from 757931d to d1fbfb3 Compare June 1, 2021 22:34
@leebyron leebyron added the PR: breaking change 💥 implementation requires increase of "major" version number label Jun 1, 2021
@leebyron leebyron force-pushed the value-to-literal branch from 30deb95 to d5fb50a Compare June 3, 2021 17:10
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 #3049

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.
@leebyron leebyron force-pushed the input-validation branch from d1fbfb3 to 9a9d330 Compare June 3, 2021 17:11
yaacovCR added a commit that referenced this pull request Oct 18, 2024
[#3086 rebased on
main](#3086).

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()`.

Co-authored-by: Lee Byron <[email protected]>
@yaacovCR
Copy link
Contributor

Merged rebased version of this PR as #3813.

@yaacovCR yaacovCR closed this Oct 18, 2024
yaacovCR added a commit that referenced this pull request Oct 27, 2024
[#3049 rebased on
main](#3049).

This is the last rebased PR from the original PR stack concluding with
#3049.

* Rebased: #3809 [Original: #3092]
* Rebased: #3810 [Original: #3074]
* Rebased: #3811 [Original: #3077]
* Rebased: #3812 [Original: #3065]
* Rebased: #3813 [Original: #3086]
* Rebased: #3814 (this PR) [Original: #3049]

Update: #3044 and #3145 have been separated from this stack.

Changes from original PR:
1. `astFromValue()` is deprecated instead of being removed.

@leebyron comments from #3049, the original PR:
> Implements
[graphql/graphql-spec#793](graphql/graphql-spec#793)
> 
> * BREAKING: Changes default values from being represented as an
assumed-coerced "internal input value" to a pre-coerced "external input
value" (See chart below).
> This allows programmatically provided default values to be represented
in the same domain as values sent to the service via variable values,
and allows it to have well defined methods for both transforming into a
printed GraphQL literal string for introspection / schema printing (via
`valueToLiteral()`) or coercing into an "internal input value" for use
at runtime (via `coerceInputValue()`)
> To support this change in value type, this PR adds two related
behavioral changes:
>   
> * Adds coercion of default values from external to internal at runtime
(within `coerceInputValue()`)
> * Removes `astFromValue()`, replacing it with `valueToLiteral()` for
use in introspection and schema printing. `astFromValue()` performed
unsafe "uncoercion" to convert an "Internal input value" directly to a
"GraphQL Literal AST", where `valueToLiteral()` performs a well defined
transform from "External input value" to "GraphQL Literal AST".
> * Adds validation of default values during schema validation.
> Since assumed-coerced "internal" values may not pass "external"
validation (for example, Enum values), an additional validation error
has been included which attempts to detect this case and report a
strategy for resolution.
> 
> Here's a broad overview of the intended end state:
> 
> ![GraphQL Value
Flow](https://user-images.githubusercontent.com/50130/118379946-51ac5300-b593-11eb-839f-c483ecfbc875.png)

---------

Co-authored-by: Lee Byron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants