Upgrade to graphql-java 17.0

[berngp]

This commit upgrades the framework to graphql-java 17.0.

Required changes.

• Upgrade to graphql-java-tools 11.1.0, it now supports graphql-java 17.0
• Include graphql-java-extended-scalars since it was moved out of graphql-java.
• Upgrade to Spring Boot 2.4.5 due that graphql-java-kickstarter requires a min of 2.4.5. If we don't do this the MVN Enforcer Plugin will fail the build. Same case for junit, log4j, and spring-aop.

[apollo-cla]

@berngp: 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/

[setchy]

@berngp - graphql-java-tools v11.1.0 released yesterday, includes support for graphql-java v17

https://github.com/graphql-java-kickstart/graphql-java-tools/releases
note: v11.0.3 and v11.1.0 have the same release notes

[paulbakker]

@berngp - graphql-java-tools v11.1.0 released yesterday, includes support for graphql-java v17

https://github.com/graphql-java-kickstart/graphql-java-tools/releases
note: v11.0.3 and v11.1.0 have the same release notes

I'm wondering why this is relevant? The federation lib shouldn't have a dependency on graphql-java-kickstart.

[setchy]

Correct, I believe it's limited to the spring-examples @paulbakker

[paulbakker]

Correct, I believe it's limited to the spring-examples @paulbakker

This is getting a bit off-topic, but wouldn't it be a lot easier (from a maintenance perspective) dropping the spring-examples and keep examples/testing limited to integration with graphql-java directly? How you wire things up in a Spring project doesn't need to be a concern of this library.

[berngp]

@setchy upgraded to graphql-java-tools v11.1.0 and included the graphql-java-extended-scalars dependency as well since it is not longer available as part of graphql-java.

[berngp]

@setchy happy to squash if you rather have the pr merged as a single commit.

[berngp]

@setchy scratch that. I had to rebase and then fix some dependency version that the MVN Enforcer plugin now complained about.

[setchy]

@sachindshinde 👆 - appreciate your help to review and merge/release

[sachindshinde]

@setchy Thanks for the ping here! I can take a look at this sometime later this week/early next week (as part of QA, we'll also have to smoke test a release RC in Apollo's infra prior to release).

EDIT: Apologies for the delays here. I'll be helping an engineer ramp up on the codebase to help maintain it, so we'll need to push back the timeline a bit to 2021/09/07 for review and release.

[berngp]

Kindly asking for an update. Thanks
Just read that the review was pushed. Thanks for the update.

[sachindshinde]

Update here: Due to an incident escalation in our backend, we couldn't get to this PR today, but we intend to get to this tomorrow (2021/09/08).

[sachindshinde]

Update here: While we have temporarily resolved the incident, product/planning has decided our team must take an interrupt this sprint to resolve the incident in the medium-term, and has asked we push back the date here two weeks to 2021/09/22 (apologies for the delays here again 🙇).

[sachindshinde]

@berngp
The changes here for versioning looked mostly fine, although there are a few issues:

• For FederationSdlPrinter, when we bump graphql-java versions, we usually don't try to tweak the printer to make it compile and pass tests, but instead just rebase the changes introduced by this repo onto the new version of SchemaPrinter.
  • This usually involves copying the SchemaPrinter used by that graphql-java version, renaming the class, and then replaying the changeset introduced by this repo onto that.
  • There a few reasons we opt for this route:
    • The changeset this repo introduces on SchemaPrinter is rather small.
    • The tests in this repo for FederationSdlPrinter are very minimal compared to the tests that graphql-java uses to test SchemaPrinter. The tests in this repo for FederationSdlPrinter are primarily to test the features that this repo adds to SchemaPrinter, and not the base functionality of SchemaPrinter itself.
      • This means that tweaking FederationSdlPrinter until it compiles and tests pass may be insufficient, as it will miss edge cases (e.g. subtle changes in API behavior).
    • Rebasing the changeset on the newer SchemaPrinter of graphql-java means that we'll get bugfixes to SchemaPrinter, in addition to the changes needed to SchemaPrinter to support the new API changes (in addition to subtle changes that need to be made, by virtue of the maintainers of graphql-java knowing more about the subtleties of their API changes).
  • In the near future, we expect to delete FederationSdlPrinter, and go back to relying on the SchemaPrinter from graphql-java. This should hopefully make these kinds of PRs substantially easier.
    • The reason we have FederationSdlPrinter is to support very old gateways that have odd requirements of the subgraph schemas presented at query { _service { sdl } } (specifically, they require standard and federated GraphQL directive definitions and types to be omitted from that SDL, making them invalid GraphQL).
      • These old gateways wouldn't work with newer GraphQL features usable in newer graphql-java versions, so there's an argument to be made that it's fine to drop support for them, as they couldn't really use newer graphql-java versions in a safe way (in other words, we only partially supported that use case).
• Regarding the dependencyManagement section for the graphql-java-extended-scalars artifact, it appears to be needed due to a bug in graphql-java-kickstart. (We don't use extended scalars in this repo explicitly, but they're used by dependencies.) At a cursory glance, it looks like code somewhere in a graphql-java-kickstart artifact is calling methods/classes from a newer version of graphql-java-extended-scalars than is requested by that artifact.
  • This can indeed be resolved though dependencyManagement, although I'd prefer that it be placed next to the Jackson dependencyManagement entries in the spring-example POM since this is specifically an issue with a transitive dependency of spring-example.
  • The scope of the dependencyManagement shouldn't be test, as it's also needed at runtime. (E.g. running mvn -pl spring-example spring-boot:run will fail if it's just test scope.)

We're looking to cut an RC release this Wednesday morning and make a release by Wednesday EOD, so I've went ahead and implemented the above suggestions in #127 .

[berngp]

Thanks for taking care of the changes with #127. Will close this PR in favor of it.