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 v17.3 #127

Merged
merged 10 commits into from
Sep 22, 2021

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented Sep 22, 2021

This PR is an iteration of the changes suggested in #122 (many thanks to @berngp for the PR, and @setchy @paulbakker for the comments). I've added a Co-authored-by section to the commits where changes from #122 were used.

Summary of changes:

  • Bump graphql-java to v17.3 in the pom.xml
  • Update FederationSdlPrinter to use graphql-java v17.3's SchemaPrinter
  • Bumps graphql-java-kickstart artifacts to 11.1.0 and graphql-java-tools to 11.1.2
    • Reason: These artifacts needed updates to support graphql-java v17.
  • Bumps Spring Boot artifacts to 2.5.4, slf4j-api to 1.7.32, junit-jupiter to 5.7.2, and Jackson artifacts to 2.12.5.
    • Reason: Spring Boot needed to be bumped for the newer graphql-java-kickstart artifacts, and the rest needed to be transitively bumped. (These were to ensure we use the upper-bound version of all transitive dependencies, as required by Maven enforcer.)
  • Removes the dependencyManagement entry for spring-aop from spring-example.
    • Reason: It doesn't seem to be needed anymore, as other dependencies currently pull in the right version. (May need it in the future though.)
  • Adds a dependencyManagement entry for graphql-java-extended-scalars to spring-example.
    • Reason: We don't directly rely on this artifact in this repo's code, but one of the graphql-java-kickstart artifacts does, and it appears to declare itself needing an earlier version of graphql-java-extended-scalars than it actually needs. So we use dependencyManagement here to force the version higher in spring-example.

Note that this is a backwards incompatible version bump due to backwards incompatible changes in graphql-java v17 (specifically in the APIs used in FederationSdlPrinter).

sachindshinde and others added 9 commits September 22, 2021 01:50
Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Martin Bonnin <martin@mbonnin.net>
…dlPrinter

Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Martin Bonnin <martin@mbonnin.net>
…s to 11.1.2

Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Bernardo Gomez Palacio <bernardo.gomezpalacio@gmail.com>
…2, and Jackson to 2.12.5, to satisfy Maven enforcer upper bounds in spring-example. We also remove an unneeded dependencyManagement section for spring-aop

Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Bernardo Gomez Palacio <bernardo.gomezpalacio@gmail.com>
…g-example to fix bug in graphql-spring-boot-starter

Co-authored-by: Martin Bonnin <martin@mbonnin.net>
Co-authored-by: Bernardo Gomez Palacio <bernardo.gomezpalacio@gmail.com>
Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

LGTM!

@setchy
Copy link
Contributor

setchy commented Sep 22, 2021

Nice!

@sachindshinde - graphql-java-kickstart 12.0.0 was released on the weekend.

I believe this might be the version you are after as it uses both graphql-java 17.3 and graphql-java-tools 11.1.2

@sachindshinde
Copy link
Contributor Author

@setchy
We actually tried using 12.0.0 initially, but it turns out only some of the graphql-java-kickstart artifacts have been upgraded while others haven't been yet. Specifically, graphiql-spring-boot-starter hasn't been updated yet. (They appear to be evolving versions in lockstep with each other, so I'm guessing it's recommended to use the same versions across them. graphql-java-tools versions OTOH seems to be evolving separately.)

@setchy
Copy link
Contributor

setchy commented Sep 22, 2021

graphql-java-kickstart 12.0.0 had a heap of big changes, one which moved to a BOM model, removed all the previously separate starters (playground, voyager, graphiql, etc) and added application.properties instead. eg: GraphiQL. This is why you won't find any of those other starters part 11.1.0.

re: graphql-java-tools
I've just reached out to the graphql-java-tools maintainers this morning on Slack. I agree they followed an unusual set of versions when releasing 17.x compatible libs (ie: 17.1 -> 11.0.3, 17.2 -> 11.1.2). I've asked if the open graphql-java 17.3 PR could be merged and a new graphql-java-tools 12.x release published to get back in-step with the other kickstart libs

@setchy
Copy link
Contributor

setchy commented Sep 22, 2021

Semi-related - do you have any thoughts on whether spring-example remain in here longer term, or is replaced by implementations contributed to apollo-federation-subgraph-compatibility?

I know you recently added a graphql-java-tools implementation and I followed up a few weeks back with a dgs implementation

… the child POM, as they're not shared

Co-authored-by: Martin Bonnin <martin@mbonnin.net>
@sachindshinde
Copy link
Contributor Author

@setchy

graphql-java-kickstart 12.0.0 had a heap of big changes, one which moved to a BOM model, removed all the previously separate starters (playground, voyager, graphiql, etc) and added application.properties instead. eg: GraphiQL. This is why you won't find any of those other starters part 11.1.0.

Thanks for the insights here!

We tried using 12.0.0 for graphql-java-kickstart and ran into some errors when running spring-boot:run, specifically:

Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'defaultValidator' defined in class path resource [org/springframework/boot/autoconfigure/validation/ValidationAutoConfiguration.class]: Invocation of init method failed; nested exception is java.lang.NoSuchMethodError: javax.validation.BootstrapConfiguration.getClockProviderClassName()Ljava/lang/String;

We got the same issue when trying to use the BOM for 12.0.0.

We also tried using the BOM for 11.1.0, but that ran into this error:

Caused by: java.lang.NoSuchMethodError: graphql.schema.idl.SchemaGeneratorHelper.buildDirective(Lgraphql/schema/idl/SchemaGeneratorHelper$BuildContext;Lgraphql/language/Directive;Ljava/util/Set;Lgraphql/introspection/Introspection$DirectiveLocation;Lgraphql/schema/GraphqlTypeComparatorRegistry;)Lgraphql/schema/GraphQLDirective;

This went away oddly when we manually overrided the graphql-java-tools version to 11.1.2.

I'm not quite sure what's causing the issues here yet, as I haven't really dug into it. It could be an issue in graphql-java-kickstart, or it could be an issue in spring-example's runtime classpath introduced by misconfiguration on our part. Whatever the cause, I don't believe it should block this PR, so I'm going to push the task of using BOMs and 12.0.0 to a later PR.

Semi-related - do you have any thoughts on whether spring-example remain in here longer term, or is replaced by implementations contributed to apollo-federation-subgraph-compatibility?

I know you recently added a graphql-java-tools implementation and I followed up a few weeks back with a dgs implementation

I believe apollo-federation-subgraph-compatibility is more for testing spec compatibility of subgraph libraries/implementations, and not necessarily about presenting simple examples to users, so I do believe spring-example still has some usefulness in that regard. (Ideally though, we could have a way to run gateways in the example so users could see the whole thing working together.)

The other aspect here is that spring-example currently (for better or worse) acts as a simple test of federation-graphql-java-support. Ideally we should separate testing from examples.

Regarding issues about new graphql-java versions releasing and us wanting to support it before graphql-java-kickstart supports it, child POMs can evolve independently, so we can release newer versions of federation-graphql-java-support without releasing the example (albeit it's a bit manual/painful). However, because the example is used as a test, there is some value to potentially wait for graphql-java-kickstart to support it first. (Hopefully one day we won't have to wait on this.)

@sachindshinde sachindshinde merged commit 91b81ce into master Sep 22, 2021
@sachindshinde sachindshinde deleted the sachin/graphql-java-17.3 branch September 22, 2021 17:29
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

3 participants