Parse SDL definitions and extensions with optional bodies, per spec#3670
Parse SDL definitions and extensions with optional bodies, per spec#3670trevor-scheer wants to merge 11 commits intographql:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 0f7148c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
trevor-scheer
changed the title
extend schema parsing issue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3670 +/- ##
==========================================
+ Coverage 65.32% 65.35% +0.02%
==========================================
Files 122 122
Lines 7003 7003
Branches 2260 2265 +5
==========================================
+ Hits 4575 4577 +2
+ Misses 2411 2409 -2
Partials 17 17
🚀 New features to boost your workflow:
|
trevor-scheer
changed the title
extend schema parsing issueextend schema correctly when root operations list is absent
|
Sorry @trevor-scheer — I am not familiar with the code base here. Just have contributed what I could to get incremental delivery moving… |
|
Hey @acao, is there anything I can do to help move this along? |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
This looks correct to me, the tests look sensible.
However; it looks like the same issue applies to object, interface, enum and input object extensions?
Actually... for those, it looks like the {...} should be optional in all cases (extension and original definition), though they appear to be required in this parser definition? It's not immediately obvious to me why schema doesn't follow that same pattern, e.g. if you can have
input Foo @oneof
scalar Bar
extend input Foo {
int: Int
bar: Bar
}why could you not also have:
schema @blah
type Query {
int: Int
}
extend schema {
query: Query
}Perhaps this warrants a spec tweak. That's out of scope for this PR though!
I'm unfamiliar with the parser, so I defer to someone more familiar.
|
Regarding my above comment, I did raise a spec tweak to allow However, Lee doesn't see value in this: https://youtu.be/Lo0OhLoMBII?t=2754 If you have a strong use case for it, please let me know via that pull request! |
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Per the GraphQL spec, both `ObjectTypeDefinition` and
`ObjectTypeExtension` permit omitting the `{ FieldDefinition+ }` body:
- https://spec.graphql.org/draft/#sec-Objects
- https://spec.graphql.org/draft/#sec-Object-Extensions
Previously the parser required the body in `ObjectTypeDef`, which made
forms like `type Foo @directive` fail to parse cleanly when followed by
another definition. Extract the body into a new `FieldDefs` rule and
make it optional. `[Kind.OBJECT_TYPE_EXTENSION]` continues to reuse
`ObjectTypeDef` and inherits the relaxed grammar.
Per the GraphQL spec, both `InterfaceTypeDefinition` and
`InterfaceTypeExtension` permit omitting the `{ FieldDefinition+ }`
body:
- https://spec.graphql.org/draft/#sec-Interfaces
- https://spec.graphql.org/draft/#sec-Interface-Extensions
Mark the body as optional in `InterfaceDef` (sharing the new
`FieldDefs` rule with `ObjectTypeDef`).
`[Kind.INTERFACE_TYPE_EXTENSION]` continues to reuse `InterfaceDef`
and inherits the relaxed grammar.
Per the GraphQL spec, both `UnionTypeDefinition` and `UnionTypeExtension` permit omitting the `= NamedType | …` member list: - https://spec.graphql.org/draft/#sec-Unions - https://spec.graphql.org/draft/#sec-Union-Extensions Extract the `=` and member list into a new `UnionMembers` rule and mark it as optional in `UnionDef`. `[Kind.UNION_TYPE_EXTENSION]` continues to reuse `UnionDef` and inherits the relaxed grammar.
Per the GraphQL spec, both `EnumTypeDefinition` and
`EnumTypeExtension` permit omitting the `{ EnumValueDefinition+ }`
body:
- https://spec.graphql.org/draft/#sec-Enums
- https://spec.graphql.org/draft/#sec-Enum-Extensions
Extract the body into a new `EnumValueDefs` rule and mark it as
optional in `EnumDef`. `[Kind.ENUM_TYPE_EXTENSION]` continues to reuse
`EnumDef` and inherits the relaxed grammar.
Per the GraphQL spec, both `InputObjectTypeDefinition` and
`InputObjectTypeExtension` permit omitting the
`{ InputValueDefinition+ }` body:
- https://spec.graphql.org/draft/#sec-Input-Objects
- https://spec.graphql.org/draft/#sec-Input-Object-Extensions
Extract the body into a new `InputValueDefs` rule and mark it as
optional in `InputDef`. `[Kind.INPUT_OBJECT_TYPE_EXTENSION]` continues
to reuse `InputDef` and inherits the relaxed grammar.
trevor-scheer
changed the title
extend schema correctly when root operations list is absent6 participants
The online parser required a body (
{ ... }, or= ...forunion) in several places where the GraphQL spec marks the body as optional, causing otherwise-valid schema documents to fail to tokenize cleanly — especially when a body-less definition or extension was followed by another definition.This PR aligns the parser with the spec for each construct:
extend schema(no root operation defs)type/extend type(no fields body)interface/extend interface(no fields body)union/extend union(no=member list)enum/extend enum(no values body)input/extend input(no fields body)Each construct gets its own commit for easy review.
Out of scope
{ implements, directives, body }(andextend scalarto have at least one directive). The parser is already permissive around such constraints (e.g. zero-or-moreDirectivelists), so this PR doesn't try to enforce them — that's validator territory, not a syntax-highlighting parser.