[ES-2394] java21 migration Mosip identity plugin #177
Conversation
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
WalkthroughBump Java baseline to 21, update Maven plugins and dependency versions, migrate javax.* to jakarta.*, replace Spring Base64 utility with java.util.Base64, adjust KeyGenerator initialization to accept SecureRandom, and update tests to use a mocked KafkaTemplate. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/HelperService.java (1)
118-137: Key generation change is correct; consider reusing the existingsecureRandomUsing
KeyGeneratorUtils.getKeyGenerator(symmetricAlgorithm, symmetricKeyLength, new SecureRandom())is a good fit for Java 21 and the MOSIP crypto stack and should generate proper symmetric keys. Since this class already maintains a staticSecureRandom secureRandom, you could reuse it here (instead ofnew SecureRandom()) for consistency and to avoid repeated RNG instantiation:- KeyGenerator keyGenerator = KeyGeneratorUtils.getKeyGenerator(symmetricAlgorithm, symmetricKeyLength, new SecureRandom()); + KeyGenerator keyGenerator = KeyGeneratorUtils.getKeyGenerator(symmetricAlgorithm, symmetricKeyLength, secureRandom);mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/MockIdentityVerifierPluginImplTest.java (1)
10-12: Kafka async send mocking looks correct; minor cleanup possibleThe new Kafka-related scaffolding (mock
RecordMetadata/ProducerRecord, wrapping inSendResult, and returning a completedCompletableFuturefromkafkaTemplate.send(...)) matches the Spring Kafka 3.xKafkaTemplateAPI and gives good coverage by asserting two sends and the transactionId on the firstIdentityVerificationResult.Two minor cleanups you might consider:
resultTopicis set in both@Beforeand the test; you can rely on the@Beforesetup only.- Instead of manually setting the
kafkaTemplatefield viaReflectionTestUtils, you could declare it as@Mockand let@InjectMockswire it intoMockIdentityVerifierPluginImpl.These are non-blocking and purely for test clarity.
Also applies to: 24-25, 33-34, 72-83, 88-95
mosip-identity-plugin/pom.xml (1)
69-95: Java 21 / Spring Boot 3.2 upgrade looks consistent; align Spring Boot BOM and plugin versionsThe POM changes overall look coherent for the Java 21 and Spring Boot 3.2 migration:
java.version,maven.compiler.source/targetset to 21.- Maven plugin versions modernized (compiler, surefire, jacoco, etc.).
- Spring Cloud 2023.0.8 and Spring Boot moved to the 3.2.x line.
- MOSIP and biometrics dependencies updated to the expected 1.3.x beta/SNAPSHOT versions.
spring-boot-starter-testnow uses BOM-managed versions, which is good.One thing to tidy up:
spring.boot.versionis set to3.2.12(used e.g. forspring-boot-maven-plugin), while the imported BOMspring-boot-dependenciesis3.2.3. It’s generally safer to keep these on the same patch version to avoid subtle differences between plugin behavior and managed dependency versions.Consider updating one of them so both the BOM and the plugin use the same Spring Boot 3.2.x patch level.
Also applies to: 95-105, 126-130, 160-164, 229-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mosip-identity-plugin/pom.xml(4 hunks)mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/HelperService.java(1 hunks)mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImpl.java(1 hunks)mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/util/BiometricUtil.java(3 hunks)mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/MockIdentityVerifierPluginImplTest.java(5 hunks)
🔇 Additional comments (2)
mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImpl.java (1)
45-47: Jakarta@PostConstructimport is appropriate for Spring Boot 3 / Java 21The switch to
jakarta.annotation.PostConstructin this file is correct and consistent with the Jakarta namespace migration. The repo-wide verification confirmed a remainingjavax.annotation.PostConstructimport exists insunbird-rc-plugin/src/main/java/io/mosip/esignet/plugin/sunbirdrc/service/SunbirdRCAuthenticationService.java:41, which should be migrated separately as part of a broader cleanup effort.mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/util/BiometricUtil.java (1)
17-18: Unable to confirm the prior implementation – recommend manual verificationThe review comment assumes that prior code used Spring's
Base64Utils.encodeToUrlSafeString(), which would have delegated to Apache Commons Codec (no padding) or Java 8's Base64 (with padding). However:
- No
Base64Utilsimport exists anywhere in the codebase- No Apache Commons Codec dependency is declared in
pom.xml- Git history is unavailable to show what was actually replaced
- Current code uses only
java.util.Base64.getUrlEncoder(), which includes padding by defaultThe
.withoutPadding()suggestion is technically sound if the prior implementation used Apache Commons Codec (which omits padding). However, without confirming what was previously used, it's impossible to validate whether this change introduces a breaking difference in the Base64 encoding output.
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mosip-identity-plugin/pom.xml (1)
130-151: Extensive exclusions indicate potential upstream dependency compatibility issues.Both
biometrics-utiland especiallykernel-cbeffutil-apihave numerous exclusions to avoid transitive conflicts with Spring Boot 3.2.12 and provided dependencies. While these exclusions appear necessary for the migration, they suggest these upstream artifacts were built against older Spring Boot versions.Consider:
- Coordinating with the MOSIP kernel and biometric teams to release versions native to Spring Boot 3.x (without the old Spring Boot and spring-cloud transitive deps)
- This would reduce maintenance burden and exclusion management
- For now, these exclusions are acceptable but may indicate that the included versions need updates soon
Also applies to: 164-213
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mosip-identity-plugin/pom.xml(4 hunks)mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/HelperService.java(1 hunks)mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/MockIdentityVerifierPluginImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/HelperService.java
🔇 Additional comments (6)
mosip-identity-plugin/pom.xml (5)
70-70: Java 21 configuration looks correct.The compiler version and source/target alignment are properly configured for Java 21.
Also applies to: 74-76
85-86: Spring versions and dependencyManagement configuration is correct.The Spring Cloud version aligns with Spring Boot 3.x era, and the BOM import is properly configured.
Also applies to: 95-105
404-411: Add explicit surefire plugin version and verify the module system flag is still necessary.The plugin lacks an explicit
<version>tag, which may result in unpredictable version resolution. Additionally, verify if the--add-opens java.xml/jdk.xml.internal=ALL-UNNAMEDflag is still required with Java 21 + Spring Boot 3.2.12.Apply this diff to pin the surefire version:
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-surefire-plugin</artifactId> + <version>${maven.surefire.plugin.version}</version> <configuration> <skipTests>${skipTests}</skipTests>To verify the flag requirement, confirm:
- Whether Spring Boot 3.2.12 or XML processing libraries in the dependency tree require this JVM flag
- Check if any dependency uses
jdk.xml.internalclasses (e.g., Woodstox, StAX parsers)
223-227: Verify JUnit 4 is still the correct testing framework for Spring Boot 3.2.12.JUnit 4.13.1 is pinned explicitly, but Spring Boot 3.2.12 and
spring-boot-starter-teststrongly prefer JUnit 5 (Jupiter). Confirm whether:
- JUnit 4 is intentional (e.g., legacy test compatibility), or
- Tests should be migrated to JUnit 5 for full Spring Boot 3.x alignment
If JUnit 5 is preferred, consider removing the explicit junit 4 dependency and letting
spring-boot-starter-testmanage the test framework version.
88-90: Note: Pre-release dependency versions in use.The project depends on several beta and SNAPSHOT versions (
esignet 1.7.1-SNAPSHOT,esignet-signup 1.3.1-SNAPSHOT,kernel-keymanager-service 1.3.0-beta.4, etc.). These are appropriate for a migration branch but ensure they are stabilized before merging todevelopor production branches.Confirm:
- These pre-release versions are intentional for this migration PR
- A plan exists to pin to stable releases once Java 21/Spring Boot 3.2.12 compatibility is validated
mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/MockIdentityVerifierPluginImplTest.java (1)
42-46: Review comment is incorrect; MockIdentityVerifierPluginImpl does not have a KafkaTemplate fieldThe original review comment assumes that
MockIdentityVerifierPluginImplexposes aKafkaTemplatefield that@InjectMockswill wire. However, inspection of the actual class shows it has only four fields:storyName,configServerUrl,resourceLoader, andobjectMapper. There is noKafkaTemplatefield.Because the implementation has no
KafkaTemplatefield, the test's@InjectMockscannot inject the mockkafkaTemplateinto the target object. The mock is created in the test but remains unused and never wired intomockIdentityVerifierPlugin. The warnings in the original review comment about field ambiguity, silent wiring breakage, and injection fallibility are not applicable.If the test intends to mock Kafka interactions, the implementation class would need to actually declare and use a
KafkaTemplatefield first.Likely an incorrect or invalid review comment.
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mosip-identity-plugin/pom.xml (1)
206-210: Fix indentation on line 208.Line 208 has extra leading spaces before the
<version>tag. Align it with the surrounding tags for consistency.<dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-starter-test</artifactId> - <version>3.2.12</version> + <version>3.2.12</version> <scope>test</scope> </dependency>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mosip-identity-plugin/pom.xml(5 hunks)
🔇 Additional comments (4)
mosip-identity-plugin/pom.xml (4)
70-90: Java 21 and Spring framework upgrades are well-aligned.The Java 11→21 jump and corresponding Spring Boot 2.3.6→3.2.12 and Spring Cloud Hoxton.SR8→2023.0.8 upgrades are appropriate. Spring Boot 3.x mandates Jakarta EE (replacing javax namespaces), which aligns with the summary's mention of
javax→jakartamigration and the code changes in other files.Verify that the module version bumps (lines 88–90:
kernel-keymanager-serviceto 1.3.0-beta.4,esignetto 1.7.1-SNAPSHOT,esignet-signupto 1.3.1-SNAPSHOT) are available in your artifact repositories and that downstream consumers of this plugin are ready for these versions.
118-118: Verify stability of beta-versioned dependencies.Lines 118 and 152 introduce beta versions (
biometrics-utilandkernel-cbeffutil-apiboth 1.3.0-beta.1, andkernel-keymanager-serviceat 1.3.0-beta.4). Confirm that these beta versions are production-ready and have no breaking changes relative to the previous 1.2.0.x releases. Consider adding a note in the release notes or PR description about the beta dependency versions.Also applies to: 152-152
398-400: Surefire argLine correctly adapted for Java 21.The removal of
--illegal-access=permit(a deprecated Java 11 compatibility flag) and addition of--add-opens java.xml/jdk.xml.internal=ALL-UNNAMEDis the proper Java 21 approach for accessing internal APIs. This is consistent with the Java module system and necessary for XML parsing operations used in the codebase.
85-86: Spring Boot and Spring Cloud versions are compatible — no action required.Spring Cloud 2023.0.8 (Leyton) is compatible with Spring Boot 3.2.12; 2023.0.x supports Boot 3.2.x and 3.3.x. The pom.xml shows sound dependency management with proper exclusions (slf4j-api, spring-boot starters, Jackson variants) preventing transitive conflicts. Jackson version 2.15.0 is pinned consistently across dependencies.
Note: Spring Boot 3.2.12 marks the end of open-source support for the 3.2 line; plan to upgrade to 3.3.x or later.
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sunbird-rc-plugin/pom.xml (1)
160-165: mockito-core 5.20.0 is well-aligned with Java 21.The addition of mockito-core 5.20.0 (recent, Java 21 compatible) is a good testing practice. Consider in a future refactor upgrading from junit 4.13.1 to junit 5, which has better Java 21 support and annotation-based configuration.
sunbird-rc-plugin/src/main/java/io/mosip/esignet/plugin/sunbirdrc/service/SunbirdRCAuthenticationService.java (1)
267-267: Consider replacing deprecatedAPPLICATION_JSON_UTF8.While not part of this PR's changes,
MediaType.APPLICATION_JSON_UTF8is deprecated in Spring 5.2+ and removed in Spring 6.0+ (used by Spring Boot 3.x). Since this is a Java 21 migration, consider updating toMediaType.APPLICATION_JSON, which defaults to UTF-8.Apply this diff:
.post(UriComponentsBuilder.fromUriString(registrySearchUrl).build().toUri()) - .contentType(MediaType.APPLICATION_JSON_UTF8) + .contentType(MediaType.APPLICATION_JSON) .body(requestBody);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sunbird-rc-plugin/pom.xml(5 hunks)sunbird-rc-plugin/src/main/java/io/mosip/esignet/plugin/sunbirdrc/service/SunbirdRCAuthenticationService.java(1 hunks)
🔇 Additional comments (6)
sunbird-rc-plugin/pom.xml (5)
69-75: Java 21 migration baseline looks correct.All Java version properties are consistently set to 21, and maven-compiler-plugin 3.14.1 explicitly supports Java 21. The redundant source/target properties (both at the top level and plugin level) are harmless but could be consolidated in future refactors.
76-76: Plugin versions appropriately updated for Java 21 compatibility.maven-surefire-plugin 3.1.2, jacoco 0.8.14, and maven-javadoc-plugin 3.4.0 are all recent stable versions with explicit Java 21 support. Version pinning is good for build reproducibility.
Also applies to: 81-82, 292-292
83-84: Verify SNAPSHOT/beta dependency versions for Java 21 compatibility.The internal dependencies esignet-integration-api (1.7.1-SNAPSHOT) and kernel-keymanager-service (1.3.0-beta.4) are in SNAPSHOT/beta states. While this may be intentional for coordinated development, ensure they are confirmed as Java 21 compatible within your organization's testing environment.
140-140: Verify velocity 1.7 compatibility with Java 21 module access restrictions.The very old velocity 1.7 (released 2009) likely accesses internal JDK APIs, which is why
--add-opens java.xml/jdk.xml.internal=ALL-UNNAMEDis configured in the surefire argLine. While this workaround may work, velocity 1.7 is no longer maintained and may have security or compatibility issues with Java 21's stricter module system.Consider whether upgrading to a modern, Java 21-compatible velocity version (e.g., 2.x) is feasible within your migration scope. If velocity 1.7 must be retained, verify it works correctly under Java 21 with the --add-opens flag in production environments, not just tests.
Also applies to: 296-299
1-166: No explicit javax→jakarta migration visible in this pom.xml.The PR objectives mention migrating from javax.* to jakarta.* namespaces, but this pom.xml contains no direct javax or jakarta dependencies. This is likely correct for a lightweight plugin, but ensure that the internal dependencies (esignet-integration-api 1.7.1-SNAPSHOT, kernel-keymanager-service 1.3.0-beta.4) have been updated to Jakarta in their respective POMs if they previously used javax.* APIs.
sunbird-rc-plugin/src/main/java/io/mosip/esignet/plugin/sunbirdrc/service/SunbirdRCAuthenticationService.java (1)
41-44: Jakarta namespace migration is syntactically correct.The migration from
javax.*tojakarta.*annotations is properly executed. All affected annotations (@PostConstruct,@Valid,@NotBlank,@NotNull) are used throughout the class at lines 93 and 115-116.Ensure the project's Spring Boot version is 3.0 or higher, as Jakarta EE support is required for these imports to function at runtime.
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
3 participants
Summary by CodeRabbit
Chores
Bug Fixes / Compatibility
Tests
✏️ Tip: You can customize this high-level summary in your review settings.