Fix adding descriptions to type extensions #1582
Conversation
If a schema had both a type definition and a correspdonding type
extension, the printing code was mistakenly printing the description of
the type definition on the type extension as well. But since description
are not syntactically valid on type extension in graphQL, this leads to
syntactic errors.
More precisely, an input schema like:
```graphql
""" I'm a type """
type T {
a: Int
}
extend type T {
b: Int
}
```
was (before this patch) mistakenly printed as:
```graphql
""" I'm a type """
type T {
a: Int
}
""" I'm a type """
extend type T {
b: Int
}
```
which is syntactically invalid due to the description on the type
extension.
Note: the patch also fix a related additional issue where the presence
of type extension might have the type definition description ignored due
a lack of conditional when setting descriptions in `buildSchema`.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| @@ -299,7 +299,9 @@ function buildNamedTypeInner( | |||
| const enumType = type as EnumType; | |||
| for (const enumVal of definitionNode.values ?? []) { | |||
| const v = enumType.addValue(enumVal.name.value); | |||
| v.description = enumVal.description?.value; | |||
| if (enumVal.description) { | |||
There was a problem hiding this comment.
Is this a change? Behavior looks the same to me.
There was a problem hiding this comment.
See comment on types below. I actually don't think the change is necessary in that case of enum values, but just made it to mirror the other similar cases in the file.
| @@ -315,7 +317,9 @@ function buildNamedTypeInner( | |||
| break; | |||
| } | |||
| buildAppliedDirectives(definitionNode, type, extension); | |||
| type.description = definitionNode.description?.value; | |||
| if (definitionNode.description) { | |||
There was a problem hiding this comment.
That's the part I'm referring to in the "Note" of the commit message: it's not the same behaviour in that this code may run multiple times for the same type, so if we had a description the first time (say on the type definition), then we were erasing it if there was no description the 2nd time (say on the type extension). The change makes it so we don't erase it anymore.
| @@ -427,6 +427,10 @@ test('handling of descriptions', () => { | |||
| pages: Int @Important | |||
| } | |||
|
|
|||
| extend type Book { | |||
There was a problem hiding this comment.
do we need to check the result anywhere?
There was a problem hiding this comment.
The test ends by comparing the result of printSchema and the original input. So before this patch the test would fail due the bug solved here.
If a schema had both a type definition and a correspdonding type
extension, the printing code was mistakenly printing the description of
the type definition on the type extension as well. But since description
are not syntactically valid on type extension in graphQL, this leads to
syntactic errors.
More precisely, an input schema like:
was (before this patch) mistakenly printed as:
which is syntactically invalid due to the description on the type
extension.
Note: the patch also fix a related additional issue where the presence
of type extension might have the type definition description ignored due
a lack of conditional when setting descriptions in
buildSchema.