Upgrade springboot-java8 to Java 17 LTS - #74
tobydrinkall wants to merge 2 commits into
Conversation
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| <groupId>com.h2database</groupId> | ||
| <artifactId>h2</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
📝 Info: Maven and Gradle builds differ in H2 dependency scope
pom.xml declares com.h2database:h2 in default (compile) scope while build.gradle:40 uses runtimeOnly. No source references H2 classes directly, so both work, but keeping the two build files aligned (e.g. <scope>runtime</scope> in Maven) avoids divergence between the packaged artifacts.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Leaving Maven at compile scope for now — this PR is scoped to the JDK 17 / Boot 2.7.18 upgrade and both builds package H2 identically today. Worth aligning in a follow-up.
| <java.version>17</java.version> | ||
| <maven.compiler.release>17</maven.compiler.release> | ||
| <maven-compiler-plugin.version>3.13.0</maven-compiler-plugin.version> | ||
| <maven-surefire-plugin.version>3.5.6</maven-surefire-plugin.version> |
There was a problem hiding this comment.
🔍 surefire version property is declared but no plugin pins it
maven-surefire-plugin.version is set to 3.5.6, which overrides the Spring Boot parent's managed surefire version. Worth confirming that surefire 3.5.6 is actually released and compatible with the pinned parent, since no <plugin> entry in the build makes the override explicit and a bad value would only surface at test time (this project currently has no tests).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: surefire 3.5.6 resolves and runs under the 2.7.18 parent — the verified build logs --- surefire:3.5.6:test (default-test) @ gs-spring-boot ---. The pin is intentional (a standing version decision for this upgrade), so no explicit <plugin> entry needed.
| gradle: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '17' | ||
| - uses: gradle/actions/setup-gradle@v3 | ||
| - run: ./gradlew -V clean bootJar |
There was a problem hiding this comment.
📝 Info: Gradle CI job skips dependency caching that the Maven job enables
The maven job sets cache: maven on setup-java, but the gradle job relies solely on gradle/actions/setup-gradle@v3. That action does provide Gradle caching by default, so this is not broken, but the asymmetry (no cache: gradle on setup-java) is worth noting if build time matters.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional: gradle/actions/setup-gradle@v3 already caches Gradle dependencies, so adding cache: gradle to setup-java would be redundant.
| } | ||
|
|
||
| RestTemplate restTemplate = new RestTemplate(); |
There was a problem hiding this comment.
📝 Info: Quote is still fetched twice on startup
main calls logRandomQuote after SpringApplication.run has already executed the CommandLineRunner bean that does the same fetch, so the (now warning-only) remote call happens twice per startup. The description acknowledges the duplicated WARN; removing the main-side call and the ad-hoc new RestTemplate() would avoid an unnecessary outbound request on every boot.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional — calling the quote fetch from both main and the CommandLineRunner is an explicit requirement of this upgrade, preserving the original program's behaviour. Now that it only WARNs, the duplicate outbound call is harmless.
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
| <dependency> | ||
| <groupId>javax.annotation</groupId> | ||
| <artifactId>javax.annotation-api</artifactId> | ||
| <version>1.3.2</version> | ||
| </dependency> |
There was a problem hiding this comment.
📝 Info: Unused javax.annotation-api dependency pinned in both builds
No source file under src/main/java references javax.annotation (verified by grep). Since Spring Boot 2.7 still targets Java EE javax.* and the JDK 17 removal of java.xml.ws.annotation only matters when the code actually uses @Resource/@PostConstruct, this explicit 1.3.2 pin is dead weight and an unmanaged version that will drift from Boot's BOM.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The javax.annotation-api:1.3.2 pin is an explicit requirement of this upgrade (a standing decision to keep the javax.* namespace explicitly available rather than implicitly), so it stays even though no source references it today.
| distribution: temurin | ||
| java-version: '17' | ||
| - uses: gradle/actions/setup-gradle@v3 | ||
| - run: ./gradlew -V clean bootJar |
There was a problem hiding this comment.
📝 Info: Gradle CI job only builds bootJar, so test compilation is never exercised
The Maven job runs clean package (compiles and runs tests), but the Gradle job runs only clean bootJar, which never resolves testCompileClasspath or compiles test sources. Any future test-scope dependency problem (e.g. testImplementation("junit:junit") losing BOM-managed versioning) would go undetected in CI. Using ./gradlew build would keep the two jobs symmetric.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The workflow content is specified for this upgrade, and the repo has no test sources at all (surefire logs No tests to run), so bootJar vs build makes no practical difference here. Reasonable follow-up once tests exist.
Summary
Moves both build paths (Maven and Gradle) to JDK 17 with
--release 17and Spring Boot 2.7.18, the last Java 17-compatible 2.x line. Thejavax.*namespace is retained deliberately — nojakarta.*migration.Two non-obvious changes:
pom.xmlhad<packaging>pom</packaging>, which silently preventedspring-boot-maven-pluginfrom producing an executable jar. Nowjar, sotarget/gs-spring-boot-0.1.0.jaris runnable.Applicationfetched a quote from the long-deadgturnquist-quoters.cfapps.ioinmainand in theCommandLineRunner; the resultingRestClientExceptionaborted startup on any JDK. Both call sites now go through one helper that survives both a failed call and an empty body (getForObjectreturnsnullon 204/empty rather than throwing):Build / dependency changes
pom.xml: parent2.0.2.RELEASE→2.7.18;packagingpom→jar;java.version1.8→17; addedmaven.compiler.release=17, explicitmaven-compiler-plugin3.13.0with<release>17</release>,maven-surefire-plugin.version=3.5.6,javax.annotation:javax.annotation-api:1.3.2; removedspring-boot-properties-migrator.build.gradle: Boot Gradle plugin →2.7.18, addedio.spring.gradle:dependency-management-plugin:1.0.15.RELEASE;sourceCompatibility/targetCompatibility 1.8→ Java 17 toolchain +options.release = 17;bootJar.baseName/version→archiveBaseName/archiveVersion(the legacy properties are removed in Gradle 8);compile/testCompile→implementation/testImplementation.3.3.9→3.9.6, Gradle4.6→7.6.4;mvnw/gradlewmade executable so CI can invoke them.CI
New
.github/workflows/build.ymlwith two JDK 17 (temurin) jobs:mvn -B -V clean packageand./gradlew -V clean bootJar. Both are green on this PR.Verification
No tests exist in this repo, so verification is compile + package + runtime smoke test.
java -jar target/gs-spring-boot-0.1.0.jar:The WARN appears twice (once per call site) and startup completes. H2
2.1.214(managed by Boot 2.7.18) accepts the existing inline SQL (DROP TABLE customers IF EXISTS,SERIAL).GET /topic→200:[{"id":"spring","subjectName":"Spring Framework","subjectDescription":"Spring Framework Description"},{"id":"java","subjectName":"Core Java","subjectDescription":"Java Description"},{"id":"javascript","subjectName":"javascript Framework","subjectDescription":"javascript Framework Description"}]Deviations from the plan
java-17-upgrade-v2: a supersededjava-17-upgradebranch already existed on the remote and was left untouched rather than force-pushed.build.gradlewas missingspring-boot-starter-jdbcandh2, which the existing sources require; added so the Gradle build compiles and the jar runs (the Maven build already had them).javax.annotation-api:1.3.2was added topom.xmlas well asbuild.gradleto pin it in both builds.Known pre-existing issue (not fixed — out of upgrade scope)
TopicService.getTopicWithIdcallsOptional.get()without a presence check, soGET /topic/{unknownId}returns HTTP 500. This exists onmasterand is unrelated to the upgrade.Link to Devin session: https://app.devin.ai/sessions/73dca597bcd744a4aa1459f40d98f5c1
Requested by: @tobydrinkall
Devin Review