Upgrade Java 8 -> 17 (Spring Boot 2.7.18) for Maven and Gradle builds - #56
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:
|
| try { | ||
| Quote quote = restTemplate.getForObject("http://gturnquist-quoters.cfapps.io/api/random", Quote.class); | ||
| log.info(String.valueOf(quote)); | ||
| } catch (Exception e) { | ||
| log.warn("Could not fetch random quote: {}", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
📝 Info: Broad catch around the quote fetch hides stack traces and non-network failures
catch (Exception e) plus log.warn(..., e.getMessage()) swallows every failure mode of the call — including JSON deserialization errors and programming errors — and logs only the message with no stack trace, which makes real regressions in this code path invisible. Narrowing to RestClientException (or at least passing e to the logger) would keep the graceful-degradation goal while preserving diagnostics. Same pattern appears in both main() and the CommandLineRunner bean (src/main/java/hello/Application.java:56-62).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The broad catch (Exception) is intentional here: the explicit goal of this change is that a dead/misbehaving demo endpoint must never prevent the app from booting on JDK 17, so I don't want to let e.g. a deserialization error propagate out of the CommandLineRunner and abort startup again. I take your point about diagnostics though — since this is a throwaway demo call, dropping the stack trace is acceptable, but I'm happy to switch to logging the full exception (log.warn("...", e)) if you'd prefer to keep the trace. Leaving the broad catch as-is unless the maintainer wants it narrowed.
| try { | ||
| Quote quote = restTemplate.getForObject( | ||
| "http://gturnquist-quoters.cfapps.io/api/random", Quote.class); | ||
| log.info(String.valueOf(quote)); | ||
| } catch (Exception e) { | ||
| log.warn("Could not fetch random quote: {}", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
📝 Info: Dead external host retained in code after the failure is made non-fatal
Both call sites still point at http://gturnquist-quoters.cfapps.io/api/random, a host that no longer resolves. With the new try/catch the app boots, but every startup now performs a doomed DNS lookup/connection attempt (two of them: one in main() and one in the CommandLineRunner bean) and emits a warning, adding avoidable startup latency depending on resolver timeouts. Removing the demo call or pointing it at a live endpoint would be cleaner, as the PR description itself suggests.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed this is the cleaner end state. I deliberately kept the demo call (just made it non-fatal) to keep the diff minimal and preserve the original demo behavior, and flagged removal as a follow-up in the PR description. The extra startup cost is one failed DNS lookup per call site; happy to remove both calls (and the Quote/RestTemplate demo wiring) if the maintainer prefers — let me know and I'll do it in this PR.
… align H2 scope Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
There was a problem hiding this comment.
📝 Info: H2 2.x DDL compatibility relies on legacy syntax
The startup DDL uses DROP TABLE customers IF EXISTS (trailing IF EXISTS) and the non-standard SERIAL column type; both are legacy/compat forms that H2 2.x (pulled in by the 2.7 parent) still parses but which are not part of its documented grammar. The PR states boot was verified manually; if the H2 version ever moves again, this is the first place that will break. Rewriting as DROP TABLE IF EXISTS customers and id IDENTITY/BIGINT AUTO_INCREMENT would be more future-proof.
(Refers to lines 74-76)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — the existing DDL (DROP TABLE customers IF EXISTS, id SERIAL) parses and runs cleanly on the H2 2.x that the 2.7 parent pulls in (verified at startup: tables created, rows inserted and queried). I'm leaving it as-is to keep this PR a focused Java 17 / Spring Boot bump rather than rewriting demo SQL, but I've noted your future-proofing suggestion (DROP TABLE IF EXISTS customers + id IDENTITY) as a follow-up in the PR description.
| @@ -1,6 +1,6 @@ | |||
| #Thu Mar 01 09:01:15 CST 2018 | |||
| distributionBase=GRADLE_USER_HOME | |||
There was a problem hiding this comment.
📝 Info: Wrapper version differs from what the PR description states
The PR description says the Gradle wrapper was regenerated to 8.5, but the committed gradle/wrapper/gradle-wrapper.properties targets 7.6.4 (a later commit, Address review: ... Gradle wrapper to 7.6.4, changed it). The gradlew script and wrapper jar do look consistent with the 7.6.x wrapper (networkTimeout present, no validateDistributionUrl), so the build should work; only the description is stale. 7.6.4 is also the safer choice for the Spring Boot 2.7.18 Gradle plugin, which does not officially support Gradle 8.x.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Thanks — updated the PR description to reflect the wrapper being 7.6.4 (and the Maven wrapper bump to 3.9.9). And agreed, 7.6.4 is the safer choice inside the Boot 2.7 supported matrix, which is exactly why I switched off 8.5.
| dependencies { | ||
| compile("org.springframework.boot:spring-boot-starter-web") | ||
| testCompile("junit:junit") | ||
| runtimeOnly("org.springframework.boot:spring-boot-properties-migrator") |
There was a problem hiding this comment.
📝 Info: properties-migrator added to Gradle runtime as well
spring-boot-properties-migrator is now a runtime dependency in both builds. Per Spring's docs it is a temporary migration aid and should be removed once the upgrade is settled; leaving it in ships extra runtime property-scanning in the fat jar produced by bootJar. The description already lists removal as a follow-up.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — it's intentionally kept in both builds as a temporary migration aid for this bump (it surfaces renamed 2.x properties; it reported none for this app), and removal from both pom.xml and build.gradle is listed as a follow-up in the PR description.
✅ Runtime verification — Java 17 / Spring Boot 2.7.18 (all endpoints pass)Built with Startup on JDK 17 (the core of this upgrade)The dead external quote host logs a WARN but startup completes — the non-fatal try/catch works as intended. H2 DDL/insert/query all succeed. Endpoint checks (all HTTP 200)
Note (pre-existing, not a regression from this PR): Verified by Devin's testing agent. |
Summary
Upgrades this Spring Boot demo from Java 8 → Java 17 (LTS) and Spring Boot 2.0.2.RELEASE → 2.7.18 (last Java 17-compatible 2.x line, still
javax.*). Both the Maven and Gradle builds compile, and the app boots and serves all documented REST endpoints on JDK 17.Deliberately not moving to Spring Boot 3.x — the
javax.*→jakarta.*namespace migration is out of scope here and is a recommended follow-up (see Residual risks).pom.xmlspring-boot-starter-parent2.0.2.RELEASE→2.7.18.<java.version>1.8</java.version>→<java.version>17</java.version>. The 2.7 parent maps this tomaven.compiler.release=17, so the compiler validates against the Java 17 API (bytecode confirmed as class-file major version 61).<packaging>pom</packaging>→jar. This was a real bug: withpompackaging the Maven build never compiledsrc/main/javaor produced a runnable artifact.jaris clearly correct (there is asrc/main/java+spring-boot-maven-plugin), and the build/run/endpoints all pass with it.<scope>runtime</scope>to match Gradle'sruntimeOnly.maven-compiler-plugin(3.10.1) andmaven-surefire-plugin(2.22.2), both Java 17-safe.spring-boot-properties-migrator(runtime) kept as a temporary migration aid for surfacing renamed 2.x properties; it emitted no warnings for this app and should be removed after the bump.build.gradlespring-boot-gradle-plugin2.0.2.RELEASE→2.7.18(matches Maven).sourceCompatibility/targetCompatibility = 1.8→ Java toolchain targeting 17:java { toolchain { languageVersion = JavaLanguageVersion.of(17) } }compile→implementation,testCompile→testImplementation.bootJar { baseName / version }→archiveBaseName / archiveVersion(baseName/versionare removed in modern Gradle).spring-boot-starter-web, butApplication.javausesJdbcTemplate+ H2, so the Gradle build could not have compiled the sources. Addedspring-boot-starter-jdbc,com.h2database:h2(runtime) andspring-boot-properties-migrator(runtime) so both builds are consistent (same Spring Boot version, Java target, and dependencies).Gradle wrapper
gradle/wrapper/gradle-wrapper.properties4.6→7.6.4(Gradle < 7.3 cannot run on JDK 17; 7.6.4 stays inside the Spring Boot 2.7 supported matrix — Gradle 8.x is not officially supported by the 2.7 plugin). Regenerated via./gradlew wrapper --gradle-version 7.6.4, which also updatedgradlew,gradlew.batandgradle-wrapper.jar.Maven wrapper
.mvn/wrapper/maven-wrapper.properties3.3.9→3.9.9for consistency with the Gradle wrapper bump and to run on a modern-JDK-supported Maven (3.3.9 predates JDK 9 support; it happened to build green here on 17 but is an unsupported pairing).Application code
WebMvcConfigurerAdapter, no removed Spring MVC/JDBC APIs, noapplication.propertiesrenames, and H2 2.x (pulled in by the 2.7 parent) accepted the existing DDL (DROP TABLE ... IF EXISTS,CREATE TABLE ... id SERIAL).http://gturnquist-quoters.cfapps.io/api/random(Pivotal's shut-downcfapps.io) inside aCommandLineRunner, throwingUnknownHostExceptionand aborting boot. Wrapped both quote fetches in try/catch that logs a warning, so the demo feature is preserved but a dead external endpoint no longer prevents the app from starting. Minimal diff:Note:
target/(stale committed.classfiles) is tracked in the repo; this PR does not touch those build artifacts.Validation (JDK 17,
openjdk 17.0.13)Maven —
./mvnw clean verify(Apache Maven 3.9.9,Java version: 17.0.13):No unit tests exist in the repo (
src/testabsent), so there are no tests to run under Maven/Gradle; compilation + packaging + repackage succeed.Gradle —
./gradlew clean build(Gradle 7.6.4,JVM: 17.0.13):Runtime —
java -jar target/gs-spring-boot-0.1.0.jar:Endpoint checks with
curlon JDK 17:Residual risks / follow-ups
jakarta.*migration intentionally deferred — the recommended next step for a modern JDK 17 baseline.spring-boot-properties-migrator(from both builds) once the version bump is settled — it is only a temporary migration aid and adds runtime property-scanning to the fat jar.gturnquist-quoters.cfapps.ioquote service is gone; the call now fails gracefully. Consider removing it or pointing at a live quote source.DROP TABLE customers IF EXISTS,SERIAL); H2 2.x still parses them butDROP TABLE IF EXISTS customers+id IDENTITYwould be more future-proof.target/compiled classes are checked into the repo; consider gitignoringtarget/,build/,.gradle/in a separate cleanup.Link to Devin session: https://app.devin.ai/sessions/b941fb29ceab40319abef85352f738e6
Requested by: @tobydrinkall
Devin Review