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

RFC: Default value validation & coercion #3814

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jan 2, 2023

#3049 rebased on main.

This is the last rebased PR from the original PR stack concluding with #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

  • 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

@yaacovCR yaacovCR added PR: breaking change 💥 implementation requires increase of "major" version number spec RFC Implementation of a proposed change to the GraphQL specification labels Jan 2, 2023
@yaacovCR yaacovCR requested a review from leebyron January 2, 2023 10:07
@netlify
Copy link

netlify bot commented Jan 2, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit cda413d
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/671e92df8c738a0008525f51
😎 Deploy Preview https://deploy-preview-3814--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link

github-actions bot commented Jan 2, 2023

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
Copy link
Contributor Author

A changelog entry for the first 2 changes in the PR stack might read as follows:

** Implements spec changes around the introduction of the Schema Coordinate nomenclature.

  • Introduces new parser method: parseSchemaCoordinate() which can parse a string/source schema coordinate into an AST .
  • Introduces new methods resolveSchemaCoordinate() and resolveASTSchemeCoordinate() which resolve unparsed and parsed schema coordinates in a particular schema context, returning a ResolvedSchemaElement.
  • Introduces the new discriminated union ResolvedSchemaElement to represent the result of a schema coordinate resolved in the context of a particular schema.
  • Improves error messages across the code base using the coordinate nomenclature.

For the next 8 changes, an overall changelog entry might read as follows:

** Implements spec changes around default value validation.
** Fixes introspection not always returning the original SDL supplied default value.
** Fixes custom scalars not required to replacing variables correctly.

BREAKING CHANGES:

  • A default values is now supplied programmatically as an "External input value" rather than "Internal input value"; the value is now coerced by coerceInputValue() using the appropriate coercion rules. This is a significant BREAKING CHANGE, and will likely require schema authors to check and convert all programmatically supplied default values.

    • To help with this transition, if we can surmise that an default value was supplied as an "Internal input value," a helpful error message will be reported. This error is reported if (a) validation of the default value fails; (b) we can "uncoerce" the default value by treating it like an output value and attempting to serialize it; (c) validation of the "uncoerced" default value succeeds.
    • Introduces a memoized method coerceDefaultValue() that coerces default values according to the appropriate rules.
    • Schema element defaultValue attributes have been replaced with objects of new type GraphQLDefaultValueUsage that include either the original literal value or the uncoerced "External input value."
  • Replaces astFromValue() entirely with the new method valueToLiteral() for use in introspection and schema printing.

    • astFromValue() performed unsafe "uncoercion" from an "Internal input value" to a "GraphQL Literal AST", where valueToLiteral() performs a well defined transform from "External input value" to "GraphQL Literal AST".
  • Replaces valueFromAST() with new method coerceInputLiteral().

    • coerceInputLiteral() now utilizes new replaceVariables() to replaces any variables found within an AST Value literal with their corresponding literals so as to ensure that values generates from ASTs are always constant.
    • The value provided to scalar parseLiteral nodes is now ConstValueNode; the second variables argument has been removed.
  • coerceInputValue() no longer performs error processing, instead simply returning early with value of undefined.

    • New validateInputValue() and validateInputLiteral() methods extracted from coerceInputValue and ValuesOfCorrectTypeRule, respectively, perform unified error reporting.
    • coerceVariableValues() still appropriately reports errors: an undefinedresults from coerceInputValue() will trigger a call to validateInputValue() that will generate the appropriate error.
  • The return value of coerceVariableValues() has changed to an object of new type VariableValues that preserves the original sources (including the default values), a change that enables the behavior of replaceVariables().

@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch 4 times, most recently from 0419c5b to 57e32d7 Compare May 31, 2023 12:20
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from 57e32d7 to 8b135c5 Compare September 17, 2024 11:34
@yaacovCR yaacovCR requested a review from a team as a code owner September 17, 2024 11:34
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch 4 times, most recently from 65ad03b to 082f177 Compare September 17, 2024 13:50
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from 082f177 to 62093da Compare September 17, 2024 14:00
@yaacovCR yaacovCR mentioned this pull request Sep 27, 2024
17 tasks
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from 62093da to 19f6e00 Compare September 29, 2024 12:45
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch 2 times, most recently from ba56cee to c0bc613 Compare October 15, 2024 14:11
yaacovCR added a commit that referenced this pull request Oct 15, 2024
…itionRule` (#4194)

### TLDR => let's consider moving validation of variable positions with
respect to one of into the `VariablesInAllowedPositionRule`, i.e. out of
the `ValuesOfCorrectTypeRule`.

<hr>

This work was pulled out of the work rebasing the Default Value
Coercion/Validation PR stack at #3814 on the OneOf and Experimental
Fragment Variables changes.

In that PR stack (work originally done by @leebyron within #3049), Lee
extracts the validation done by the `ValuesOfCorrectTypeRule` into a
`validateInputLiteral()` utility that can be called at validation-time
or at run-time. At validation time, `validateInputLiteral()` validates
statically, is not supplied variables **or their types**, and does not
validate variables, while at run-time it is supplied variables and does
validate them.

<hr>

With OneOf Input Objects, we have a situation in which Input Objects
will define technically nullable/optional input fields, but the addition
of the `@oneOf` directive tells the validator/executor that we have to
supply exactly one of these fields (and it cannot be given the value
`null`). In essence, we are saying that when `@oneOf` is applied, the
fields of the input object are no longer technically nullable positions,
although they are still optional. This was for a combination of type
reasoning, backwards compatibility, and simplicity, working overall
within the constraints of GraphQL where only nullable fields are
optional.

So, to validate variable usage with OneOf Input Object, we need to make
sure that a variable with a nullable type is not allowed to show up in a
field position on a OneOf Input Object. Where should this be done?
Currently, this is done within the `ValuesOfCorrectTypeRule`, which for
this purpose was modified to collect the operation variable definitions.
@leebyron 's changes in the abovementioned stack extracts this to
`validateInputLiteral()`, where we run into a problem, we don't have
access to the operation (or experimental fragment variables)! Lee's work
preserving variable sources and definitions organizes the definitions by
source, so if we are analyzing statically, we don't have the source or
the definitions.

There are two paths forwards.

One idea is to modify Lee's work, and split the definitions from the
sources, and supply them to `validateInputLiteral()` even when working
statically, which complicates the signature of a number of functions.

What if we take a step back, and ask ourselves if we should have really
modified `ValuesOfCorrectTypeRule` to collect all those operation
variable definitions? If we move this bit to
`VariablesInAllowedPositionRule`, then we avoid the entire complexity.
That's what this PR does.

<hr>

How did this happen anyway? Shouldn't it be clear from the spec change
in which validation rule the changes should have gone? Actually....not
exactly. According to [the proposed spec
changes](graphql/graphql-spec#825), this work
was not done within the `ValuesOfCorrectTypeRule` or the
`VariablesInAllowedPositionRule`, but instead within a wholly new "OneOf
Input Object Rule." That's not the way it got implemented, and in my
opinion for good reason! I'm planning on separately submit some comments
on the RFC to that effect, that we can eliminate the need for the new
rule, and fold the changes into the existing `ValuesOfCorrectTypeRule`
-- which basically says that if it can't coerce, it's not valid, and
because of the coercion changes does not require any actual new text --
except within the `VariablesInAllowedPositionRule`.

Anyway, TLDR TLDR => let's consider moving validation of variable
positions with respect to one of into the
`VariablesInAllowedPositionRule`, i.e. out of the
`ValuesOfCorrectTypeRule`. Discussion of allowed variable positions just
belongs within that rule, just look at the rule name. :)
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch 2 times, most recently from e0eaccc to 76d8ce6 Compare October 15, 2024 19:42
@graphql graphql deleted a comment from Rodipaun Oct 15, 2024
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch 3 times, most recently from d6863e6 to 9c87e56 Compare October 18, 2024 08:30
leebyron and others added 2 commits October 27, 2024 21:21
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Deprecates `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from d1d889a to cda413d Compare October 27, 2024 19:22
@yaacovCR yaacovCR merged commit fbb191a into graphql:main Oct 27, 2024
21 checks passed
@yaacovCR yaacovCR deleted the coerce-default-value-rebased branch October 27, 2024 19:24
yaacovCR added a commit that referenced this pull request Dec 17, 2024
#3814 added default value validation and coercion, deprecating `astFromValue()` (which was unsafe, used `serialize()` to uncoerce input values provided in the internal format) and replacing it with a call to `valueToLiteral()` which safely operates on external values.

This PR makes that change backwards compatible by reintroducing it as a new config option of `default` instead of replacing the existing option of `defaultValue`, where the type of `default` is:

```ts
export type GraphQLDefaultInput =
  | { value: unknown; literal?: never }
  | { literal: ConstValueNode; value?: never };
```

`default.value` is the external default value, while old config option of `defaultValue` is the internal value. Instead of removing it in v17, it is deprecated, and will be removed in v18. 

Side note: `GraphqlDefaultInput` renamed from `GraphQlDefaultValueUsage` introduced within the #3814 PR stack.

Co-authored-by: Michael Hayes <michael@hayes.io>
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 spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants