Upgrade to Java 17 and Spring Boot 3.3.5 - #57
tobydrinkall wants to merge 3 commits into
Conversation
Bump Maven/Gradle compiler targets to 17, upgrade Spring Boot 2.0.2 -> 3.3.5, refresh Maven (3.9.9) and Gradle (8.10.2) wrappers, and modernise the deprecated JdbcTemplate.query args form. 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:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-parent</artifactId> | ||
| <version>2.0.2.RELEASE</version> | ||
| <version>3.3.5</version> |
There was a problem hiding this comment.
🔍 Boot 3 removes trailing-slash URL matching for existing endpoints
Spring Framework 6 / Boot 3 changed PathPatternParser to be the default and removed trailing-slash matching (setUseTrailingSlashMatch is deprecated/false). Endpoints declared in src/main/java/hello/controller/TopicController.java:21-81 and src/main/java/hello/controller/HelloController.java:46-87 will therefore no longer answer requests with a trailing slash (e.g. GET /topic/ now returns 404 whereas it worked under Boot 2.0). No code change is needed for the happy path, but any client or doc relying on trailing slashes will break silently after this upgrade.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate, and inherent to Boot 3 — Spring 6 dropped trailing-slash matching and the opt-back-in (setUseTrailingSlashMatch) is deprecated, so I'm not restoring it. Verified the non-slash happy path against the built jar: GET / -> {"id":1,"content":"Hello, World!"}, GET /topic and GET /topic/java return the expected JSON. The README documents all endpoints without trailing slashes, so no documented usage regresses.
| <groupId>org.springframework</groupId> | ||
| <artifactId>gs-spring-boot</artifactId> | ||
| <packaging>pom</packaging> | ||
| <packaging>jar</packaging> |
There was a problem hiding this comment.
📝 Info: Maven packaging switch to jar now actually compiles and packages sources
Changing packaging from pom to jar means the Maven build now compiles src/main/java and runs spring-boot-maven-plugin:repackage for the first time. Note that application.properties lives at the repository root rather than src/main/resources, so it is not packaged into the jar — the runtime picks it up only when the process's working directory is the repo root. It currently contains just a commented-out server.port, so behavior is unaffected today, but the file will be silently ignored for anyone running the produced jar from elsewhere.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and pre-existing — the root application.properties was already outside src/main/resources before this PR; the only difference is that a jar now actually gets produced. Its sole entry is a commented-out server.port, so nothing changes behaviourally. Leaving it as-is rather than widening this PR's scope; moving it into src/main/resources would be a sensible follow-up.
…enerate Maven wrapper Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
| # Find the actual extracted directory name (handles snapshots where filename != directory name) | ||
| actualDistributionDir="" | ||
|
|
||
| # For Cygwin, switch paths to Windows format before running java | ||
| if $cygwin; then | ||
| [ -n "$M2_HOME" ] && | ||
| M2_HOME=`cygpath --path --windows "$M2_HOME"` | ||
| [ -n "$JAVA_HOME" ] && | ||
| JAVA_HOME=`cygpath --path --windows "$JAVA_HOME"` | ||
| [ -n "$CLASSPATH" ] && | ||
| CLASSPATH=`cygpath --path --windows "$CLASSPATH"` | ||
| # First try the expected directory name (for regular distributions) | ||
| if [ -d "$TMP_DOWNLOAD_DIR/$distributionUrlNameMain" ]; then | ||
| if [ -f "$TMP_DOWNLOAD_DIR/$distributionUrlNameMain/bin/$MVN_CMD" ]; then | ||
| actualDistributionDir="$distributionUrlNameMain" | ||
| fi | ||
| fi | ||
|
|
||
| # traverses directory structure from process work directory to filesystem root | ||
| # first directory with .mvn subdirectory is considered project base directory | ||
| find_maven_basedir() { | ||
| local basedir=$(pwd) | ||
| local wdir=$(pwd) | ||
| while [ "$wdir" != '/' ] ; do | ||
| if [ -d "$wdir"/.mvn ] ; then | ||
| basedir=$wdir | ||
| break | ||
| # If not found, search for any directory with the Maven executable (for snapshots) | ||
| if [ -z "$actualDistributionDir" ]; then | ||
| # enable globbing to iterate over items | ||
| set +f | ||
| for dir in "$TMP_DOWNLOAD_DIR"/*; do | ||
| if [ -d "$dir" ]; then | ||
| if [ -f "$dir/bin/$MVN_CMD" ]; then | ||
| actualDistributionDir="$(basename "$dir")" | ||
| break | ||
| fi | ||
| fi | ||
| wdir=$(cd "$wdir/.."; pwd) | ||
| done | ||
| echo "${basedir}" | ||
| } | ||
|
|
||
| # concatenates all lines of a file | ||
| concat_lines() { | ||
| if [ -f "$1" ]; then | ||
| echo "$(tr -s '\n' ' ' < "$1")" | ||
| fi | ||
| } | ||
|
|
||
| export MAVEN_PROJECTBASEDIR=${MAVEN_BASEDIR:-$(find_maven_basedir)} | ||
| MAVEN_OPTS="$(concat_lines "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS" | ||
| set -f | ||
| fi | ||
|
|
||
| # Provide a "standardized" way to retrieve the CLI args that will | ||
| # work with both Windows and non-Windows executions. | ||
| MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $@" | ||
| export MAVEN_CMD_LINE_ARGS | ||
| if [ -z "$actualDistributionDir" ]; then | ||
| verbose "Contents of $TMP_DOWNLOAD_DIR:" | ||
| verbose "$(ls -la "$TMP_DOWNLOAD_DIR")" | ||
| die "Could not find Maven distribution directory in extracted archive" | ||
| fi |
There was a problem hiding this comment.
🔍 mvnw deviates from the upstream 3.3.4 script and skips an error check on the Java fallback download
The committed mvnw/mvnw.cmd include a non-upstream "find the actual extracted directory name" block, and the Java-based download fallback at the Downloader invocation has no || die ... guard (unlike the wget/curl branches), so a failed Java download surfaces later as a confusing "failed to unzip" message. Additionally the Unix script derives the cache directory name from a Java-style string hash while the PowerShell script uses a SHA-256 hex digest, so Windows and Unix users cache the same distribution under different directories. None of this breaks the build, but hand-modified wrapper scripts drift from what mvn wrapper:wrapper regenerates.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
mvnw/mvnw.cmd are not hand-modified — they're verbatim output of mvn wrapper:wrapper -Dmaven=3.9.9 (maven-wrapper-plugin 3.3.4, the version resolved by Maven 3.9.9), which is also where the distributionType=only-script switch and the deletion of maven-wrapper.jar come from. So the blocks you flagged (extracted-directory discovery, the un-guarded Java download fallback, the hash-vs-SHA256 cache-dir difference between the sh and cmd scripts) are upstream plugin behaviour; patching them here would be the actual drift, since the next wrapper:wrapper run would revert it. Better filed upstream against maven-wrapper-plugin if it's worth pursuing.
…, regenerate Gradle wrapper Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
| "SELECT id, first_name, last_name FROM customers WHERE first_name = ?", | ||
| (rs, rowNum) -> new Customer(rs.getLong("id"), rs.getString("first_name"), rs.getString("last_name")), | ||
| "Josh" |
There was a problem hiding this comment.
🔍 Legacy H2 DDL syntax is the main runtime risk of the H2 2.x jump
Boot 3.3.5 manages H2 2.x, and the startup runner still uses the legacy grammar DROP TABLE customers IF EXISTS (trailing IF EXISTS) plus the SERIAL column type. Both are legacy/compatibility forms that H2 2.x tightened up in places; the same pattern comes from the old Spring guide. The PR states this path was executed successfully on the new stack, so I did not flag it, but if the H2 version is later bumped or the compatibility mode changes, this DDL is the first thing that will break at startup (the runner failure would propagate out of SpringApplication.run).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed working, not just asserted: with Boot 3.3.5's managed H2 (2.2.224) the runner logs Creating tables, the four inserts, then Querying for customer records where first_name = 'Josh' with no SQL error — H2 2.x still accepts both DROP TABLE ... IF EXISTS and SERIAL. Leaving the guide's DDL as-is so this PR stays an upgrade rather than a rewrite; modernising it to DROP TABLE IF EXISTS customers / BIGINT AUTO_INCREMENT would be a clean follow-up.
| private static void logRandomQuote(RestTemplate restTemplate) { | ||
| try { | ||
| log.info(String.valueOf(restTemplate.getForObject(QUOTE_URL, Quote.class))); | ||
| } catch (RestClientException e) { | ||
| log.warn("Could not fetch a quote from {}: {}", QUOTE_URL, e.getMessage()); | ||
| } |
There was a problem hiding this comment.
📝 Info: Quote fetch failures are now silently downgraded to a warning
logRandomQuote swallows any RestClientException (including ResourceAccessException for the decommissioned gturnquist-quoters.cfapps.io host) and logs a warning. This is what makes java -jar runs survive, but it also means the URL being permanently dead is now invisible in normal operation, and String.valueOf(...) will log the literal null if the endpoint ever returns an empty body instead of surfacing a problem. Consider removing the dead call entirely as the description suggests.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Deliberate, and it's the lesser of two evils: before this the process died with UnknownHostException on every java -jar, which made the endpoints unreachable. The failure isn't silent — it's a WARN naming the URL and the cause on each of the two calls. I'd agree deleting the dead call is the better end state, but that removes a demo feature the repo advertises, so I've asked the author to confirm rather than deciding it inside an upgrade PR. String.valueOf is only there to avoid an NPE on a null body; getForObject returning null against a live quote service isn't a scenario reachable today.
| dependencies { | ||
| compile("org.springframework.boot:spring-boot-starter-web") | ||
| testCompile("junit:junit") | ||
| implementation 'org.springframework.boot:spring-boot-starter-web' | ||
| implementation 'org.springframework.boot:spring-boot-starter-jdbc' | ||
| runtimeOnly 'com.h2database:h2' |
There was a problem hiding this comment.
📝 Info: Gradle and Maven builds now agree on dependencies, but h2 scoping differs subtly
Maven declares h2 with runtime scope and Gradle uses runtimeOnly, which are equivalent for this app since no source touches H2 APIs. Note the Maven side previously had h2 at compile scope; narrowing it is safe here only because there are no H2 imports in src/. Similarly, no explicit test { useJUnitPlatform() } is needed in build.gradle because the Spring Boot Gradle plugin configures it, and there are currently no tests under src/test.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right on all three points. H2 narrowing is safe — rg 'org\.h2' src/ finds nothing, it's purely a JDBC-URL-driven runtime dependency. I'll leave test { useJUnitPlatform() } in place: it's redundant with the Boot plugin today but harmless, and it makes the intent explicit for whoever adds the first test.
1 participant
Summary
Moves this repo off Java 8 / Spring Boot 2.0.2 onto Java 17 (LTS) + Spring Boot 3.3.5, for both build systems.
pom.xml: parentspring-boot-starter-parent2.0.2.RELEASE→3.3.5,java.version1.8→17, and<packaging>pom</packaging>→jar. Thepompackaging meant Maven never compiledsrc/or produced an artifact at all — withjarthe compile +bootJarrepackage actually run, which is what makes the Java 17 target verifiable. Also droppedspring-boot-properties-migrator(a temporary Boot-2→3 aid with nothing to migrate here) and brought scopes in line with the Gradle build (H2runtime, addedspring-boot-starter-test).build.gradle: rewritten from the legacybuildscript{}/apply pluginform to theplugins {}block (Boot3.3.5, dependency-management1.1.6),sourceCompatibility/targetCompatibility1.8→17, and removed configurations Gradle 8 no longer has (compile/testCompile→implementation/runtimeOnly/testImplementation,bootJar.baseName/version→archiveBaseName/archiveVersion).3.3.9→3.9.9and Gradle4.6→8.10.2, both fully regenerated (wrapper:wrapper/gradlew wrapper), so the launcher scripts and wrapper jars match their distributions. Both old versions fail outright on a JDK 17 runtime, so this is required, not cosmetic.mvnw/gradlewwere also committed non-executable (100644); fixed.Application.java:JdbcTemplate.query(sql, Object[], RowMapper)is deprecated in Spring 6 and was the sole deprecation warning; switched to the varargs overload. Separately, the app fetched a random quote from the long-decommissionedgturnquist-quoters.cfapps.ioin two places and died withUnknownHostException; sincejarpackaging now makes that reachable, both call sites route through one guarded helper:No
javax.*→jakarta.*migration was needed: the app has no servlet/JPA/validation imports, so Boot 3 was a drop-in.Verification
mvn clean packageandgradlew buildboth succeed on JDK 17.0.13 with zero warnings.WARNinstead of a fatal error, the JDBCCommandLineRunnercreates/queries the H2customerstable, andGET /→{"id":1,"content":"Hello, World!"}plusGET /topic/GET /topic/javareturn the expected JSON.Behaviour change to be aware of: Spring 6 removed trailing-slash URL matching, so
GET /topic/now 404s where it worked on Boot 2. The README documents all endpoints without trailing slashes.Link to Devin session: https://app.devin.ai/sessions/ff425d53b0874e7da7fd26ebf17d067a
Requested by: @tobydrinkall
Devin Review