Skip to content

SDLParser disallow empty type curly brace syntax #3620

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

SDLParser disallow empty type curly brace syntax #3620

wants to merge 1 commit into from

Conversation

tinnou
Copy link
Contributor

@tinnou tinnou commented Jun 5, 2024

Description

This is a backward incompatible change.

The GraphQL spec no longer allow empty brackets after empty type definitions, e.g (type Foo {} is not valid and should be type Foo), while this was specifically left in the parser in 2019 for backwards compatibility reasons, feel free to merge if/when you think it's time to move forward.

PR #3619 should merge first before this so it's possible to re-parse a printed SDL.

@tinnou
SDLParser disallow empty type curly brace syntax
25a8121 
GraphQL spec no longer allow empty brackets after empty type definitions.
@bbakerman
Copy link
Member

While more modern spec compliant - I am not sure we would ever want to pull the trigger on this.

There must be 1000s of schemas out there using type Foo{} and extend type Foo constructs and this will break them.

I am not sure the juice is worth the squeeze in this case. A minor spec advancement for possible major pain.

I would love to hear from others on this

@bbakerman bbakerman added the breaking change requires a new major version to be relased label Jun 6, 2024
@tinnou
Copy link
Contributor Author

tinnou commented Jun 6, 2024

While more modern spec compliant - I am not sure we would ever want to pull the trigger on this.

There must be 1000s of schemas out there using type Foo{} and extend type Foo constructs and this will break them.

I am not sure the juice is worth the squeeze in this case. A minor spec advancement for possible major pain.

I would love to hear from others on this

My thought was we can shelf this until #3619 has been merged and graphql-java has gone through a few major versions. But even then, there is no way to tell for sure "when" is a good time, especially in cases where SDL has been saved in some cold storage. Feel free to close.

@tinnou tinnou closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbakerman bbakerman Awaiting requested review from bbakerman

@andimarek andimarek Awaiting requested review from andimarek

@dondonz dondonz Awaiting requested review from dondonz

Assignees
No one assigned
Labels
breaking change requires a new major version to be relased
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tinnou @bbakerman