-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 #794
base: main
Are you sure you want to change the base?
Schema Coordinates #794
Conversation
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.
Thanks for your hard work on this!
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.
Looks good to me; a few optional tweaks suggested.
spec/Section 3 -- Type System.md
Outdated
Schema Coordinates are always unique. Each type, field, argument, enum value, or | ||
directive may be referenced by exactly one possible Schema Coordinate. |
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.
feels like something worth highlighting and being clear about as a guarantee in the spec
spec/Section 3 -- Type System.md
Outdated
For example, the following is *not* a valid Schema Coordinate: | ||
|
||
```graphql counter-example | ||
Entity.Business | ||
``` | ||
|
||
In this counter example, both `Entity.Business` and `Business` would refer to | ||
the `Business` type. Since it would be ambiguous what the "primary key" should | ||
be in an application that uses Schema Coordinates to reference types, this is | ||
not allowed. |
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.
is this waffle? thought it might be helpful to have a little explanation/demonstration of this property
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.
Could just be
- Since it would be ambiguous what the "primary key" should be in an application that uses Schema Coordinates to reference types, this is not allowed.
+ Such ambiguity is disallowed (and hence why this is is invalid syntax).
(More terse, but doesn't walk the reader through why this is bad)
Voting lines are now open!
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.
In this counter example,
Entity.Business
is redundant sinceBusiness
already uniquely identifies theBusiness
type. Such redundancy is disallowed by this spec - every type, field, field argument, enum value, directive, and directive argument has exactly one canonical Schema Coordinate.
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.
👍 thanks benjie, stealing this wording
Happy new year! 🥳 It's possible we still want more time for the RFC to marinate, but I'll attend so I can be available to folks to discuss the schema coordinates rfc/spec edit in person. graphql/graphql-spec#794 Thanks!
At Indeed we log usage of schema elements to 1) support deprecation and 2) understand usage from a product management perspective (i.e. is this API we built adding value?). We defined our own coordinate system that is not quite as complete as this spec. A nice benefit of formalizing a coordinate system is that shared tools can be built to solve these and other use cases. 👍 👍 👍 |
4f854af
to
c0b82d5
Compare
I made some fairly significant edits to the prose and examples section in an attempt to simplify. @magicmark please take a look and let me know what you think. |
81ef020
to
4299d8c
Compare
IIRC the only thing holding this from Approval stage is a landed GraphQL.js PR? |
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`
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`
3. If {type} is an Enum type: | ||
1. Let {enumValueName} be the value of the second {Name}. | ||
2. Return the enum value of {type} named {enumValueName}. | ||
4. Otherwise if {type} is an Input Object type: | ||
1. Let {inputFieldName} be the value of the second {Name}. | ||
2. Return the input field of {type} named {inputFieldName}. | ||
5. Otherwise: | ||
1. Assert {type} must be an Object or Interface type. | ||
2. Let {fieldName} be the value of the second {Name}. | ||
3. Return the field of {type} named {fieldName}. |
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 section should also specify the behaviour for Union types.
At the moment the spec does not define the behaviour for Union.UnionTypeMember
3. If {type} is an Enum type: | |
1. Let {enumValueName} be the value of the second {Name}. | |
2. Return the enum value of {type} named {enumValueName}. | |
4. Otherwise if {type} is an Input Object type: | |
1. Let {inputFieldName} be the value of the second {Name}. | |
2. Return the input field of {type} named {inputFieldName}. | |
5. Otherwise: | |
1. Assert {type} must be an Object or Interface type. | |
2. Let {fieldName} be the value of the second {Name}. | |
3. Return the field of {type} named {fieldName}. | |
3. If {type} is an Union type: | |
1. Assert {type} must be an Enum, Input Object, Object or Interface type. | |
4. If {type} is an Enum type: | |
1. Let {enumValueName} be the value of the second {Name}. | |
2. Return the enum value of {type} named {enumValueName}. | |
5. Otherwise if {type} is an Input Object type: | |
1. Let {inputFieldName} be the value of the second {Name}. | |
2. Return the input field of {type} named {inputFieldName}. | |
6. Otherwise: | |
1. Assert {type} must be an Object or Interface type. | |
2. Let {fieldName} be the value of the second {Name}. | |
3. Return the field of {type} named {fieldName}. |
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.
Perhaps I'm misunderstanding your question, but to clarify:
UnionTypeMember
The definition for this would live somewhere else in the schema, outside of the context of the union. You would write the schema coordinate to that instead.
i.e. Union.UnionTypeMember
is not a valid schema coordinate.
The rule of thumb is: there should only be one way to reference a thing with Schema Coordinates
Is there still some ambiguity in this spec?
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.
@magicmark Makes sense to me
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.
@leebyron is it worth keeping in the clarification (or something similar) from a previous commit?
My gut says this may crop up again and cause some confusion - worth the counter example?
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.
@leebyron is it worth keeping in the clarification (or something similar) from a previous commit?
magicmark/graphql-spec@
ac5318f
/spec/Section%203%20--%20Type%20System.md?plain=1#L391-L399My gut says this may crop up again and cause some confusion - worth the counter example?
Just chiming in. From my perspective the counter example would be ideal (maybe even clarify that directives on types are also invalid?).
Maybe add a line on why union members are not valid schema coordinates, instead of solely stating that they are not?
- Note: You may not select members inside a union definition.
+ Note: Union members do not uniquely identify elements in the schema. Therefore the following counter example is *not* considered a valid Schema Coordinate:
...or something along those lines
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.
fwiw this is all sort of encapsulated by the following line the spec:
A schema coordinate is always unique. Each schema element may be referenced
by exactly one possible schema coordinate.
++ for the explainer, I would maybe try and reuse this language (e.g. "as stated above, each schema element...."), or hyperlink back to this source of truth somehow
Regarding input arguments, the example is shown as:
What would it look like for more interesting sub-input-types on an argument? Example: input BazInput {
count: Int
color: String
}
input BarInput {
baz_settings: BazInput
}
input FooInput {
bar_params: BarInput
}
type Query {
foos(foo_params: FooInput)
} Would the above translate to the following coordinates?
|
This would be: Query.foos(foo_params:) => Query.foos(foo_params:) |
Indeed, each schema entity should have exactly one schema coordinate. If you want to identify those kinds of paths in an operation (rather than the schema), then operation expressions might be what you’re after: https://github.com/graphql/graphql-wg/blob/main/rfcs/OperationExpressions.md#arguments |
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`
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`
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`
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`
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`
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`
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`
@PascalSenn @benjie Trying to understand the last two comments:
Q1 - "This would be" - intended to mean this is direction the spec will go as it's not covered in this PR? I assume it would also apply to directives, so with
I saw the comment here: https://github.com/graphql/graphql-wg/blob/main/rfcs/SchemaCoordinates.md?plain=1#L370-L375 but it throws me a little as it talks about supporting more than "schema coordinates" but then the first example seems to be a schema coordinate Also note the example |
@ddebrunner Schema Coordinates and Operation Expressions are two separate proposals with related syntax. Every entity in a schema has exactly one schema coordinate, there is no schema coordinate The property You can, however, use Operation Expressions to indicate the accessing of something in this manner, since Operation Expressions are not unique. The comment you quote relates to (what became) Operation Expressions, which is a system that builds upon the Schema Coordinates syntax to talk about paths within operations (rather than the schema). |
@benjie I'm not talking about operations, but schema elements. A type system directive is applied to a schema element and thus is also a schema element(?), so what are the coordinates to such arguments? E.g.
Now if the value of
But I'm assuming that the use of type system directive |
I don't think the application of a directive is covered by this proposal, only the directive itself. At runtime your You could use operation expressions to indicate the path of this directive application, I guess it's more "document expressions" than "operation expressions" thinking about it. |
Fair enough, but I hadn't assumed that the RFC was related to runtime, thus the fact that introspection doesn't return type system directives would seem to be irrelevant to the RFC. |
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`
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`
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`
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`
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`
👋
I've pulled out the proposed spec edits for Schema Coordinates (issue: #735) into this PR.
(The RFC now lives in the original PR: https://github.com/graphql/graphql-spec/pull/746/files)
@leebyron you mentioned it might be possible to simplify the grammar - I played around a bit since the original PR, but I think I need some help here. What did you have in mind?
As suggested, tagging @eapache @mjmahone as additional reviewers as we had some good conversation in last WG meeting about this.
Thanks!