Migrate to Spring Boot 3.2 / Java 21 - #915
choikh0423 wants to merge 7 commits into
Conversation
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
🤖 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}
| /** HS512 requires a 512-bit key, so shorter secrets are expanded with SHA-512. */ | ||
| private static byte[] keyBytes(String secret) { | ||
| byte[] bytes = secret.getBytes(StandardCharsets.UTF_8); | ||
| if (bytes.length >= HS512_KEY_LENGTH) { | ||
| return bytes; | ||
| } | ||
| try { | ||
| return MessageDigest.getInstance("SHA-512").digest(bytes); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new IllegalStateException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Short secrets change both key material and signing algorithm
The configured jwt.secret in src/main/resources/application.properties:9 is 86 bytes, so it is used verbatim and existing tokens keep verifying with HS512. For an overridden secret shorter than 64 bytes, however, the change is larger than just "a different key": jjwt selects the MAC algorithm from key length, so e.g. a 60-byte secret previously signed with HS384 and will now sign with HS512 over a SHA-512-derived key. Also secret.getBytes() (platform default charset) became getBytes(UTF_8), which changes the key for non-ASCII secrets on non-UTF-8 platforms. Both only matter for deployments overriding the secret, but the release note should mention algorithm change, not just token invalidation.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right — the bundled 86-byte jwt.secret is unaffected, and the release-note caveat for deployments overriding it with a shorter value is now called out in the PR description.
| .permitAll() | ||
| .anyRequest() | ||
| .authenticated()) | ||
| .addFilterBefore(jwtTokenFilter(), UsernamePasswordAuthenticationFilter.class); |
There was a problem hiding this comment.
📝 Info: Filter bean is still double-registered in the servlet chain
jwtTokenFilter() is exposed as a @Bean and also added to the security chain (src/main/java/io/spring/api/security/WebSecurityConfig.java:62). Because it is a Filter bean, Spring Boot also auto-registers it as a plain servlet filter, so it runs twice per request (harmless but wasteful, and it means the filter also runs outside the security chain ordering). This is pre-existing behavior, but the migration would have been a natural point to declare it with a FilterRegistrationBean disabling auto-registration, or to instantiate it directly instead of as a bean.
Was this helpful? React with 👍 or 👎 to provide feedback.
| tasks.named('generateJava') { | ||
| schemaPaths = ["${projectDir}/src/main/resources/schema"] // List of directories containing schema files | ||
| packageName = 'io.spring.graphql' // The package name to use to generate sources | ||
| typeMapping = ["PageInfo": "graphql.relay.PageInfo"] |
There was a problem hiding this comment.
📝 Info: Codegen typeMapping to graphql.relay.PageInfo relies on schema compatibility
Mapping the schema's PageInfo to graphql.relay.PageInfo (build.gradle:71) avoids touching the data fetchers, but it hard-couples generated *Connection builders to graphql-java's relay class. If the GraphQL schema's PageInfo ever grows fields beyond hasNextPage/hasPreviousPage/startCursor/endCursor, codegen will silently produce a type that cannot express them. Worth a comment in build.gradle so a future schema change doesn't produce a confusing failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
| .path(handlerParameters.getPath()) | ||
| .extensions(errorsToMap(errors)) | ||
| .build(); | ||
| return DataFetcherExceptionHandlerResult.newResult().error(graphqlError).build(); | ||
| return CompletableFuture.completedFuture( | ||
| DataFetcherExceptionHandlerResult.newResult().error(graphqlError).build()); | ||
| } else { | ||
| return defaultHandler.onException(handlerParameters); | ||
| return defaultHandler.handleException(handlerParameters); |
There was a problem hiding this comment.
📝 Info: Exception handler results are completed synchronously
handleException builds the error and wraps it via CompletableFuture.completedFuture(...) on the calling thread (src/main/java/io/spring/graphql/exception/GraphQLCustomizeExceptionHandler.java:41-42 and :66-67), which preserves the previous synchronous semantics exactly; the delegation to defaultHandler.handleException at line 69 correctly returns the default handler's future rather than blocking on it. No behavioral difference expected.
(Refers to lines 32-69)
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
| /** HS512 requires a 512-bit key, so shorter secrets are expanded with SHA-512. */ | ||
| private static byte[] keyBytes(String secret) { | ||
| byte[] bytes = secret.getBytes(StandardCharsets.UTF_8); | ||
| if (bytes.length >= HS512_KEY_LENGTH) { | ||
| return bytes; | ||
| } | ||
| try { | ||
| return MessageDigest.getInstance("SHA-512").digest(bytes); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new IllegalStateException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Short JWT secrets now produce a different signing key, invalidating previously issued tokens
The new keyBytes helper hashes any secret shorter than 64 bytes with SHA-512. The bundled jwt.secret in src/main/resources/application.properties:9 is 86 bytes, so it is used verbatim and existing tokens keep verifying, as the PR description states. However, any deployment that overrides jwt.secret with a shorter value will now sign with a completely different key (and with HS512 rather than the previously inferred HS256/HS384), so all tokens issued before the upgrade will fail verification and users will be silently logged out. Worth calling out in release notes. Note also that SHA-512 stretching adds no entropy — a weak short secret remains weak, it merely satisfies jjwt's key-length check.
Was this helpful? React with 👍 or 👎 to provide feedback.
| tasks.named('generateJava') { | ||
| schemaPaths = ["${projectDir}/src/main/resources/schema"] // List of directories containing schema files | ||
| packageName = 'io.spring.graphql' // The package name to use to generate sources | ||
| typeMapping = ["PageInfo": "graphql.relay.PageInfo"] |
There was a problem hiding this comment.
📝 Info: PageInfo type mapping keeps the relay type but couples codegen to graphql-java internals
typeMapping = ["PageInfo": "graphql.relay.PageInfo"] prevents DGS codegen 6 from generating its own types.PageInfo, which keeps ArticleDatafetcher/CommentDatafetcher (which construct graphql.relay.DefaultPageInfo, see src/main/java/io/spring/graphql/ArticleDatafetcher.java:359-361) compiling unchanged. The trade-off is that the generated *Connection builders now depend on a graphql-java relay class whose shape (e.g. cursor accessors) is outside the schema's control; a future graphql-java bump could break codegen again. Migrating the data fetchers to the generated type would be the more durable fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed that migrating the data fetchers to the generated types.PageInfo is the more durable fix; keeping it out of this PR to hold the migration to minimal behavioral change, since the fetchers build graphql.relay.DefaultPageInfo from cursor pagers in several places.
| target project.fileTree(project.rootDir) { | ||
| include '**/*.java' | ||
| exclude 'build/generated/**/*.*', 'build/generated-examples/**/*.*' | ||
| include 'src/**/*.java' | ||
| } |
There was a problem hiding this comment.
📝 Info: Spotless no longer formats sources outside src/
Narrowing the target from **/*.java (minus build/generated) to src/**/*.java fixes the Gradle 8 implicit-dependency failure, but it also silently stops checking any Java outside src/ (e.g. buildSrc or future module directories). Given the repo currently only has src/, this is fine today; if modules are added later the formatting gate will quietly not cover them.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
| @Bean | ||
| public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | ||
| http.csrf(csrf -> csrf.disable()) | ||
| .cors(cors -> {}) | ||
| .exceptionHandling( | ||
| handling -> | ||
| handling.authenticationEntryPoint( | ||
| new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED))) | ||
| .sessionManagement( | ||
| management -> management.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) |
There was a problem hiding this comment.
📝 Info: Dropping WebSecurityConfigurerAdapter re-enables Spring Boot's default in-memory user
With WebSecurityConfigurerAdapter gone and no UserDetailsService/AuthenticationManager/AuthenticationProvider bean declared anywhere, UserDetailsServiceAutoConfiguration no longer backs off, so Boot creates an InMemoryUserDetailsManager with a random generated password printed at startup. Since neither form login nor HTTP Basic is enabled in the new filter chain, this is not exploitable, but it does add a surprising "Using generated security password" log line and an unused DaoAuthenticationProvider. Declaring an empty InMemoryUserDetailsManager (or a no-op AuthenticationManager) would suppress it.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — the startup log does show "Using generated security password". Since neither form login nor basic auth is enabled it's unreachable, so I'm leaving it out of this PR rather than adding a bean that has no counterpart in the Boot 2 config; happy to add an empty InMemoryUserDetailsManager if the repo owner wants the log line gone.
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
| .dispatcherTypeMatchers(DispatcherType.ERROR) | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.OPTIONS) | ||
| .permitAll() | ||
| .requestMatchers("/graphiql", "/graphiql/**") | ||
| .permitAll() |
There was a problem hiding this comment.
📝 Info: ASYNC dispatches are authorized in Spring Security 6 but not permitted here
The new chain permits DispatcherType.ERROR explicitly, which fixes the 401-instead-of-404 case, but Spring Boot's default spring.security.filter.dispatcher-types is REQUEST, ASYNC, ERROR, so AuthorizationFilter also runs on ASYNC dispatches. JwtTokenFilter extends OncePerRequestFilter (src/main/java/io/spring/api/security/JwtTokenFilter.java:19) whose shouldNotFilterAsyncDispatch() defaults to true, and with SessionCreationPolicy.STATELESS no context is restored, so any handler that goes async would be re-authorized as anonymous and rejected by anyRequest().authenticated(). Today no REST controller returns an async type and /graphql is permitAll, so nothing is broken; worth keeping in mind if DGS or a controller ever returns CompletableFuture/DeferredResult on a secured path.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The description was updated right after that commit and now describes the ERROR-only dispatcher rule plus the /graphiql/** path rule — the review job appears to have read the previous revision of the body. Agreed on the caveat that any other future internal forward to a non-permitted path would be re-authorized as anonymous; that's the intended tradeoff versus blanket-permitting FORWARD.
| .requestMatchers("/graphiql", "/graphiql/**") | ||
| .permitAll() |
There was a problem hiding this comment.
📝 Info: /graphiql/ permitAll is broader than the previous exact-path rule**
Boot 2 only permitted the exact path /graphiql; the new rule permits every path under /graphiql/. This is required for the forwarded /graphiql/index.html, but it also unauthenticates anything else DGS (or a future controller) maps under that prefix. Given GraphiQL only serves static assets there, impact is limited, but it is a wider surface than before.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate — DGS only serves the GraphiQL static assets under that prefix today, and the alternative (permitting all FORWARD dispatches) is wider still. Flagging as an accepted tradeoff rather than narrowing further to the single index.html, since the asset set is a DGS implementation detail.
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
| private static byte[] keyBytes(String secret) { | ||
| byte[] bytes = secret.getBytes(StandardCharsets.UTF_8); | ||
| if (bytes.length >= HS512_KEY_LENGTH) { | ||
| return bytes; | ||
| } | ||
| try { | ||
| return MessageDigest.getInstance("SHA-512").digest(bytes); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new IllegalStateException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Key derivation branch is untested and only the short-secret path is exercised by tests
keyBytes has two paths: verbatim bytes for secrets >= 64 bytes (the configured jwt.secret in src/main/resources/application.properties is 86 chars, so production takes this path) and a SHA-512 expansion for shorter ones (the 60-char test secret). Only the SHA-512 branch is covered by DefaultJwtServiceTest. Also note the switch from secret.getBytes() (platform default charset) to UTF-8 — identical for the current ASCII secret, but a non-ASCII secret deployed on a non-UTF-8 default charset would now derive a different signing key and invalidate existing tokens.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fair — the ≥64-byte (verbatim) branch is only exercised implicitly via the app context tests, which do use the 86-byte configured secret, so both paths run in the suite but only the short one is asserted directly. Not adding a dedicated unit test here since the branch is a migration workaround the repo owner may prefer to remove entirely (see the short-secret threads). The UTF-8 charset note applies only to non-ASCII secrets on a non-UTF-8 default charset; explicit UTF-8 seemed strictly better than platform-dependent bytes.
| JwtService expiredJwtService = new DefaultJwtService(SECRET, -3600); | ||
| String token = | ||
| "eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiJhaXNlbnNpeSIsImV4cCI6MTUwMjE2MTIwNH0.SJB-U60WzxLYNomqLo4G3v3LzFxJKuVrIud8D8Lz3-mgpo9pN1i7C8ikU_jQPJGm8HsC1CquGMI-rSuM7j6LDA"; | ||
| expiredJwtService.toToken(new User("email@email.com", "username", "123", "", "")); | ||
| Assertions.assertFalse(jwtService.getSubFromToken(token).isPresent()); |
There was a problem hiding this comment.
📝 Info: Expiry test now genuinely exercises expiration rather than signature failure
The previous hardcoded token was signed with an unrelated key, so should_get_null_with_expired_jwt actually passed on signature verification failure, not expiry. Generating a token with sessionTime = -3600 makes the assertion meaningful: the token verifies but parseSignedClaims throws ExpiredJwtException, which getSubFromToken's catch-all converts to Optional.empty(). Note the broad catch (Exception) still means the test cannot distinguish expiry from any other parse failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right on both counts. Tightening getSubFromToken's catch-all to distinguish ExpiredJwtException from other parse failures would change production behavior beyond the migration, so leaving that out of this PR.
Co-Authored-By: Kyu Choi <kyuhwanchoi0423@gmail.com>
| .dispatcherTypeMatchers(DispatcherType.ERROR) | ||
| .permitAll() |
There was a problem hiding this comment.
📝 Info: Blanket permitAll for ERROR dispatch also exposes container error pages
dispatcherTypeMatchers(DispatcherType.ERROR).permitAll() restores Boot 2 behavior for internal error forwards (Spring Security 6 authorizes every dispatcher type), but it permits all ERROR dispatches, including ones originating from otherwise-protected endpoints such as /user or /articles/feed. Since the error handling here (src/main/java/io/spring/api/exception/CustomizeExceptionHandler.java) only renders generic error payloads and the authentication entry point still runs for the initial REQUEST dispatch, this shouldn't leak protected data, but it is a broader rule than the /graphiql/** path rule chosen elsewhere for the same class of problem.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and intentional: the ERROR dispatch has no stable path to scope to (Boot forwards everything to /error), so permitting the dispatcher type is the only practical equivalent of Boot 2's behavior — whereas the GraphiQL forward does have a stable path, hence the narrower rule there. Verified the anonymous error body is Boot's default (timestamp/status/error/path, no message or trace).
| JwtService expiredJwtService = new DefaultJwtService(SECRET, -3600); | ||
| String token = | ||
| "eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiJhaXNlbnNpeSIsImV4cCI6MTUwMjE2MTIwNH0.SJB-U60WzxLYNomqLo4G3v3LzFxJKuVrIud8D8Lz3-mgpo9pN1i7C8ikU_jQPJGm8HsC1CquGMI-rSuM7j6LDA"; | ||
| expiredJwtService.toToken(new User("email@email.com", "username", "123", "", "")); |
There was a problem hiding this comment.
📝 Info: Expiry test now exercises a locally minted token rather than a fixed wire-format token
Replacing the hard-coded JWT with new DefaultJwtService(SECRET, -3600).toToken(...) keeps the expiry assertion meaningful (the parser rejects on exp), but it no longer pins the token wire format/signature, so a future change to key derivation or algorithm selection would silently keep this test green. A complementary test asserting that a token signed with a different key is rejected would cover the signature path that the old fixture incidentally covered.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The signature path is still covered by should_get_null_with_wrong_jwt (garbage token), though not by a well-formed token signed with a different key. Leaving the extra case out of this migration PR.
| target project.fileTree(project.rootDir) { | ||
| include '**/*.java' | ||
| exclude 'build/generated/**/*.*', 'build/generated-examples/**/*.*' | ||
| include 'src/**/*.java' | ||
| } |
There was a problem hiding this comment.
📝 Info: Spotless no longer excludes generated sources but now scans only src
Narrowing the target to src/**/*.java drops the explicit build/generated* excludes, which is fine today because DGS codegen writes into build/generated. If codegen output is ever configured under src/ (a common generateJava customization), spotless would start formatting/failing on generated files; keeping an explicit exclude alongside the narrowed include would be more robust.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Codegen writes to build/generated (the plugin default, unchanged here), so src/**/*.java can't pick it up today; re-adding a build/generated* exclude alongside the narrowed include would be dead config unless someone relocates codegen output under src/.
1 participant
Summary
Full Spring Boot 3 migration: Gradle 7.4 → 8.5 (wrapper scripts/jar regenerated via
./gradlew wrapper), Java 11 → 21 (README updated to match), Boot 2.6.3 → 3.2.5 (dependency-management 1.1.4), MyBatis starter 2.2.2 → 3.0.3, DGS starter 4.9.21 → 8.5.3 with codegen plugin 5.0.6 → 6.0.3, spotless 6.25.0, jjwt 0.12.5, rest-assured 5.4.0, sqlite-jdbc 3.45.3.0, joda-time 2.12.7../gradlew clean buildis green on JDK 21 (68 tests), and the endpoints below were smoke-tested against a running app.javax.validation/servlet/annotation→jakarta.*across 21 files (javax.cryptoinDefaultJwtServicestays — it's JDK, not EE).Non-mechanical parts worth reviewing:
WebSecurityConfig:WebSecurityConfigurerAdapteris gone, soconfigure(HttpSecurity)became aSecurityFilterChainbean using the lambda DSL,authorizeRequests/antMatchers→authorizeHttpRequests/requestMatchers. Same rules, entry point, stateless session policy, CSRF-off/CORS-on, andjwtTokenFilter()placement beforeUsernamePasswordAuthenticationFilter.Two rules differ from Boot 2 and exist to preserve behavior, because Spring Security 6's
AuthorizationFilterauthorizes every dispatcher type rather than just REQUEST, so internal dispatches were re-authorized as anonymous and fell through toanyRequest().authenticated():Without the ERROR rule,
GET /articles/unknown-slugreturned 401 instead of 404. Without/graphiql/**,/graphiqlreturned 401, because DGS'sGraphiQLConfigurerserves it as a view-controller forward to/graphiql/index.html(a path rule is used rather than permittingDispatcherType.FORWARD, so no other forward bypasses authorization). Neither is caught by the existing MockMvc tests. Verified on a running app:/graphiql200, anonymousPOST /graphql {tags}200,/tags200,/articles/nope404,/articles/feed401,/user401.GraphQLCustomizeExceptionHandler: graphql-java'sDataFetcherExceptionHandlernow only hasCompletableFuture<Result> handleException(params);onExceptionwas removed. Bodies unchanged, results wrapped inCompletableFuture.completedFuture(...).CustomizeExceptionHandler.handleMethodArgumentNotValid: Spring 6 passesHttpStatusCodeinstead ofHttpStatus.DefaultJwtService: jjwt 0.12 API (Jwts.parser().verifyWith(key).build().parseSignedClaims(token).getPayload(), buildersubject/expiration). It also now rejects HS512 keys shorter than 512 bits, which brokeDefaultJwtServiceTest's 60-byte secret, so short secrets are expanded:Secrets ≥ 64 bytes (including the configured
jwt.secret) are used verbatim, so existing tokens still verify. Release-note caveat for deployments overridingjwt.secretwith a shorter value: both the key material and the MAC algorithm change (jjwt previously derived HS256/HS384 from key length), so previously issued tokens stop verifying. The SHA-512 pass only satisfies jjwt's length check — it adds no entropy; rejecting short secrets outright is stricter but changes the configuration contract, so it's left for a separate decision.should_get_null_with_expired_jwtused a token hard-coded against the raw 60-byte key, which would now fail on signature rather than expiry, so it mints the expired token vianew DefaultJwtService(SECRET, -3600)instead.build.gradlecodegen: DGS codegen 6 generates its owntypes.PageInfoinstead of reusinggraphql.relay.PageInfo, which broke the*Connectionbuilders. Restored withtypeMapping = ["PageInfo": "graphql.relay.PageInfo"]rather than editing the data fetchers.spotless target narrowed to
src/**/*.java: with Gradle 8 the oldrootDirtree (excluding onlybuild/generated*) picked upbuild/outputs and failed validation with implicit-dependency errors oncompileJava/generateJava.Link to Devin session: https://app.devin.ai/sessions/8832d9d8bb6949b48e3598b9cf654919
Requested by: @choikh0423