Skip to content
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

Do not render emplty field list #1220

Merged
merged 3 commits into from
Dec 21, 2021

Conversation

Fluxx
Copy link
Contributor

@Fluxx Fluxx commented Dec 20, 2021

According to the GraphQL object spec, for a given type Foo the fields definition for that type is optional. As far as I can tell, if you want to not specify a fields list, you need to also omit the wrapping {...}. i.e:

// This is invalid
type Foo {
}

// This is valid
type Foo

Currently, Caliban always prints schema with the wrapping {...}, even if there are no fields for the type, which I believe is invalid. I tested this against our schema in Apollo and saw the below. Sensitive parts omitted, but the important part is the AST parse failure.

$ echo 'schema {query: Q} type Q {}' | rover subgraph check [...]
Checking the proposed schema for subgraph [...]
error: Could not parse partial schema as AST.

$ echo 'schema {query: Q} type Q' | rover subgraph check [...]
Checking the proposed schema for [...]
Compared 108 schema changes against 25 operations

@ghostdogpr
Copy link
Owner

Looks okay! The CI failed because of formatting, can you run fmt in sbt?

@Fluxx
Copy link
Contributor Author

Fluxx commented Dec 21, 2021

Will do.

Also, we ran into a similar problem where an empty schema also causes issues, where both schema and schema {} are invalid. I will see if I can make that change as part of this PR as well.

@Fluxx
Copy link
Contributor Author

Fluxx commented Dec 21, 2021

Ran fmt and also added some behavior to handle empty schemas. Please take a look!

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ghostdogpr ghostdogpr merged commit 7df8ec8 into ghostdogpr:master Dec 21, 2021
Fluxx added a commit to Fluxx/caliban that referenced this pull request Jan 24, 2022
* Do not render emplty field list

* First round for format

* Handle empty schema as well
Fluxx added a commit to Fluxx/caliban that referenced this pull request Jan 27, 2022
* Do not render emplty field list

* First round for format

* Handle empty schema as well
@Fluxx Fluxx deleted the render/fix_empty_types branch February 4, 2022 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants