schema-kitchen-sink.graphql has out-of-spec test case

[swolchok]

Line 67 of schema-kitchen-sink.graphql is extend type Foo @onType {}. According to both the prose schema language extension spec and the full summary of the grammar from graphql/graphql-spec#90, this is ungrammatical:

TypeExtensionDefinition : extend ObjectTypeDefinition
ObjectTypeDefinition : type Name ImplementsInterfaces? Directives? { FieldDefinition+ }

At least one FieldDefinition must be included to match FieldDefinition+ according to the notation conventions:

A subscript suffix "{Symbol+}" is shorthand for a list of one or more of that symbol.

I'm not sure if this is a bug in the (admittedly RFC) spec or the graphql-js implementation.

[swolchok]

Also applies to type NoFields {} on the following non-blank line.

[tcr]

Changing the parser from "any" to "many" results in these tests failing:

• type Hello implements World { }
• type Hello implements Wo, rld { }
• extend type Foo @onType {} (as well as in schema-printer-test)
• type NoFields {} (as well as in schema-printer-test)
• type Hello implements Bar {}

A cursory look through Github (via the BigQuery public dataset) didn't turn up any public uses which expect zero fields.

[leebyron]

Thanks for the investigation, @tcr!

There's two directions we could go. One is to change this implementation from "any" to "many" as you suggest, the other is to change the language in the spec proposal from Term+ to Term* (essentially, make the "any" implementation correct)

One benefit from the "many" approach suggested in the spec proposal is that schema text like type NoFields {} is intentionally non-parsing. That means that it's not possible to write an invalid schema where a type has no fields. However that also leads to a drawback of the "many" approach which is that the extend type may be extending in a way which does not add fields. The extend type Foo @onType {} example should be possible - it describes adding some metadata to a type via a directive, or extend type Foo implements Bar {} which describes how Foo should implement Bar but assumes Foo already defines the necessary fields.

I'm open to feedback about how to address this, but I'm leaning in favor of leaving this implementation as is, but changing the spec proposal to the "any" case to match, thus potentially allowing invalid schema text to be written, but not limiting otherwise reasonable use cases. The invalid schema text can still be caught by other systems - schema validation (which we already have baked into the schema constructor functions) as an example.

[wincent]

I think you're leaning in the right direction.

[leebyron]

I can also scope the change only to ObjectTypeDefinition - the other definitions don't apply to extend type

[tcr]

To summarize my understanding:

• type NoFields {} is undesirable but not yet defined as an error—you can still query it as no_fields { __typename } for example.
• extend Type Foo implements Bar {} should be allowed.
• extend type Foo @onType {} should be allowed, and by extension type Foo @addFields {} should be too?

And of course you could split ObjectTypeDefinition into separate cases for extend and regular type decls. But given the different choices, I don't see much benefit of enforcing at the syntax level that a type has more than 0 fields. So changing the spec proposal to ObjectTypeDefinition* makes sense to me.

[IvanGoncharov]

@swolchok @tcr It was fixed in graphql/graphql-spec@bde02b9
By allowing allowing to omit curly braces, e.g. type NoField, extend type Foo implements Bar and extend type Foo @onType.

graphql-js was adapted to this change in #1117
So I think this issue can be closed.

[leebyron]

Thanks for following up on this, @IvanGoncharov! I definitely intended to mark this issue fixed with that set of changes.