-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Schema Coordinates #3044
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
Schema Coordinates #3044
Conversation
c7b87e2 to
0a5630a
Compare
magicmark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!
This is way beyond what I was hacking with. Hadn't even considered reusing the existing parser. (I was fiddling around with regexes...)
Thanks for putting this together - this makes a lot of sense.
a4a8920 to
42f9072
Compare
|
Great feedback - I think I've resolved both:
|
42f9072 to
1399f7c
Compare
3451e3e to
2f893d6
Compare
1399f7c to
3cec8c2
Compare
3cec8c2 to
e688f88
Compare
e688f88 to
c68ca9b
Compare
c68ca9b to
8fa8a4c
Compare
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`
8fa8a4c to
98e7541
Compare
benjie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is essentially good to go except for the comments noted, and that I think we should explicitly forbid {Ignored} from schema coordinates.
benjie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work everyone, especially @magicmark!
[graphql#3049 rebased on main](graphql#3049). This is the last rebased PR from the original PR stack concluding with graphql#3049. * Rebased: graphql#3809 [Original: graphql#3092] * Rebased: graphql#3810 [Original: graphql#3074] * Rebased: graphql#3811 [Original: graphql#3077] * Rebased: graphql#3812 [Original: graphql#3065] * Rebased: graphql#3813 [Original: graphql#3086] * Rebased: graphql#3814 (this PR) [Original: graphql#3049] Update: graphql#3044 and graphql#3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from graphql#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: > >  --------- Co-authored-by: Lee Byron <[email protected]>
…QLEnumValue (graphql#4288) this extracts logic from graphql#3044 and graphql#3145 (later rebased as graphql#3807 and [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
[graphql#3049 rebased on main](graphql#3049). This is the last rebased PR from the original PR stack concluding with graphql#3049. * Rebased: graphql#3809 [Original: graphql#3092] * Rebased: graphql#3810 [Original: graphql#3074] * Rebased: graphql#3811 [Original: graphql#3077] * Rebased: graphql#3812 [Original: graphql#3065] * Rebased: graphql#3813 [Original: graphql#3086] * Rebased: graphql#3814 (this PR) [Original: graphql#3049] Update: graphql#3044 and graphql#3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from graphql#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: > >  --------- Co-authored-by: Lee Byron <[email protected]>
…QLEnumValue (graphql#4288) this extracts logic from graphql#3044 and graphql#3145 (later rebased as graphql#3807 and [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
[graphql#3049 rebased on main](graphql#3049). This is the last rebased PR from the original PR stack concluding with graphql#3049. * Rebased: graphql#3809 [Original: graphql#3092] * Rebased: graphql#3810 [Original: graphql#3074] * Rebased: graphql#3811 [Original: graphql#3077] * Rebased: graphql#3812 [Original: graphql#3065] * Rebased: graphql#3813 [Original: graphql#3086] * Rebased: graphql#3814 (this PR) [Original: graphql#3049] Update: graphql#3044 and graphql#3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from graphql#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: > >  --------- Co-authored-by: Lee Byron <[email protected]>
…QLEnumValue (graphql#4288) this extracts logic from graphql#3044 and graphql#3145 (later rebased as graphql#3807 and [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
[graphql#3049 rebased on main](graphql#3049). This is the last rebased PR from the original PR stack concluding with graphql#3049. * Rebased: graphql#3809 [Original: graphql#3092] * Rebased: graphql#3810 [Original: graphql#3074] * Rebased: graphql#3811 [Original: graphql#3077] * Rebased: graphql#3812 [Original: graphql#3065] * Rebased: graphql#3813 [Original: graphql#3086] * Rebased: graphql#3814 (this PR) [Original: graphql#3049] Update: graphql#3044 and graphql#3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from graphql#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: > >  --------- Co-authored-by: Lee Byron <[email protected]>
…QLEnumValue (graphql#4288) this extracts logic from graphql#3044 and graphql#3145 (later rebased as graphql#3807 and [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
The original schema coordinates PR on v17 (graphql#3044), since backported to 16.x.x, had non-specified support for schema coordinates for metafields. Co-authored-by: Benjie Gillam <[email protected]> Co-authored-by: Mark Larah <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
[graphql#3049 rebased on main](graphql#3049). This is the last rebased PR from the original PR stack concluding with graphql#3049. * Rebased: graphql#3809 [Original: graphql#3092] * Rebased: graphql#3810 [Original: graphql#3074] * Rebased: graphql#3811 [Original: graphql#3077] * Rebased: graphql#3812 [Original: graphql#3065] * Rebased: graphql#3813 [Original: graphql#3086] * Rebased: graphql#3814 (this PR) [Original: graphql#3049] Update: graphql#3044 and graphql#3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from graphql#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: > >  --------- Co-authored-by: Lee Byron <[email protected]>
…QLEnumValue (graphql#4288) this extracts logic from graphql#3044 and graphql#3145 (later rebased as graphql#3807 and [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
The original schema coordinates PR on v17 (graphql#3044), since backported to 16.x.x, had non-specified support for schema coordinates for metafields. Co-authored-by: Benjie Gillam <[email protected]> Co-authored-by: Mark Larah <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
[graphql#3049 rebased on main](graphql#3049). This is the last rebased PR from the original PR stack concluding with graphql#3049. * Rebased: graphql#3809 [Original: graphql#3092] * Rebased: graphql#3810 [Original: graphql#3074] * Rebased: graphql#3811 [Original: graphql#3077] * Rebased: graphql#3812 [Original: graphql#3065] * Rebased: graphql#3813 [Original: graphql#3086] * Rebased: graphql#3814 (this PR) [Original: graphql#3049] Update: graphql#3044 and graphql#3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from graphql#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: > >  --------- Co-authored-by: Lee Byron <[email protected]>
…QLEnumValue (graphql#4288) this extracts logic from graphql#3044 and graphql#3145 (later rebased as graphql#3807 and [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
The original schema coordinates PR on v17 (graphql#3044), since backported to 16.x.x, had non-specified support for schema coordinates for metafields. Co-authored-by: Benjie Gillam <[email protected]> Co-authored-by: Mark Larah <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
The original schema coordinates PR on v17 (graphql#3044), since backported to 16.x.x, had non-specified support for schema coordinates for metafields. Co-authored-by: Benjie Gillam <[email protected]> Co-authored-by: Mark Larah <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
Depends on #3115
Implements graphql/graphql-spec#794
Adds:
.SchemaCoordinatenode andisSchemaCoodinate()predicateprint()andvisit()parseSchemaCoordinate()since it is a parser entry point.resolveSchemaCoordinate()andresolveASTSchemeCoordinate()which implement the semantics (name mirrored frombuildASTSchema) as well as the return typeGraphQLSchemaElement