Schema Coordinates #3044

leebyron · Apr 16, 2021

Depends on #3115

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

src/language/lexer.js

magicmark

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

src/language/ast.js

src/utilities/resolveSchemaCoordinate.js

leebyron · 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

src/language/ast.ts

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`

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

…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 · 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.

JoviDeCroock · May 22, 2025

src/utilities/index.ts

+// Schema coordinates
+export {
+  resolveSchemaCoordinate,
+  resolveASTSchemaCoordinate,


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

src/language/__tests__/lexer-test.ts

src/language/ast.ts

martinbonnin · May 25, 2025

src/utilities/index.ts

+  resolveSchemaCoordinate,
+  resolveASTSchemaCoordinate,


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

~~fwiw, I vote for utilities~~

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

src/utilities/resolveSchemaCoordinate.ts

magicmark · 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 · Jun 2, 2025

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 · 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

src/language/tokenKind.ts

martinbonnin · Jun 6, 2025

src/utilities/__tests__/resolveSchemaCoordinate-test.ts

+      type: schema.getType('String'),
+    });
+
+    expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(undefined);


should this be null?

Suggested change

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

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

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

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?

undefined is what @leebyron's original implementation used. I don't have a strong opinion either way here, there's a valid semantic argument for either. I would be ok to switch. Do you feel strongly that it should be null?

From an outside vantage point, it sure looks surprising that the spec says null but graphql-js returns undefined.

But also I'm not the biggest graphql-js connoisseur out there so I'll leave y'all to decide.

@benjie wdyt?

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

yaacovCR · Jun 9, 2025

src/utilities/index.ts

+  resolveSchemaCoordinate,
+  resolveASTSchemaCoordinate,


~~fwiw, I vote for utilities~~

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

yaacovCR · Jun 10, 2025

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:

perhaps by renaming the coordinate property introduced by Add coordinate field to schema element definitions #3808 to something else
perhaps by not including a coordinate property at all => schemaElement.toString() essentially holds that value anyway on the main branch after the work done in introduce GraphQLField, GraphQLInputField, GraphQLArgument, and GraphQLEnumValue #4288)
perhaps simply by documentation

magicmark · Jun 10, 2025

a bare <meta>.__typename isn't something that would/could exist in a GraphQL query document right? I can't think what use cases there would be to need to support that as something we'd want to parse as a schema coordinate. Therefore, your suggestion of "perhaps simply by documentation" (and possibly a runtime assertion if required) sg2m?

leebyron requested a review from IvanGoncharov April 16, 2021 09:33

leebyron force-pushed the schema-coordinates branch 2 times, most recently from c7b87e2 to 0a5630a Compare April 16, 2021 09:46

IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Apr 16, 2021

magicmark reviewed Apr 16, 2021

View reviewed changes

src/language/lexer.js Outdated Show resolved Hide resolved

magicmark approved these changes Apr 16, 2021

View reviewed changes

src/language/printer.js Outdated Show resolved Hide resolved

IvanGoncharov reviewed Apr 20, 2021

View reviewed changes

src/language/ast.js Outdated Show resolved Hide resolved

IvanGoncharov reviewed Apr 20, 2021

View reviewed changes

src/utilities/resolveSchemaCoordinate.js Outdated Show resolved Hide resolved

leebyron force-pushed the schema-coordinates branch from a4a8920 to 42f9072 Compare April 22, 2021 07:27

leebyron requested a review from IvanGoncharov April 22, 2021 16:58

leebyron force-pushed the schema-coordinates branch from 42f9072 to 1399f7c Compare May 27, 2021 23:55

leebyron changed the base branch from main to lexer-cleanup May 27, 2021 23:55

leebyron added the PR: feature 🚀 requires increase of "minor" version number label May 27, 2021

leebyron force-pushed the lexer-cleanup branch 2 times, most recently from 3451e3e to 2f893d6 Compare May 28, 2021 22:21

leebyron force-pushed the schema-coordinates branch from 1399f7c to 3cec8c2 Compare May 28, 2021 22:31

leebyron mentioned this pull request May 28, 2021

Add coordinate field to schema element definitions #3145

Open

leebyron force-pushed the schema-coordinates branch from 3cec8c2 to e688f88 Compare May 30, 2021 06:30

IvanGoncharov reviewed Jun 1, 2021

View reviewed changes

src/language/ast.ts Outdated Show resolved Hide resolved

leebyron force-pushed the schema-coordinates branch from e688f88 to c68ca9b Compare June 2, 2021 17:55

leebyron requested a review from IvanGoncharov June 2, 2021 17:55

leebyron mentioned this pull request Jun 2, 2021

[2021-01-07] Schema Coordinates: Add functions to graphql-js for schema coordinate parsing/printing graphql/graphql-wg#581

Closed

leebyron force-pushed the lexer-cleanup branch from 0d6a096 to b6864ac Compare June 3, 2021 16:51

leebyron force-pushed the schema-coordinates branch from c68ca9b to 8fa8a4c Compare June 3, 2021 17:00

Base automatically changed from lexer-cleanup to main June 3, 2021 17:34

leebyron force-pushed the schema-coordinates branch from 8fa8a4c to 98e7541 Compare June 3, 2021 17:53

martijnwalraven mentioned this pull request Jun 18, 2021

Alternative GraphQL schema abstraction apollographql/federation#823

Closed

yaacovCR mentioned this pull request Dec 29, 2022

RFC: Schema Coordinates #3807

Closed

yaacovCR mentioned this pull request Jan 2, 2023

RFC: Default value validation & coercion #3814

Merged

This was referenced Sep 21, 2024

Error messages have non-standard capitalization and quote usage graphql/graphql-spec#1112

Closed

Error messages have non-standard capitalization and quote usage #4196

Open

yaacovCR mentioned this pull request Nov 7, 2024

introduce GraphQLField, GraphQLInputField, GraphQLArgument, and GraphQLEnumValue #4288

Merged

benjie added 4 commits May 22, 2025 10:22

Merge branch 'main' into schema-coordinates

8064fd3

Appease TypeScript

a23ecd1

Change bad syntax test

c7a9fdb

Fix error deep equality tests

a59ce3f

benjie requested a review from a team as a code owner May 22, 2025 09:47

JoviDeCroock approved these changes May 22, 2025

View reviewed changes

benjie mentioned this pull request May 22, 2025

Schema Coordinates graphql/graphql-spec#794

Open

martinbonnin reviewed May 25, 2025

View reviewed changes

benjie mentioned this pull request Jun 5, 2025

Updates to schema coordinates (#3044) #4422

Merged

1 task

Updates to schema coordinates (#3044) (#4422)

bbc3dfa

Co-authored-by: Benjie <benjie@jemjie.com>

benjie reviewed Jun 5, 2025

View reviewed changes

src/language/__tests__/printer-test.ts Show resolved Hide resolved

benjie reviewed Jun 5, 2025

View reviewed changes

src/language/tokenKind.ts Outdated Show resolved Hide resolved

Add missing "as const"

b759e92

martinbonnin reviewed Jun 6, 2025

View reviewed changes

magicmark mentioned this pull request Jun 6, 2025

Implement schema coordinate spec as of 2025-06-06 #4432

Merged

yaacovCR approved these changes Jun 9, 2025

View reviewed changes

Apply suggestions from code review

1f70a93

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

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

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

Schema Coordinates #3044

Are you sure you want to change the base?

Schema Coordinates #3044

Uh oh!

Conversation

leebyron commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

magicmark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leebyron commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

benjie commented May 22, 2025

Uh oh!

JoviDeCroock May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

martinbonnin May 25, 2025

Choose a reason for hiding this comment

Uh oh!

yaacovCR Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaacovCR Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

magicmark commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinbonnin commented Jun 2, 2025

Uh oh!

benjie commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

martinbonnin Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

magicmark Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbonnin Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

magicmark Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbonnin Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

magicmark Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yaacovCR Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaacovCR commented Jun 10, 2025

Update

Point for discussion

Uh oh!

magicmark commented Jun 10, 2025

Uh oh!

leebyron commented Apr 16, 2021 •

edited

Loading

leebyron commented Apr 22, 2021 •

edited

Loading

yaacovCR Jun 9, 2025 •

edited

Loading

magicmark commented Jun 2, 2025 •

edited

Loading

magicmark Jun 9, 2025 •

edited

Loading

magicmark Jun 10, 2025 •

edited

Loading

yaacovCR Jun 9, 2025 •

edited

Loading