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.

Sign up for GitHub

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

Jump to bottom

RFC: Default value validation & coercion #3814

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

RFC: Default value validation & coercion #3814

wants to merge 8 commits into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jan 2, 2023 •
edited

#3049 rebased on main.

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

@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 •
edited

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

Name Link
🔨 Latest commit 57e32d7
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/64773bad17553e00087548f1
😎 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 settings.

@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 yaacovCR force-pushed the coerce-default-value-rebased branch from 8ea5217 to 2331ba5 Compare January 2, 2023 11:22
This was referenced Jan 31, 2023
Closed
Merged
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch 6 times, most recently from ad69cea to 02df9ab Compare February 6, 2023 12:54
@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().

@leebyron @yaacovCR
Schema Coordinates
75b9bd1 
Implements graphql/graphql-spec#794

Adds:

* DOT punctuator in lexer
* Improvements to lexer errors around misuse of `.`
* Minor improvement to parser core which simplified this addition
* `SchemaCoordinate` node and `isSchemaCoodinate()` predicate
* Support in `print()` and `visit()`
* Added function `parseSchemaCoordinate()` since it is a parser entry point.
* Added function `resolveSchemaCoordinate()` and `resolveASTSchemaCoordinate()` which implement the semantics (name mirrored from `buildASTSchema`) as well as the return type `ResolvedSchemaElement`
@leebyron @yaacovCR
Add coordinate field to schema element definitions
3df173b 
* Defines a `GraphQLSchemaElement` base class which defines a `.coordinate` property and `toString`/`toJSON` methods.
* Adds base class to types, fields, arguments, input fields, enum values, and directives.
* Uses this in validation error printing string templates.
@leebyron @yaacovCR
Add coerceInputLiteral()
bf89146 
Removes `valueFromAST()` and adds `coerceInputLiteral()` as an additional export from `coerceInputValue`.

The implementation is almost exactly the same as `valueFromAST()` with a slightly more strict type signature and refactored tests to improve coverage (the file unit test has 100% coverage)

While this does not change any behavior, it could be breaking if you rely directly on the valueFromAST() method. Use `coerceInputLiteral()` as a direct replacement.
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from 02df9ab to 917e9e3 Compare May 31, 2023 11:33
@leebyron @yaacovCR
Preserve defaultValue literals
0376313 
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 or both "value" and "literal" fields.

Since either of these fields may be set, new functions for resolving a value or literal from either have been added - `getLiteralDefaultValue` and `getCoercedDefaultValue` - these replace uses that either assumed a set value or were manually converting a value back to a literal.

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)
```
@leebyron @yaacovCR
Preserve sources of variable values
2211b7a 
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`.
@leebyron @yaacovCR
Add valueToLiteral()
3ea9f70 
* 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:

* The value provided to `parseLiteral` is now `ConstValueNode` and 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 `replaceASTVariables`
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from 917e9e3 to 49da195 Compare May 31, 2023 11:52
@leebyron @yaacovCR
Input Value Validation
2f37daa 
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. GraphQL behavior should not change, though error messages are now slightly different.
@yaacovCR yaacovCR force-pushed the coerce-default-value-rebased branch from 49da195 to 0419c5b Compare May 31, 2023 12:16
@leebyron @yaacovCR
RFC: Default value validation & coercion
57e32d7 
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:
* Remove `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 0419c5b to 57e32d7 Compare May 31, 2023 12:20
@benjie benjie mentioned this pull request Jul 8, 2023
Closed
@Rodipaun
Copy link

Help androad

@yaacovCR yaacovCR mentioned this pull request May 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leebyron leebyron Awaiting requested review from leebyron

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yaacovCR @Rodipaun @leebyron