Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Open
wants to merge 9 commits into
base: next
Choose a base branch
Loading
from
Open

Schema Coordinates #3044

wants to merge 9 commits into from

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Apr 16, 2021

Depends on #3115

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 resolveASTSchemeCoordinate() which implement the semantics (name mirrored from buildASTSchema) as well as the return type GraphQLSchemaElement

@leebyron leebyron requested a review from IvanGoncharov April 16, 2021 09:33
@leebyron leebyron force-pushed the schema-coordinates branch 2 times, most recently from c7b87e2 to 0a5630a Compare April 16, 2021 09:46
@IvanGoncharov IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Apr 16, 2021
src/language/lexer.js Outdated Show resolved Hide resolved
Copy link

@magicmark magicmark left a 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.

src/language/printer.js Outdated Show resolved Hide resolved
src/language/ast.js Outdated Show resolved Hide resolved
src/utilities/resolveSchemaCoordinate.js Outdated Show resolved Hide resolved
@leebyron leebyron force-pushed the schema-coordinates branch from a4a8920 to 42f9072 Compare April 22, 2021 07:27
@leebyron
Copy link
Contributor Author

leebyron commented Apr 22, 2021

Great feedback - I think I've resolved both:

  • Replaced fieldName with memberName in the AST to ensure it's generalized between fields, input fields, and enum values. Those are all specific instances of the general pattern of "members of a set", which is pretty common terminology for this generalized dot access pattern.
  • Took both of your great suggestions for wrapping the return type of resolveSchemaCoordinate - the ResolvedSchemaElement type is now a union of wrappers. This had the added benefits of disambiguating between "field argument" and "directive argument" and including the coordinate context in the return value

@leebyron leebyron requested a review from IvanGoncharov April 22, 2021 16:58
@leebyron leebyron force-pushed the schema-coordinates branch from 42f9072 to 1399f7c Compare May 27, 2021 23:55
@leebyron leebyron changed the base branch from main to lexer-cleanup May 27, 2021 23:55
@leebyron leebyron added the PR: feature 🚀 requires increase of "minor" version number label May 27, 2021
@leebyron leebyron force-pushed the lexer-cleanup branch 2 times, most recently from 3451e3e to 2f893d6 Compare May 28, 2021 22:21
@leebyron leebyron force-pushed the schema-coordinates branch from 1399f7c to 3cec8c2 Compare May 28, 2021 22:31
@leebyron leebyron force-pushed the schema-coordinates branch from 3cec8c2 to e688f88 Compare May 30, 2021 06:30
src/language/ast.ts Outdated Show resolved Hide resolved
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`
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 <lee@leebyron.com>
yaacovCR added a commit that referenced this pull request Dec 1, 2024
…QLEnumValue (#4288)

this extracts logic from #3044 and #3145 (later rebased as #3807 and
#3808) to implement more informative error messages without implementing
[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 <decroockjovi@gmail.com>
@benjie benjie requested a review from a team as a code owner May 22, 2025 09:47
@benjie
Copy link
Member

benjie commented May 22, 2025

I've merged the latest main into this branch and got it passing tests/etc again; am adding it to the next GraphQL WG.

// Schema coordinates
export {
resolveSchemaCoordinate,
resolveASTSchemaCoordinate,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make a note about documenting this when we start on the v17 documentation

src/language/__tests__/lexer-test.ts Show resolved Hide resolved
src/language/ast.ts Outdated Show resolved Hide resolved
Comment on lines +133 to +134
resolveSchemaCoordinate,
resolveASTSchemaCoordinate,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those functions are defined in the spec (see graphql/graphql-spec#794 (comment)), should we put them in language instead of utlilities?

Copy link
Contributor

@yaacovCR yaacovCR Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, I vote for utilities

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it could fit better into the type section or its own 'coordinate' section.

src/utilities/resolveSchemaCoordinate.ts Outdated Show resolved Hide resolved
src/utilities/resolveSchemaCoordinate.ts Outdated Show resolved Hide resolved
@magicmark
Copy link

magicmark commented Jun 2, 2025

@benjie I don't have write access to this PR since I wasn't the original author -- do you think we could merge in #4422 and then magicmark#1 to collapse the stack for easier review?

(Alternatively, i'm happy to make a yet-another new PR to collapse everything, was trying to avoid making things even more confusing though.)

@martinbonnin
Copy link

do you think we could merge in #4422 and then magicmark#1 to collapse the stack for easier review?

+1 on having a single PR for all changes.

@benjie
Copy link
Member

benjie commented Jun 5, 2025

@magicmark let's merge magicmark#1 into #4422 (you have to do that since you own that repository), then incorporate changes to support magicmark/graphql-spec#1, then I can merge the whole lot into this PR assuming there's no pushback.

Co-authored-by: Benjie <benjie@jemjie.com>
src/language/__tests__/printer-test.ts Show resolved Hide resolved
src/language/tokenKind.ts Outdated Show resolved Hide resolved
type: schema.getType('String'),
});

expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(undefined);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be null?

Suggested change
expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(undefined);
expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(null);

Copy link

@magicmark magicmark Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per @benjie's comment here magicmark#1 (comment) I believe undefined is correct, despite the spec saying "null". I think a note somewhere in the spec text saying something like this:

"Algorithms for functions in this spec may return "null" (in a non-error state). In the reference graphql-js implementation, this corresponds to a value of undefined"

would be a helpful clarification - so far it's caught us both out :P

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it'd help a lot 👍 Also, could we include the reason why we're doing this? Is there any advantage of using undefined instead of just null?

This PR is applied to the `schema-coordinates` branch PR here: #3044

Implements schema coordinate spec changes per the June 5th 2025 WG discussion

graphql/graphql-spec#794

- Add support for meta-fields (e.g. `__typename`)
- Add support for introspection types
- Revert back from FieldCoordinate+ValueCoordinate -> MemberCoordinate

cc @benjie
src/language/parser.ts Outdated Show resolved Hide resolved
src/language/parser.ts Outdated Show resolved Hide resolved
Comment on lines +133 to +134
resolveSchemaCoordinate,
resolveASTSchemaCoordinate,
Copy link
Contributor

@yaacovCR yaacovCR Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, I vote for utilities

Co-authored-by: Yaacov Rydzinski  <yaacovCR@gmail.com>
@yaacovCR
Copy link
Contributor

Update

The 2nd member of this stack by @leebyron was #3145, with that PR adding a coordinate field to all schema elements, allowing library users to move from schema element => to coordinate (as opposed to this PR, which allows the reverse, from coordinate => to schema element. I have now updated #3808 to be rebased on top of this PR with this PR as the base. We can still merge it separately.

Point for discussion

Now that we have allowed meta-fields in this PR, the other PR #3145/#3808 demonstrates that the schema element <==> coordinate relationship will not allow cycles with meta-fields.

For example, coordinate SomeType.__typename will resolve to TypeNameMetaFieldDef by this PR, an instance of GraphQLField. In #3808, TypeNameMetaFieldDef has a "coordinate" property of <meta>.__typename, which is not a valid coordinate and cannot be resolved back to TypeNameMetaFieldDef. This also affects SchemaMetaFieldDef and TypeMetaFieldDef. The meta-fields are each defined once by the library and then accessed "magically" by schema.getField(...) -- rather than being artificially defined on every type (for __typename) or every root query type of every schema (for the __schema and __type).

To preserve the symmetry we have with non-meta-fields, we would have to make <meta>.__typename or something like it valid and resolve to TypeNameMetaFieldDef and make SomeType.__typename not resolve to TypeNameMetaFieldDef.

Other options would be to simply acknowledge this asymmetry:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" 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.

7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.