feature: upgrade to Java 21 and Spring Boot 3.5 - #943
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
Migrate the app from Java 11/Spring Boot 2.6 to Java 21/Spring Boot 3.5.16: Jakarta namespace, Spring Security 6 SecurityFilterChain, MyBatis 3, DGS 10 on Spring for GraphQL, JJWT 0.12, Gradle 8.14.5. Adds GraphQL smoke tests and restores CI on JDK 21. Co-Authored-By: alex.vyshetsky <alex.vyshetsky@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}
| this.sessionTime = sessionTime; | ||
| signatureAlgorithm = SignatureAlgorithm.HS512; | ||
| this.signingKey = new SecretKeySpec(secret.getBytes(), signatureAlgorithm.getJcaName()); | ||
| this.signingKey = Keys.hmacShaKeyFor(secret.getBytes()); |
There was a problem hiding this comment.
🔍 JWT signing algorithm is now derived from secret length instead of being fixed at HS512
Keys.hmacShaKeyFor(secret.getBytes()) picks the JCA algorithm from the key's bit length (>=512 bits → HmacSHA512, 384-511 → HmacSHA384, 256-383 → HmacSHA256), whereas the old code forced HmacSHA512 regardless. With the shipped 88-byte jwt.secret in src/main/resources/application.properties:9 this still yields HS512, so production tokens keep verifying. However, any deployment that overrides jwt.secret with a 32-63 byte value will now mint HS256/HS384 tokens, and previously issued HS512 tokens for that key will fail verification (JJWT enforces key length against the header algorithm), silently logging every user out after deploy. Worth a note in upgrade docs if operators customize the secret. Note also getSubFromToken swallows all exceptions (src/main/java/io/spring/infrastructure/service/DefaultJwtService.java:42-44), so such failures are invisible.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Checked this against the old code and the algorithm selection is unchanged — JJWT 0.11's signWith(Key) also derived the algorithm from key length, not from the key's JCA name.
SignatureAlgorithm.forSigningKey(SecretKey) in 0.11.2 walks PREFERRED_HMAC_ALGS by bit length (requiring >= 256 bits) and ignores getAlgorithm(), so new SecretKeySpec(secret.getBytes(), "HmacSHA512") did not pin HS512. Ran the pre-upgrade code path against jjwt 0.11.2 to confirm:
480 bits -> header {"alg":"HS384"} # the 60-char secret in DefaultJwtServiceTest
688 bits -> header {"alg":"HS512"} # the shipped jwt.secret
So a deployment with a 32-47 byte secret was already minting HS256/HS384 before this PR; no tokens change validity across the upgrade. Pinning HS512 explicitly (signWith(key, Jwts.SIG.HS512)) would actually be a behavior change — it would start rejecting short secrets at startup, including the one the existing test uses.
| @Bean | ||
| public GraphQlHttpHandler graphQlHttpHandler(WebGraphQlHandler webGraphQlHandler) { | ||
| return new GraphQlHttpHandler( | ||
| webGraphQlHandler, new MappingJackson2HttpMessageConverter(new ObjectMapper())); | ||
| } |
There was a problem hiding this comment.
📝 Info: Only the HTTP GraphQL handler gets the plain ObjectMapper
The override replaces the auto-configured GraphQlHttpHandler so POST /graphql bodies bypass the app-wide UNWRAP_ROOT_VALUE setting. Note this scoping is partial: the auto-configured GraphQlSseHandler (and the WebSocket path, if ever enabled) still receives the application ObjectMapper and would hit the same Failed to read request failure. The schema currently declares no subscriptions (src/main/resources/schema/schema.graphqls), so this is inert today, but adding subscriptions later would reintroduce the bug. An alternative that avoids the trap is disabling UNWRAP_ROOT_VALUE globally and handling the REST envelope with @JsonRootName/explicit wrappers.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed that it's inert today — the schema declares no subscriptions, so GraphQlSseHandler is never exercised, and I'd rather not add an override for a path that has no coverage to prove it works.
On the global alternative: dropping UNWRAP_ROOT_VALUE and moving to @JsonRootName/wrapper types touches every REST param and response DTO, which is well outside a JDK/Boot upgrade. Left as-is; if subscriptions are ever added, the same converter should be passed to GraphQlSseHandler.
| @SpringBootTest( | ||
| properties = "spring.datasource.url=jdbc:sqlite:file:graphqlsmoketest?mode=memory&cache=shared") | ||
| @AutoConfigureGraphQlTester | ||
| public class GraphQLSmokeTest { |
There was a problem hiding this comment.
📝 Info: New Spring Boot tests bind to shared in-memory SQLite databases and share state across tests in a class
GraphQLSmokeTest and GraphQLHttpEndpointTest each start a full context against jdbc:sqlite:file:<name>?mode=memory&cache=shared. Because the named shared-cache DB lives for the JVM lifetime and the context is cached, should_execute_mutation_and_resolve_union_payload writes a user row that persists for other tests in the same JVM; re-running the same mutation (e.g. if the test is later parameterized or duplicated) would fail on the duplicated-username/email constraint. Also, both classes rely on the default profile, so they load the production application.properties (including dev.db overridden only by the inline property) rather than application-test.properties used elsewhere — slightly inconsistent with the existing test conventions.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The two classes use distinct database names (graphqlsmoketest vs graphqlhttptest) and neither re-runs the mutation, so the shared-cache lifetime isn't a problem today — but you're right that the createUser row persists for the JVM, so duplicating or parameterizing that test would trip the duplicated-username constraint.
On the profile: application-test.properties only sets spring.datasource.url=jdbc:sqlite::memory:, which is per-connection and so unusable for a full app context behind a Hikari pool — that's why these use an inline shared-cache URL instead of @ActiveProfiles("test"). The existing DbTestBase gets away with it because @MybatisTest uses a single connection.
| @Bean | ||
| public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | ||
| http.csrf(AbstractHttpConfigurer::disable) | ||
| .cors(withDefaults()) | ||
| .exceptionHandling( | ||
| handling -> | ||
| handling.authenticationEntryPoint( | ||
| new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED))) | ||
| .sessionManagement( | ||
| management -> management.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) | ||
| .authorizeHttpRequests( | ||
| requests -> | ||
| requests | ||
| .requestMatchers(HttpMethod.OPTIONS) | ||
| .permitAll() | ||
| .requestMatchers("/graphiql/**", "/graphql") | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/articles/feed") | ||
| .authenticated() | ||
| .requestMatchers(HttpMethod.POST, "/users", "/users/login") | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/articles/**", "/profiles/**", "/tags") | ||
| .permitAll() | ||
| .anyRequest() | ||
| .authenticated()) | ||
| .addFilterBefore(jwtTokenFilter(), UsernamePasswordAuthenticationFilter.class); | ||
| return http.build(); |
There was a problem hiding this comment.
📝 Info: Request-matcher semantics changed with the Spring Security 6 migration; checked for authorization regressions
I verified the rewritten rules preserve the old authorization outcomes: ordering is unchanged (feed authenticated() still precedes the /articles/** GET permitAll), /graphiql → /graphiql/** still matches the bare /graphiql path under PathPattern semantics, and /articles/** matches /articles itself under both AntPathMatcher and PathPattern. One residual pre-existing quirk: jwtTokenFilter() is exposed as a Filter bean, so Boot also registers it in the servlet container in addition to addFilterBefore; OncePerRequestFilter makes the second invocation a no-op, but declaring a FilterRegistrationBean with setEnabled(false) would make the intent explicit.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Thanks for the matcher-by-matcher check — that matches my reading, and the manual smoke run against a live instance confirmed the outcomes (GET /user 401 unauthenticated, /tags and POST /graphql open, feed authenticated).
Leaving the jwtTokenFilter double-registration alone: it predates this PR and OncePerRequestFilter makes it a no-op. Happy to add the FilterRegistrationBean(setEnabled(false)) if you'd like it explicit, but it's unrelated to the upgrade.
| this.signingKey = Keys.hmacShaKeyFor(secret.getBytes()); | ||
| } | ||
|
|
||
| @Override | ||
| public String toToken(User user) { | ||
| return Jwts.builder() | ||
| .setSubject(user.getId()) | ||
| .setExpiration(expireTimeFromNow()) | ||
| .subject(user.getId()) | ||
| .expiration(expireTimeFromNow()) | ||
| .signWith(signingKey) | ||
| .compact(); |
There was a problem hiding this comment.
🟨 JWT signing algorithm is now derived from secret length instead of being pinned to HS512
The signing key is now built with Keys.hmacShaKeyFor(secret.getBytes()) (src/main/java/io/spring/infrastructure/service/DefaultJwtService.java:25) and tokens are signed with signWith(signingKey) without an explicit algorithm, so the HMAC strength is implicitly chosen from the configured jwt.secret length rather than being fixed at HS512 as before. A deployment that configures a 32–47 byte secret will silently downgrade to HS256/HS384, and previously issued HS512 tokens signed with such a key will no longer verify (parsing fails and the exception is swallowed at src/main/java/io/spring/infrastructure/service/DefaultJwtService.java:42-44), logging all existing sessions out. The bundled 88-byte secret still yields HS512, so the default configuration is unaffected.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Checked this against the old code and the algorithm selection is unchanged — JJWT 0.11's signWith(Key) also derived the algorithm from key length, not from the key's JCA name.
SignatureAlgorithm.forSigningKey(SecretKey) in 0.11.2 walks PREFERRED_HMAC_ALGS by bit length (requiring >= 256 bits) and ignores getAlgorithm(), so new SecretKeySpec(secret.getBytes(), "HmacSHA512") did not pin HS512. Ran the pre-upgrade code path against jjwt 0.11.2 to confirm:
480 bits -> header {"alg":"HS384"} # the 60-char secret in DefaultJwtServiceTest
688 bits -> header {"alg":"HS512"} # the shipped jwt.secret
So a deployment with a 32-47 byte secret was already minting HS256/HS384 before this PR; no tokens change validity across the upgrade. Pinning HS512 explicitly (signWith(key, Jwts.SIG.HS512)) would actually be a behavior change — it would start rejecting short secrets at startup, including the one the existing test uses.
Summary
Java 21 can't run the current stack at all (Boot 2.6's Lombok crashes on JDK 21's
javac), so this is one atomic upgrade rather than a series: Java 11 → 21, Boot 2.6.3 → 3.5.16, Gradle 7.4 → 8.14.5, DGS 4.9 → 10.6 (now on Spring for GraphQL), MyBatis 2 → 3, JJWT 0.11 → 0.12. Behavior is unchanged: 68 existing tests plus 5 new GraphQL tests pass, and the REST + GraphQL endpoints were exercised by hand against a running app.Most of the diff is mechanical (
javax.validation/javax.servlet→jakarta.*in 21 files,WebSecurityConfigurerAdapter→ aSecurityFilterChainbean,HttpStatus→HttpStatusCodeinCustomizeExceptionHandler). The parts worth reading:GraphQL HTTP requests broke silently under the new DGS. DGS 4 parsed the request body with its own mapper; DGS 10 delegates to Spring for GraphQL, which uses the application
ObjectMapper— and this app setsspring.jackson.deserialization.UNWRAP_ROOT_VALUE=truefor the RealWorld REST envelope ({"user": {...}}). That made everyPOST /graphqlfail with400 Failed to read request, while all service-level tests still passed.GraphQLWebConfiggives the GraphQL handler its own plain mapper:GraphQLHttpEndpointTestposts a raw{"query": "{ tags }"}over HTTP so this can't regress unnoticed;GraphQLSmokeTestcovers a query, a union-typed mutation, and both branches of the exception handler.JWT signing key. JJWT 0.12 rejects
signWith(key)when the key's JCA name (HmacSHA512) outranks its length, which the 480-bit test secret hit.Keys.hmacShaKeyFor(secret.getBytes())derives the JCA algorithm from the key length instead, which is what 0.11 effectively did — the 704-bit production secret still yields HS512, so existing tokens keep verifying.Other non-obvious bits:
DataFetcherExceptionHandler.onException→CompletableFuture<...> handleException(graphql-java 24).PageInfo;typeMapping = ['PageInfo': 'graphql.relay.PageInfo']keeps the datafetchers on the relay type instead of rewriting them.rootDir/**/*.java, which under Gradle 8 is a hard error (undeclared dependency onbuild/); narrowed tosrc/**/*.java. This also fixes the pre-existingspotlessJavaCheckfailure onmaster.@MockBeanis removal-deprecated in Boot 3.5 →@MockitoBean.spring.graphql.graphiql.enabled=truepreserves/graphiql.CI was deleted in #411; restored as
.github/workflows/gradle.ymlon JDK 21, now running./gradlew clean build(build, spotless, tests) instead of justtest.Verification
./gradlew clean buildgreen on JDK 21 (73 tests).bootRun: register/login,GET /user, create article,GET /tags,POST /graphqlquery + mutation,/graphiql200, unauthenticatedGET /user401.Link to Devin session: https://app.devin.ai/sessions/24a66095075240a2930c3485eceee2d8