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: Bump graphql-java to v16.1 #85

Merged
merged 10 commits into from
Jan 21, 2021

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented Dec 9, 2020

This PR is an iteration of the changes suggested in #84 (many thanks to the authors and commenters).

Summary of changes:

  • Bump graphql-java to v16.1 in the pom.xml
  • Rename ExecutionPath to ResultPath
  • Add a dependency on org.slf4j:slf4j-api
  • Update FederationSdlPrinter to use graphql-java v16.1's SchemaPrinter
  • Add a new flag called queryTypeShouldBeEmpty to the Federation.transform() overload that accepts a GraphQLSchema to indicate that the query type in the given schema should really be empty, and that the type only contains dummy fields to satisfy GraphQL validation.
    • Reason: GraphQL object types can no longer be empty because of a new validation introduced in New validation rules about type, field, argument graphql-java/graphql-java#1955 . To be clear this is in the GraphQL spec, but graphql-java hadn't been implementing it; this is an issue with federation SDL not being valid GraphQL.
      • This new validation means we need to remove the testPrinterEmpty() test, since we can no longer represent empty types in a GraphQLSchema object.
    • For users who use other overloads of Federation.transform(), all of this is invisible; we add the dummy field for them, and it's accordingly removed in SchemaTransformer.build(). This lets us maintain backwards compatibility with users who provide empty query type (or who provide no query type) in the common case.
      • Note that there's no way to avoid the compatibility breakage for users who build their own GraphQLSchema object, as they do this before they call this library. The best option here I think is to provide clear steps in the CHANGELOG.md for the fix.
  • Simplify ExecutionResultImpl builder calls in FederatedTracingInstrumentation.instrumentExecutionResult()
    • Reason: There is now a copy method for this builder that accepts an ExecutionResult.

Note that this is a backwards incompatible version bump due to backwards incompatible changes in graphql-java v16 (e.g. the ExecutionPath -> ResultPath rename).

@smyrick
Copy link
Member

smyrick commented Jan 9, 2021

@sachindshinde This PR looks good from my perspective as a library user. Is there anything else blocking the review?

@williamboman-pp
Copy link

Would be nice to get this in place so we don't get stuck on old graphql-java versions

@setchy
Copy link
Contributor

setchy commented Jan 19, 2021

Any further updates? Many of the libraries we depend on were just released over the weekend to use graphql-java:16.1 and this is the final one in our stack

@sachindshinde sachindshinde marked this pull request as ready for review January 19, 2021 15:06
@sachindshinde
Copy link
Contributor Author

@smyrick @williamboman-pp @setchy
Apologies for the delays, I've finished changes to code and tests and taken this PR out of draft status. The only merge-blocker is that we'd like to run an RC in our own backend before cutting a release.

@setchy
Copy link
Contributor

setchy commented Jan 19, 2021

@smyrick @williamboman-pp @setchy
Apologies for the delays, I've finished changes to code and tests and taken this PR out of draft status. The only merge-blocker is that we'd like to run an RC in our own backend before cutting a release.

Great news - thanks @sachindshinde for the update

@williamboman-pp
Copy link

williamboman-pp commented Jan 19, 2021

Let me know if you want help testing, we could deploy this to our staging envs as well

Copy link
Contributor

@pcarrier pcarrier left a comment

Choose a reason for hiding this comment

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

🎉

@sachindshinde sachindshinde merged commit e612aa8 into master Jan 21, 2021
@sachindshinde sachindshinde deleted the sachin/graphql-java-v16 branch January 21, 2021 20:38
@sachindshinde
Copy link
Contributor Author

Our backend showed no issues with the RC, so I've cut release 0.6.0.

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

5 participants