-
Notifications
You must be signed in to change notification settings - Fork 29
Fix java 21 issues #657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix java 21 issues #657
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the project for Java 21 compatibility by upgrading build tooling and dependencies, removing deprecated code, and adjusting Spring Boot configurations.
- Upgrade Gradle to 8.5 and Spring Boot to 2.7.18, converting old
compile/providedRuntimesettings to the modernimplementation/compileOnly/testImplementationmodel. - Remove legacy Swagger 2 setup in favor of springdoc-openapi, update servlet initializer, application properties, and auto‐configuration exclusions.
- Introduce
NotificationTestConfigfor tests, adjust bean wiring inMappingConfiguration, and refine portlet controller imports and exception signatures.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| build.gradle, gradle/wrapper/gradle-wrapper.properties, gradle.properties | Upgraded to Gradle 8.5, Spring Boot 2.7.18, migrated dependency declarations to implementation. |
| notification-portlet-api/build.gradle | Converted old compile settings to implementation and added publication metadata. |
| SwaggerConfiguration.java | Removed legacy Swagger 2 configuration completely. |
| ServletInitializer.java | Updated import to org.springframework.boot.web.servlet.support.SpringBootServletInitializer. |
| NotificationApplication.java | Excluded JPA and DataSource auto‐configurations to avoid startup errors without a database. |
| application.properties | Set spring.main.web-application-type=servlet and disabled JTA for compatibility. |
| notification-portlet-webapp/src/test/java/org/jasig/portlet/notice/NotificationTestConfig.java | Added a standalone test configuration defining notification‐related beans. |
| notification-portlet-webapp/src/test/java/org/jasig/portlet/notice/NotificationApplicationTests.java | Allowed bean definition overriding and nested TestConfig to load custom test beans. |
| MappingConfiguration.java | Changed AddresseePostProcessor wiring from factory method to field injection. |
| SecurityConfiguration.java | Removed deprecated ErrorPageFilter setup and disabling registration. |
| NotificationLifecycleController.java | Consolidated portlet imports, updated exception signatures, and switched ModelAndView import. |
| ReadNotificationServiceDecorator.java | Simplified the boolean attribute list creation using List.of. |
Comments suppressed due to low confidence (5)
notification-portlet-webapp/src/main/java/org/jasig/portlet/notice/service/jpa/MappingConfiguration.java:44
- The
getAddresseePostProcessormethod is no longer annotated with@Bean, so Spring won't register it. Restore the@Beanannotation or register the bean elsewhere.
public AddresseePostProcessor getAddresseePostProcessor() {
notification-portlet-webapp/src/test/java/org/jasig/portlet/notice/NotificationApplicationTests.java:28
- You added
TestConfigbut didn’t specify it in@SpringBootTest. Includeclasses = TestConfig.classso beans fromNotificationTestConfigare loaded in the test context.
@SpringBootTest(properties = {"spring.main.allow-bean-definition-overriding=true"})
notification-portlet-webapp/src/test/java/org/jasig/portlet/notice/NotificationTestConfig.java:43
- The INotificationService type is used without an import. Add the appropriate import (e.g.,
import org.jasig.portlet.notice.service.INotificationService;) so the code compiles.
public INotificationService rootNotificationService() {
notification-portlet-webapp/src/main/java/org/jasig/portlet/notice/controller/NotificationLifecycleController.java:54
- Duplicate import of
ResourceRequestdetected afterimport javax.portlet.*;. Remove this redundant import to clean up the code.
import javax.portlet.ResourceRequest;
notification-portlet-webapp/src/main/java/org/jasig/portlet/notice/controller/NotificationLifecycleController.java:53
- Portlet controllers should return
org.springframework.web.portlet.ModelAndViewrather than the Spring MVCModelAndView. Update the import to the portlet version.
import org.springframework.web.servlet.ModelAndView;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the project for Java 21 compatibility, migrates to Spring Boot 2.7.x, and modernizes Gradle and portlet/JPA configurations.
- Upgrade toolchain: bump Gradle wrapper to 8.5, drop
.java-version, and test on Java 17/21 in CI - Migrate to Spring Boot 2.7: adjust
build.gradle, plugin IDs, dependency scopes, and auto-config excludes - Modernize portlet/JPA beans and controller setup: update imports, annotations, and remove deprecated Swagger2 config
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| notification-portlet-webapp/src/test/java/.../NotificationTestConfig.java | Add test bean definitions for notification services |
| notification-portlet-webapp/src/test/java/.../NotificationApplicationTests.java | Extend test config and allow bean overriding in tests |
| notification-portlet-webapp/src/main/resources/application.properties | Set servlet web type and disable JTA |
| notification-portlet-webapp/src/main/java/.../MappingConfiguration.java | Switch AddresseePostProcessor bean to use autowired instance |
| notification-portlet-webapp/src/main/java/.../SecurityConfiguration.java | Remove legacy ErrorPageFilter beans |
| notification-portlet-webapp/src/main/java/.../NotificationLifecycleController.java | Switch to @Controller, consolidate portlet imports |
| notification-portlet-webapp/src/main/java/.../action/read/ReadNotificationServiceDecorator.java | Simplify Boolean-to-string list initialization |
| notification-portlet-webapp/src/main/java/.../SwaggerConfiguration.java | Remove deprecated Swagger2 configuration |
| notification-portlet-webapp/src/main/java/.../ServletInitializer.java | Update SpringBootServletInitializer import |
| notification-portlet-webapp/src/main/java/.../NotificationApplication.java | Exclude JPA/DataSource auto-config after migration |
| notification-portlet-webapp/build.gradle | Upgrade Spring Boot plugin and migrate dependencies to implementation |
| notification-portlet-api/build.gradle | Update publishing DSL and dependency scopes |
| gradle/wrapper/gradle-wrapper.properties | Bump Gradle distribution to 8.5 |
| gradle.properties | Bump Spring and security versions |
| .github/workflows/CI.yml | Test on Java 17 and Java 21 instead of 8/11 |
Comments suppressed due to low confidence (2)
notification-portlet-webapp/src/main/java/org/jasig/portlet/notice/service/jpa/MappingConfiguration.java:44
- The method
getAddresseePostProcessor()no longer has a@Beanannotation, so theAddresseePostProcessorwon’t be registered as a Spring bean. Re-add@Bean("addresseePostProcessor")above this method to restore bean registration.
public AddresseePostProcessor getAddresseePostProcessor() {
notification-portlet-webapp/src/main/java/org/jasig/portlet/notice/controller/NotificationLifecycleController.java:53
- This imports
ModelAndViewfrom Spring MVC (web.servlet) instead of the portlet variant (org.springframework.web.portlet.ModelAndView). Update the import to use the portletModelAndViewso the@ResourceMappingmethod returns the correct type.
import org.springframework.web.servlet.ModelAndView;
| */ | ||
| compileOnly "${portletApiDependency}" | ||
| providedRuntime "${portletApiDependency}" | ||
| compileOnly "${portletApiDependency}" |
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The compileOnly "${portletApiDependency}" declaration appears twice (line 99 and 100). Remove the duplicate to avoid confusion and keep the dependency scope clear.
| compileOnly "${portletApiDependency}" |
No description provided.