feature: upgrade to Java 17 and Spring Boot 3.2.5 - #985
amitmanchella-cog wants to merge 3 commits into
Conversation
Co-Authored-By: Amit Manchella <amit.manchella@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:
|
Co-Authored-By: Amit Manchella <amit.manchella@cognition.ai>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @Bean | ||
| public JwtTokenFilter jwtTokenFilter() { |
There was a problem hiding this comment.
📝 Info: JwtTokenFilter bean is still auto-registered in the servlet chain
jwtTokenFilter() remains a @Bean of type Filter, so Spring Boot's servlet auto-registration adds it to the container filter chain in addition to the security chain (addFilterBefore). This was pre-existing, but under Spring Security 6 the chain is now built exclusively from this bean, making the double registration more visible: the filter will populate the SecurityContext for every request (including static/error dispatches) outside the security chain. Consider returning it as a plain new JwtTokenFilter() inside the chain or registering a FilterRegistrationBean with setEnabled(false).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pre-existing behavior that this upgrade doesn't change, and disabling the auto-registration would alter runtime behavior beyond the scope of the upgrade, so leaving it as-is. Worth a follow-up if the maintainers want the filter confined to the security chain.
| .requestMatchers(HttpMethod.OPTIONS, "/**") | ||
| .permitAll() | ||
| .requestMatchers("/graphiql", "/graphiql/**") | ||
| .permitAll() | ||
| .requestMatchers("/graphql") | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/articles/feed") | ||
| .authenticated() | ||
| .requestMatchers(HttpMethod.POST, "/users", "/users/login") | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/articles/**", "/profiles/**", "/tags") | ||
| .permitAll() |
There was a problem hiding this comment.
📝 Info: requestMatchers now resolve as MvcRequestMatcher instead of Ant matchers
Replacing antMatchers with requestMatchers changes matcher resolution: when Spring MVC is on the classpath the rules become MvcRequestMatchers evaluated against the path within the DispatcherServlet mapping. For paths served by static resource handling or by DGS/spring-graphql outside DispatcherServlet mappings (e.g. /graphiql assets), matching semantics can differ subtly from Ant matching. The added /graphiql/** rule covers the documented case, but if additional non-DispatcherServlet endpoints (actuator, static files) are added later they may not match as expected.
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: Delegating to DefaultDataFetcherExceptionHandler.handleException keeps async semantics
The migration from onException to handleException is consistent: both custom branches wrap results with CompletableFuture.completedFuture, and the fallback delegates to defaultHandler.handleException(...) rather than the deprecated onException, which in graphql-java 20+ throws UnsupportedOperationException by default. No blocking .join() was introduced, so the handler stays non-blocking.
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.
🔍 PageInfo type mapping relies on graphql.relay.PageInfo field coercion
Mapping the schema PageInfo to graphql.relay.PageInfo keeps the datafetchers compiling, but the schema declares endCursor/startCursor as String while graphql.relay.PageInfo exposes them as ConnectionCursor. Serialization works only because graphql-java's String coercing falls back to String.valueOf(...). This was the pre-upgrade behavior too, but it is worth verifying against the newer graphql-java version bundled with DGS 8, since stricter scalar coercion would surface as runtime serialization errors on paginated GraphQL queries.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified against the running app on the upgraded stack — Relay coercion still works with the graphql-java bundled by DGS 8.5.8:
{ articles(first:5) { pageInfo { hasNextPage hasPreviousPage startCursor endCursor } edges { cursor node { slug } } } }
-> {"pageInfo":{"hasNextPage":false,"hasPreviousPage":false,"startCursor":"1786387329360","endCursor":"1786386909054"}, ...}
Co-Authored-By: Amit Manchella <amit.manchella@cognition.ai>
| .dispatcherTypeMatchers(DispatcherType.ERROR) | ||
| .permitAll() |
There was a problem hiding this comment.
📝 Info: Permitting ERROR dispatch bypasses authorization for error responses
dispatcherTypeMatchers(DispatcherType.ERROR).permitAll() is required under Spring Security 6 (the security filter is registered for REQUEST, ASYNC and ERROR dispatch, and authorizeHttpRequests would otherwise re-authorize the /error forward and turn 404/422 into 401). Worth confirming that no sensitive content is rendered through the error dispatch path, since any error-dispatched request now skips authorization entirely; with the default BasicErrorController body this is fine.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: the app has no custom error controller or /error view, so the ERROR dispatch renders the default BasicErrorController body only.
| .requestMatchers(HttpMethod.OPTIONS, "/**") | ||
| .permitAll() | ||
| .requestMatchers("/graphiql", "/graphiql/**") | ||
| .permitAll() | ||
| .requestMatchers("/graphql") | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/articles/feed") | ||
| .authenticated() | ||
| .requestMatchers(HttpMethod.POST, "/users", "/users/login") | ||
| .permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/articles/**", "/profiles/**", "/tags") | ||
| .permitAll() | ||
| .anyRequest() | ||
| .authenticated()) |
There was a problem hiding this comment.
📝 Info: Security rule ordering and matcher semantics preserved in the DSL migration
I verified the rewritten chain is semantically equivalent to the old one: antMatchers(HttpMethod.OPTIONS) (null pattern = any path) maps to requestMatchers(HttpMethod.OPTIONS, "/**"), and the ordering /articles/feed (authenticated) before /articles/** (permitAll) is preserved, so the feed endpoint still requires auth. Note the newly added /graphiql/** permit rule widens the public surface to every GraphiQL sub-path, which is intended per the description but does expose the whole prefix.
Was this helpful? React with 👍 or 👎 to provide feedback.
| target project.fileTree(project.rootDir) { | ||
| include '**/*.java' | ||
| exclude 'build/generated/**/*.*', 'build/generated-examples/**/*.*' | ||
| exclude 'build/**', 'frontend/**' |
There was a problem hiding this comment.
📝 Info: Spotless target now skips all Java under build/ and frontend/
The exclude was broadened from the two generated-source directories to build/** and frontend/**. frontend/ currently contains no Java files, so the only practical effect is that DGS-generated sources under build/generated* are no longer formatted (as before) plus any future build outputs — no behavioral risk, but the frontend/** exclusion is currently a no-op.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right — frontend/** is defensive only. build/** is the load-bearing part: under Gradle 8 scanning build/ made spotlessJava consume processResources output and fail the implicit-dependency check.
1 participant
Summary
Modernizes the stack from Java 11 / Spring Boot 2.6.3 to Java 17 / Spring Boot 3.2.5.
Non-obvious pieces beyond the version bumps:
io.spring.graphql.types.PageInfo, incompatible with thegraphql.relay.PageInfothe datafetchers build. Instead of rewriting every datafetcher, the schema type is mapped back inbuild.gradle:DataFetcherExceptionHandleris now async:onException(params) -> DataFetcherExceptionHandlerResultbecamehandleException(params) -> CompletableFuture<...>, soGraphQLCustomizeExceptionHandlerwraps results inCompletableFuture.completedFuture(...)and delegates todefaultHandler.handleException(...).WebSecurityConfigno longer extendsWebSecurityConfigurerAdapter;configure(HttpSecurity)is replaced by aSecurityFilterChainbean using the lambda DSL,antMatchers→requestMatchers,authorizeRequests→authorizeHttpRequests. GraphiQL now serves sub-resources, so the permit rule covers/graphiql/**too (without it GraphiQL 401s on its assets).Other changes:
javax.validation.*/javax.servlet.*→jakarta.*across 20 files (javax.cryptoinDefaultJwtServicestays — JDK, not Jakarta EE).CustomizeExceptionHandler.handleMethodArgumentNotValidnow takesHttpStatusCodeinstead ofHttpStatus.fileTree(rootDir)) tripped the implicit-dependency check by scanningbuild/, so it now excludesbuild/**andfrontend/**.Verification
./gradlew clean buildpasses on JDK 17 (68 tests, 0 failures). Smoke-testedbootRun:GET /tags, user registration, authenticated article creation,GET /articles/feed→ 401 unauthenticated, validation errors → 422,POST /graphql{tags},/graphiql→ 200.Link to Devin session: https://app.devin.ai/sessions/eab1470696714c82a659d661972bb89d
Requested by: @amitmanchella-cog
Devin Review