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

graphql-java-support: Fix grammar violation in graphql-java's SchemaPrinter #52

Merged
merged 9 commits into from
Mar 19, 2020

Conversation

sachindshinde
Copy link
Contributor

graphql-java's SchemaPrinter outputs grammar-violating SDL, as described in graphql-java/graphql-java#1823 . This has affected users of federation-jvm, as shown in #21 and #23 .

It's been fixed upstream by graphql-java/graphql-java#1798 , but this hasn't been released yet. This PR introduces a hotfix until graphql-java releases their fix (and backports it to graphql-java v13).

The gist is we copy SchemaPrinter from graphql-java v13 as FederationSdlPrinter, and backport the fixes from graphql-java/graphql-java#1798 . I've added a basic test to FederationTest to check behavior for the relevant types in the backfix (objects, interfaces, enums, and input objects).

For reviewing, it'll help to diff FederationSdlPrinter against the v13 SchemaPrinter, located at https://raw.githubusercontent.com/graphql-java/graphql-java/v13.0/src/main/java/graphql/schema/idl/SchemaPrinter.java , and compare the diff against the changes files at graphql-java/graphql-java#1798 .

Copy link

@zionts zionts left a comment

Choose a reason for hiding this comment

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

This looks great to me! The one change I would like is referencing this as a temporary patch as a comment within the code so that we can go back to the GraphQL-Java schema printer once it fixes this issue.

@sachindshinde sachindshinde merged commit f8b8162 into master Mar 19, 2020
@zionts zionts deleted the sachin/remove-empty-query-type-from-sdl branch May 12, 2020 22:54
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.

None yet

2 participants