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

Update to graphql-java v16.1 #84

Closed
wants to merge 4 commits into from
Closed

Update to graphql-java v16.1 #84

wants to merge 4 commits into from

Conversation

hrkfdn
Copy link

@hrkfdn hrkfdn commented Nov 25, 2020

Some breaking changes in this one:

The tests were updated to pass the new validations, but it feels a bit hackish so maybe someone with more knowledge of the test cases/infrastructure should take a closer look. The problem is that the federation transformations are applied too late so some types are empty during validation.

Previously this was included as a transitive dependency via graphql-java
- ExecutionPath was renamed to ResultPath: graphql-java/graphql-java#1962
@apollo-cla
Copy link

@hrkfdn: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

The validations have changed as of v16 to ensure Types define at least
one field.

See also: graphql-java/graphql-java#1955
@dariuszkuc
Copy link
Member

As moving to GraphQL-Java v16.1 is a breaking change, we can remove this
part as it was introduced to support <v13.
@hrkfdn
Copy link
Author

hrkfdn commented Dec 9, 2020

@dariuszkuc Thanks for the heads up, I've cleaned it up a little.

@sachindshinde
Copy link
Contributor

Thanks for opening this PR! You're right that there are some subtleties regarding the new validations introduced in v16 that need to be handled carefully, I'll give an update here soon.

@sachindshinde
Copy link
Contributor

I've iterated on this PR and filed #85 to address some trickier issues:

  • Updates to FederationSdlPrinter need to be handled in a particular way when doing graphql-java upgrades. Specifically, the newer version of SchemaPrinter needs to be copied over FederationSdlPrinter, and then the changeset that was applied to the older version of SchemaPrinter to turn it into FederationSdlPrinter needs to be replayed on top of the newer version of SchemaPrinter (with any now-unneeded bugfix backports removed). This is done to ensure any bugfixes and subtle changes reflected in the newer SchemaPrinter are included in FederationSdlPrinter.
  • The new GraphQL validation unfortunately breaks a common case for federation users, which is providing schemas that have an empty query type or no query type.
    • For users who pass something that isn't a GraphQLSchema to Federation.transform(), we can work around this by adding a dummy field to the GraphQLSchema we generate, and modifying SchemaTransformer.build() to remove this dummy field and not expose it in query { _service { sdl } }.
    • For users who pass a GraphQLSchema to Federation.transform(), there's no way to avoid a breakage there; they'll notice the builder for GraphQLSchema throwing on build() before they even use the library. The easiest solution here I think is to add a new Federation.transform() overload for GraphQLSchema that accepts a flag that indicates whether the query type should be empty; users will then have to add a dummy field on their own prior to building, and set the flag as true.

@smyrick
Copy link
Member

smyrick commented Dec 9, 2020

Maybe the better option here is to open a PR into graphql-java and adding some way to override validations with configuration or plugins. As general schema builder code, I think that could be useful in situations outside of federation too

@sachindshinde
Copy link
Contributor

@smyrick
Would definitely be nice if we could use custom validations in graphql-java schema builders, although I imagine that takes a decent amount of refactoring to make it work.

@hrkfdn
I'm going to close out this PR, as #85 has merged and 0.6.0 has been released.

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