PII issues fixes with backward compatibility [MOSIP-44379]#1030
Draft
GOKULRAJ136 wants to merge 112 commits into
Draft
PII issues fixes with backward compatibility [MOSIP-44379]#1030GOKULRAJ136 wants to merge 112 commits into
GOKULRAJ136 wants to merge 112 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces a canonical user identity system for pre-registration that normalizes user identifiers to UUIDs, replacing plaintext identifiers in audit fields. It adds a new Changes
Sequence DiagramsequenceDiagram
participant AuthUser as Auth User Request
participant AppService as ApplicationService
participant UDS as UserDetailsService
participant UDRepo as UserDetailsRepository
participant AIM as ApplicationIdentityMigrationService
participant DB as Database
participant Cache as Cache
AuthUser->>AppService: addLostOrUpdateApplication(userId)
AppService->>UDS: getUserLookupIds(userId, piiBackCompat)
UDS->>UDS: normalizeIdentifier(userId)
UDS->>UDS: computeIdentifierHash()
UDS->>Cache: checkCache(hash)
alt Cache Hit
Cache-->>UDS: UserDetails UUID
else Cache Miss
UDS->>UDRepo: findByIdentifierHash(hash)
alt Not Found
UDS->>UDS: encryptIdentifier(userId)
UDS->>UDRepo: save(new UserDetails)
UDRepo->>DB: INSERT user_details
DB-->>UDRepo: Saved with UUID
else Found
UDRepo-->>UDS: Existing UserDetails
end
UDS->>Cache: cachePut(hash, UserDetails)
end
UDS-->>AppService: [canonicalUUID, ...]
AppService->>DB: findByCreatedByIn(lookupIds)
DB-->>AppService: ApplicationEntities
AppService->>AppService: Match authUser against entities
AppService->>AIM: migrateRawUserToEffectiveUser(preRegId, effectiveUserId)
AIM->>DB: findApplication(preRegId)
AIM->>DB: findDemographic(preRegId)
AIM->>DB: findDocuments(preRegId)
AIM->>DB: findBooking(preRegId)
AIM->>AIM: Update crBy, updBy, contactInfo
AIM->>DB: save(updatedEntities)
AIM-->>AppService: Migration complete
AppService-->>AuthUser: Success Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
GOKULRAJ136
force-pushed
the
MOSIP-PII-new
branch
from
2593635 to
fefa43c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java (1)
553-567:⚠️ Potential issue | 🟡 MinorAuthorization logic could allow unintended access with backward compatibility enabled.
When
piiBackwardCompatibilityis true andeffectiveCrBymatches the rawauthUserId, the method returns early (Line 563-564) without validating if the role check was intended. This could allow access if the raw user ID accidentally matches another user's canonical ID.🛡️ Proposed fix to make the logic explicit
if (!effectiveCrBy.equals(canonicalAuthUserId)) { - if (piiBackwardCompatibility && authUserId != null && effectiveCrBy.equals(authUserId.trim())) { - return; - } + // Backward compatibility: also accept raw identifier match + if (piiBackwardCompatibility && authUserId != null && !authUserId.trim().isEmpty() + && effectiveCrBy.equals(authUserId.trim())) { + log.debug(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, + "Authorization passed via backward compatibility for application: " + applicationEntity.getApplicationId()); + return; + } throw new PreIdInvalidForUserIdException(ApplicationErrorCodes.PRG_APP_015.getCode(), ApplicationErrorMessages.INVALID_APPLICATION_ID_FOR_USER.getMessage()); }
🧹 Nitpick comments (20)
pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java (1)
598-606: Limit helper class scope to this test class.
NullAuthoritycan be declaredprivate static finalsince it is only test scaffolding used internally.🔧 Suggested tweak
-static class NullAuthority implements GrantedAuthority { +private static final class NullAuthority implements GrantedAuthority {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java` around lines 598 - 606, The NullAuthority helper class should be made private static final to limit its scope to this test class; locate the static inner class named NullAuthority (implements GrantedAuthority) and change its declaration from package-private static to private static final so it remains test-scoped and immutable.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java (2)
269-297: Consider extracting to a shared utility class.The AI summary indicates similar masking utilities exist in
LoginService,AppointmentServiceImpl,DemographicService, and other classes. Centralizing this logic into a common utility would reduce duplication and ensure consistent masking behavior across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java` around lines 269 - 297, Extract the masking logic from LoginController.maskIdentifier into a shared utility (e.g., IdentifierMaskUtils.maskIdentifier) and replace duplicated implementations in LoginService, AppointmentServiceImpl, DemographicService, and other classes to call this single static utility method; ensure the utility preserves current behavior for emails, phone numbers (with optional '+'), UUIDs and fallback masking, keep null/blank handling, add unit tests for the utility, and update imports/usages across callers so all components use IdentifierMaskUtils.maskIdentifier.
290-294: CatchIllegalArgumentExceptioninstead ofException.
UUID.fromString()throwsIllegalArgumentExceptionfor invalid UUIDs. Catching the broadExceptiontype could mask unexpected errors.Suggested fix
try { UUID.fromString(trimmed); return "***" + trimmed.substring(trimmed.length() - 6); - } catch (Exception ignored) { + } catch (IllegalArgumentException ignored) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java` around lines 290 - 294, Replace the broad catch in LoginController's UUID masking logic with a catch for IllegalArgumentException only: when calling UUID.fromString(trimmed) inside the method that returns masked UUID (the block returning "***" + trimmed.substring(...)), change the catch(Exception ignored) to catch(IllegalArgumentException ignored) so only invalid-UUID errors are handled and other exceptions aren't accidentally suppressed; keep the existing handling (silently ignore or log) consistent with current behavior.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.java (1)
478-483: Remove unreachable small-length phone branch.Line 481-483 is unreachable because line 478 already constrains digits to
10..12. Removing it will simplify the method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.java` around lines 478 - 483, In the LoginService class, remove the unreachable small-length phone branch inside the trimmed.matches("\\+?\\d{10,12}") block: delete the inner if (digits.length() <= 4) { return (hasPlus ? "+" : "") + "****"; } since digits is already constrained to 10..12 by the regex; leave the remaining masking logic in this block intact so the method compiles and behavior is unchanged for valid 10–12 digit inputs.db_scripts/mosip_prereg/ddl/user_details.sql (2)
16-16: Remove duplicate index onidentifier_hash.Line 16 (
UNIQUE) already creates an index.idx_prereg_user_details_hashduplicates it and adds unnecessary write overhead.Proposed DDL cleanup
-CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_prereg_user_details_hash -ON prereg.user_details(identifier_hash);Also applies to: 25-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db_scripts/mosip_prereg/ddl/user_details.sql` at line 16, The schema defines identifier_hash VARCHAR(128) NOT NULL UNIQUE which already creates a unique index, so remove the redundant index creation for idx_prereg_user_details_hash (and any duplicate index statements at lines corresponding to the other occurrences mentioned) to avoid double indexing; update the DDL to drop or omit the CREATE INDEX/ADD INDEX statements that reference idx_prereg_user_details_hash while keeping the UNIQUE constraint on identifier_hash in the user_details table definition.
28-29: Reconsideridx_prereg_user_details_activekey choice.This partial index keys on
user_id, butuser_idis already primary-key indexed. Unless you have a proven query pattern that benefits from this exact predicate+key, this is likely redundant.Proposed DDL cleanup
-CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_prereg_user_details_active -ON prereg.user_details(user_id) WHERE identifier_encrypted IS NOT NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db_scripts/mosip_prereg/ddl/user_details.sql` around lines 28 - 29, The partial index idx_prereg_user_details_active on table user_details uses user_id as its key while filtering on identifier_encrypted IS NOT NULL, which is redundant because user_id is already primary-key indexed; either drop this index or change it to index the column(s) actually used by queries that include the predicate (for example index identifier_encrypted or the combination of identifier_encrypted plus any frequently queried columns), and update DDL to remove or replace CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_prereg_user_details_active with the appropriate index definition that matches real query patterns.pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/AvailibityEntity.java (1)
90-96: Naming inconsistency across entities.This entity uses
getEffectiveCrBy()/getEffectiveUpdBy(), whileInterfaceDataSyncEntityusesgetEffectiveCreatedBy()/getEffectiveUpdatedBy(). Consider standardizing the naming convention across all entities for consistency and easier maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/AvailibityEntity.java` around lines 90 - 96, The AvailibityEntity exposes getEffectiveCrBy() and getEffectiveUpdBy() which are inconsistent with InterfaceDataSyncEntity's getEffectiveCreatedBy() and getEffectiveUpdatedBy(); rename AvailibityEntity's methods to getEffectiveCreatedBy() and getEffectiveUpdatedBy(), update any internal field mappings (crBy -> createdBy/upBy -> updatedBy or keep fields but ensure getters map correctly), and search/replace all usages of getEffectiveCrBy() and getEffectiveUpdBy() to the new method names to maintain API consistency across entities.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DocumentServiceTest.java (1)
161-177: Consider extracting this setup block into a test helper.This setter chain is correct, but moving it into a small fixture/builder helper would reduce setup noise and future drift in this test class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DocumentServiceTest.java` around lines 161 - 177, Extract the repeated DocumentEntity setup in DocumentServiceTest into a test helper/builder to reduce noise: create a factory method (e.g., buildTestDocumentEntity or DocumentEntityBuilder) that encapsulates constructing DocumentEntity and setting fields used here (demographicEntity, documentId, docName, docCatCode, docTypeCode, docFileFormat, statusCode, langCode, crBy, crDtime, updBy, updDtime using DateUtils.parseDateToLocalDateTime(new Date()), encryptedDateTime, docId, docHash using HashUtill.hashUtill(cephBytes), refNumber) and replace the inline block in tests with a call to that helper; keep the helper in test sources so other tests can reuse it and update instantiations in DocumentServiceTest to use the new builder API.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.java (1)
144-155: Deduplicate canonical user resolution logic.The create/update branches repeat the same mapping/fallback block. Extracting a helper will keep behavior consistent and reduce maintenance risk.
Refactor sketch
+ private String resolveActorIdWithFallback() { + String fallback = environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID); + try { + UserDetails mappedUser = userDetailsService.findOrCreateByIdentifier(fallback); + if (mappedUser != null && mappedUser.getUserId() != null) { + return mappedUser.getUserId().toString(); + } + } catch (Exception e) { + logger.warn(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "UserDetails mapping failed", e); + } + return fallback; + } @@ - try { - UserDetails mappedUser = userDetailsService.findOrCreateByIdentifier(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - if (mappedUser != null && mappedUser.getUserId() != null) { - otpTxn.setUpdBy(mappedUser.getUserId().toString()); - } else { - otpTxn.setUpdBy(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - } - } catch (Exception e) { - logger.warn(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "UserDetails mapping failed for otp update", e); - otpTxn.setUpdBy(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - } + otpTxn.setUpdBy(resolveActorIdWithFallback()); @@ - try { - UserDetails mappedUser = userDetailsService.findOrCreateByIdentifier(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - if (mappedUser != null && mappedUser.getUserId() != null) { - txn.setCrBy(mappedUser.getUserId().toString()); - } else { - txn.setCrBy(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - } - } catch (Exception e) { - logger.warn(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "UserDetails mapping failed for otp create", e); - txn.setCrBy(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - } + txn.setCrBy(resolveActorIdWithFallback());Also applies to: 166-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.java` around lines 144 - 155, The create/update branches in OTPManager duplicate the canonical user resolution logic: extract a private helper method (e.g., resolveCanonicalUpdater) that calls userDetailsService.findOrCreateByIdentifier(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)), sets the updater value using otpTxn.setUpdBy(...) with the mappedUser.getUserId().toString() fallback to the raw property, and logs failures via logger.warn(...) with the exception; replace the duplicated blocks in both create and update paths with a call to this helper to keep behavior consistent and reduce maintenance.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java (1)
493-521: Consider centralizingmaskIdentifierin a shared utility.The masking logic is repeated in multiple services; consolidating it reduces drift and keeps masking policy consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java` around lines 493 - 521, The maskIdentifier implementation in AppointmentServiceImpl is duplicated across services; extract this logic into a single shared utility (e.g., IdentifierMasker.maskIdentifier or MaskUtils.maskIdentifier) in a common/shared module, make the method public static, move the exact logic from AppointmentServiceImpl.maskIdentifier into that utility, update AppointmentServiceImpl and other services to call the new utility method instead of their local copies, and update/merge unit tests to reference the centralized method and remove duplicated implementations to keep masking policy consistent.pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java (1)
39-84: Add edge-case tests for ciphertext encoding and blank identifiers.Current tests use text-like encrypted bytes only and don’t cover whitespace identifier rejection. Adding both cases will protect the critical paths introduced in this PR.
🧪 Suggested test additions
+ `@Test` + public void testFindOrCreateRejectsBlankIdentifier() { + org.junit.jupiter.api.Assertions.assertThrows(IllegalArgumentException.class, + () -> userDetailsService.findOrCreateByIdentifier(" ")); + } + + `@Test` + public void testEncryptDecryptWithBinaryCiphertextRoundTrip() { + when(userDetailsRepository.findByIdentifierHash(any())).thenReturn(Optional.empty()); + byte[] binaryCipher = new byte[] {(byte)0xFF, (byte)0x00, (byte)0xA5, (byte)0x7F}; + when(cryptoUtil.encrypt(any(), any())).thenReturn(binaryCipher); + when(cryptoUtil.decrypt(any(), any())).thenReturn("plain".getBytes(StandardCharsets.UTF_8)); + when(userDetailsRepository.save(any())).thenAnswer(i -> i.getArgument(0)); + + UserDetails saved = userDetailsService.findOrCreateByIdentifier("user1"); + when(userDetailsRepository.findById(saved.getUserId())).thenReturn(Optional.of(saved)); + Optional<String> decrypted = userDetailsService.getDecryptedIdentifier(saved.getUserId()); + + assertTrue(decrypted.isPresent()); + assertEquals("plain", decrypted.get()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java` around lines 39 - 84, Add two edge-case unit tests in UserDetailsServiceTest: one that verifies findOrCreateByIdentifier and getDecryptedIdentifier handle non-text/binary ciphertext bytes by mocking cryptoUtil.encrypt to return non-UTF8 bytes (e.g., arbitrary byte array) and asserting decryption and save/lookup still behave correctly; and another that verifies findOrCreateByIdentifier rejects blank/whitespace identifiers by calling userDetailsService.findOrCreateByIdentifier with blank inputs and asserting no save on userDetailsRepository and that an appropriate empty/exceptional result is returned. Use existing mocks (userDetailsRepository, cryptoUtil) and methods (findOrCreateByIdentifier, getDecryptedIdentifier, findById) to set up repository returns and verify interactions.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java (1)
292-297: Remove redundantcrBy/updByassignments indocumentEntitySetter.Line 292-293 are immediately overwritten by Line 295-296, which adds noise without effect.
♻️ Proposed cleanup
- copyDocumentEntity.setCrBy(sourceEntity.getCrBy()); - copyDocumentEntity.setUpdBy(sourceEntity.getUpdBy()); // copy canonical user references if present copyDocumentEntity.setCrBy(sourceEntity.getEffectiveCrBy()); copyDocumentEntity.setUpdBy(sourceEntity.getEffectiveUpdBy());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java` around lines 292 - 297, The documentEntitySetter contains redundant assignments: remove the initial copyDocumentEntity.setCrBy(sourceEntity.getCrBy()) and copyDocumentEntity.setUpdBy(sourceEntity.getUpdBy()) since they are immediately overwritten by copyDocumentEntity.setCrBy(sourceEntity.getEffectiveCrBy()) and copyDocumentEntity.setUpdBy(sourceEntity.getEffectiveUpdBy()); keep only the effective-crBy/updBy assignments (and the existing setLangCode) so the method uses sourceEntity.getEffectiveCrBy()/getEffectiveUpdBy() and avoid the duplicate setCrBy/setUpdBy calls.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.java (1)
88-90: Please add explicit tests for canonical mapping behavior.Line 88 adds
UserDetailsServicemocking, but the suite still doesn’t assert the new canonicalcrBy/updBypath (success and mapping-failure fallback cases) indtoToEntity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.java` around lines 88 - 90, The test suite needs explicit unit tests verifying the new canonical mapping path in dtoToEntity: add two tests in DocumentServiceUtilTest that exercise dtoToEntity using the mocked UserDetailsService—(1) a success case where UserDetailsService returns a UserDetails with the canonical identifier and assert the resulting entity fields crBy and updBy are set to that canonical value, and (2) a failure/fallback case where UserDetailsService returns null or throws and assert dtoToEntity falls back to the previous behavior (e.g., uses the original dto values or a default) for crBy/updBy; locate the call to dtoToEntity and the UserDetailsService mock in the test class and update assertions accordingly to validate both canonical mapping and fallback.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java (3)
978-988:getUserLookupIdsis duplicated from ApplicationService.This method is identical to the one in
ApplicationService. All three helper methods should be centralized.Would you like me to generate a utility class that consolidates these three duplicated methods (
resolveCanonicalUserId,maskIdentifier,getUserLookupIds)? This would improve maintainability and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java` around lines 978 - 988, The getUserLookupIds method is duplicated from ApplicationService along with resolveCanonicalUserId and maskIdentifier; consolidate these three helpers into a single utility class (e.g., UserIdUtils) containing resolveCanonicalUserId, maskIdentifier, and getUserLookupIds as static methods, move the shared logic there, update DemographicService and ApplicationService to call UserIdUtils.resolveCanonicalUserId, UserIdUtils.maskIdentifier, and UserIdUtils.getUserLookupIds (removing the local copies), and ensure visibility, null-handling and piiBackwardCompatibility behavior are preserved and covered by existing tests or add small unit tests for the new utility methods.
948-976:maskIdentifieris duplicated from ApplicationService.This method is identical to the one in
ApplicationService. Extract to a shared utility such asUserDetailsServiceor a newPiiMaskingUtilclass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java` around lines 948 - 976, The maskIdentifier method in DemographicService is a duplicate of the one in ApplicationService; extract the logic into a shared utility class (e.g., PiiMaskingUtil) or an existing shared service (e.g., UserDetailsService) and replace the local method with a call to that utility. Create a public static maskIdentifier(String) in PiiMaskingUtil (or a public method on UserDetailsService), move the masking logic there, update DemographicService to call PiiMaskingUtil.maskIdentifier(value) (or userDetailsService.maskIdentifier(value)), and remove the duplicate private maskIdentifier from DemographicService; ensure imports and any tests/refactors reference the single shared implementation.
932-946:resolveCanonicalUserIdis duplicated from ApplicationService.This method is identical to the one in
ApplicationService. Extract to a shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java` around lines 932 - 946, The resolveCanonicalUserId method is duplicated in DemographicService and ApplicationService; extract it into a shared utility (e.g., a new CanonicalUserUtil or UserIdMapper) and have both classes call that utility. Move the logic that uses userDetailsService.findOrCreateByIdentifier(...) and maskIdentifier(...) into the utility, or alternatively keep the dependency on userDetailsService in the callers and expose a utility method like mapToCanonicalUserId(UserDetailsService, String) that performs the try/catch and logging; update DemographicService.resolveCanonicalUserId and the one in ApplicationService to delegate to the new utility, adjust imports, ensure the logger and maskIdentifier are accessible (or pass a masking function/logger into the utility), and run tests to verify no behavioral changes.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java (4)
669-679:getUserLookupIdsmethod duplicated across services.The
getUserLookupIdsmethod is duplicated. Consider moving all three helper methods (resolveCanonicalUserId,maskIdentifier,getUserLookupIds) to a shared utility class likeUserDetailsServiceor a newUserIdUtilsclass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java` around lines 669 - 679, The getUserLookupIds duplication should be removed from ApplicationService and the three helper methods (resolveCanonicalUserId, maskIdentifier, getUserLookupIds) should be extracted into a shared utility class (e.g., UserIdUtils or add to existing UserDetailsService); create the new class with those methods as public static (or instance) helpers, update ApplicationService to call UserIdUtils.resolveCanonicalUserId(...) and UserIdUtils.getUserLookupIds(...) (or inject UserDetailsService and call the moved methods), and remove the duplicate implementations from other services so all callers reference the single shared implementation.
436-444: Indentation is inconsistent and logic is hard to follow.The authorization check block has inconsistent indentation making the logic flow difficult to understand. The nested conditionals with negation patterns could lead to maintenance issues.
♻️ Suggested refactor for clarity
- String authUserId = authUserDetails().getUserId(); - String canonicalAuthUserId = resolveCanonicalUserId(authUserId); - String effectiveCrBy = applicationEntity.getEffectiveCrBy() == null ? "" : applicationEntity.getEffectiveCrBy().trim(); - if (!effectiveCrBy.equals(canonicalAuthUserId)) { - if (!(piiBackwardCompatibility && effectiveCrBy.equals(authUserId == null ? "" : authUserId.trim()))) { - throw new PreIdInvalidForUserIdException(ApplicationErrorCodes.PRG_APP_015.getCode(), - ApplicationErrorMessages.INVALID_APPLICATION_ID_FOR_USER.getMessage()); - } - } + String authUserId = authUserDetails().getUserId(); + String canonicalAuthUserId = resolveCanonicalUserId(authUserId); + String effectiveCrBy = applicationEntity.getEffectiveCrBy() == null ? "" : applicationEntity.getEffectiveCrBy().trim(); + boolean matchesCanonical = effectiveCrBy.equals(canonicalAuthUserId); + boolean matchesRaw = piiBackwardCompatibility && + effectiveCrBy.equals(authUserId == null ? "" : authUserId.trim()); + if (!matchesCanonical && !matchesRaw) { + throw new PreIdInvalidForUserIdException(ApplicationErrorCodes.PRG_APP_015.getCode(), + ApplicationErrorMessages.INVALID_APPLICATION_ID_FOR_USER.getMessage()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java` around lines 436 - 444, Refactor the authorization check in ApplicationService by normalizing values first (call authUserDetails().getUserId(), resolveCanonicalUserId(authUserId), and trim applicationEntity.getEffectiveCrBy() into a local effectiveCrBy variable) and then replace the nested negated condition with a clear sequence: if effectiveCrBy equals canonicalAuthUserId -> allow; else if piiBackwardCompatibility && effectiveCrBy equals trimmed authUserId -> allow; else throw PreIdInvalidForUserIdException using ApplicationErrorCodes.PRG_APP_015 and ApplicationErrorMessages.INVALID_APPLICATION_ID_FOR_USER. Ensure consistent indentation and remove double-negation to improve readability around authUserDetails(), resolveCanonicalUserId(), effectiveCrBy, piiBackwardCompatibility, and the exception throw.
622-637: ExtractresolveCanonicalUserIdto a shared utility class.This method is duplicated identically in both
ApplicationServiceandDemographicService. Extract it to a shared utility class to avoid duplication and ensure consistent behavior across both services.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java` around lines 622 - 637, The resolveCanonicalUserId method is duplicated in ApplicationService and DemographicService; extract it into a shared utility (e.g., IdentityUtils or UserIdResolver) and replace both copies with calls to that utility. Move the logic that checks null/empty, calls userDetailsService.findOrCreateByIdentifier(userId), trims mappedUser.getUserId(), and returns "" on failure into a single static or instance method (e.g., IdentityUtils.resolveCanonicalUserId(userId, userDetailsService, logger)), preserve the existing logging call (include maskIdentifier) by either accepting a Logger and maskIdentifier helper or keeping maskIdentifier as a shared utility, and update ApplicationService and DemographicService to delegate to the new utility (inject userDetailsService or pass it as an argument) so behavior and logging remain identical.
639-667: Extensive duplication ofmaskIdentifiermethod across 7 files.The identical
maskIdentifierimplementation is duplicated across ApplicationService, DemographicService, LoginService, AppointmentServiceImpl, CommonServiceUtil, DataSyncServiceUtil, and LoginController. This utility method should be centralized in a shared location (e.g., as a public static method in CommonServiceUtil) to eliminate code duplication across services and modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java` around lines 639 - 667, The maskIdentifier method is duplicated across multiple classes; extract it to a single shared utility and replace duplicates with calls to that utility. Create a public static String maskIdentifier(String) in CommonServiceUtil with the exact logic currently in ApplicationService.maskIdentifier, then remove the duplicate private methods from ApplicationService, DemographicService, LoginService, AppointmentServiceImpl, DataSyncServiceUtil, and LoginController and update those classes to call CommonServiceUtil.maskIdentifier(value) (adding imports as needed); run compilation/tests to ensure signatures and visibility match and behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Around line 17-20: The column pair identifier_encrypted and encrypted_dtimes
in the user_details DDL are currently inconsistent (identifier_encrypted
nullable but encrypted_dtimes NOT NULL); add a constraint so their presence is
paired: either make both nullable or add a CHECK constraint on the table (e.g.,
CHECK ((identifier_encrypted IS NULL AND encrypted_dtimes IS NULL) OR
(identifier_encrypted IS NOT NULL AND encrypted_dtimes IS NOT NULL))) to enforce
that identifier_encrypted and encrypted_dtimes are NULL or NOT NULL together,
referencing the identifier_encrypted and encrypted_dtimes columns in the
user_details table definition.
- Around line 25-29: Add db_scripts/mosip_prereg/ddl/user_details.sql to the
migration orchestration by adding an include/import entry for that file in
db_scripts/mosip_prereg/ddl.sql so the file is deployed, and edit
user_details.sql to remove the CONCURRENTLY keyword from the index statements:
replace the two "CREATE INDEX CONCURRENTLY IF NOT EXISTS
idx_prereg_user_details_hash" and "CREATE INDEX CONCURRENTLY IF NOT EXISTS
idx_prereg_user_details_active" statements with "CREATE INDEX IF NOT EXISTS"
versions (keeping the same index names idx_prereg_user_details_hash and
idx_prereg_user_details_active and their ON clauses) so the DDL runs
transactionally under the orchestration.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/ApplicationRepostiory.java`:
- Around line 41-45: Guard against null or empty userIds before invoking the
repository methods in the service/caller that use
ApplicationRepostiory.findByCreatedByIn(...) and
ApplicationRepostiory.findByCreatedByInBookingType(...): if userIds == null ||
userIds.isEmpty() return Collections.emptyList() (or an equivalent empty result)
instead of calling the repository, so you never bind an empty collection to the
JPQL IN parameter; apply the same check before both findByCreatedByIn and
findByCreatedByInBookingType calls.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java`:
- Around line 473-483: The code currently sets
applicationEntity.setUpdBy(authUserDetails().getUserId()) before attempting
canonicalization, so if userDetailsService.findOrCreateByIdentifier(...) fails
the raw auth ID is persisted; change the flow to not set updBy from
authUserDetails() up-front: remove that initial setUpdBy call and only call
applicationEntity.setUpdBy(mappedUser.getUserId().toString()) after a successful
mapping (i.e., when mappedUser != null && mappedUser.getUserId() != null); in
the catch block do not revert to or persist the raw auth ID (leave updBy null or
unchanged) and keep the existing warn log in userDetails mapping failure
handling.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java`:
- Around line 880-930: The userValidation method's flow misuses
userDetailsService.findOrCreateByIdentifier (in the mappedPreregUser lookup)
which may create unintended user entries and the fallback raw ID check runs even
when the canonical lookup succeeded but didn't match; change the prereg lookup
to a read-only lookup (e.g., use or add a findByIdentifier/read-only method
instead of findOrCreateByIdentifier for mappedPreregUser), restructure logic in
userValidation to: resolve canonicalAuthUserId once (mappedUser), attempt a
non-creating lookup for prereg (mappedPreregUser) and only if that lookup
throws/fails or returns null fall back to raw ID comparison (trimmedAuthUserId
vs trimmedPreregUserId), and simplify control flow to avoid nested try/catch and
ambiguous return paths while keeping the same exception
(PreIdInvalidForUserIdException) when final checks fail.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.java`:
- Around line 492-493: The current masking in LoginService (the branch computing
visible = Math.min(4, trimmed.length()) and returning "***" +
trimmed.substring(...)) leaks short identifiers; change the logic so that if
trimmed.length() <= 4 you return a fully masked value (a string of '*' repeated
trimmed.length()), otherwise compute visible = 4 and return a mask of '*'
repeated (trimmed.length() - visible) concatenated with
trimmed.substring(trimmed.length() - visible); update the code around the
visible calculation and return to use length-based star repetition instead of
the fixed "***" prefix.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java`:
- Around line 200-205: The current strict-mode check in CommonServiceUtil allows
empty canonical/prereg IDs to be treated as a match; update the logic in the
block that uses trimmedCanonicalAuthUserId and trimmedPreregUserId (when
piiBackwardCompatibility is false) to fail-closed: if either
trimmedCanonicalAuthUserId or trimmedPreregUserId is null/empty/blank, throw
PreIdInvalidForUserIdException (using DemographicErrorCodes.PRG_PAM_APP_017 and
DemographicErrorMessages.INVALID_PREID_FOR_USER) instead of allowing the
comparison to proceed, and apply the same emptiness check and exception behavior
to the similar comparison block later (the other 210-216 block) to ensure
missing canonical IDs are treated as invalid ownership.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java`:
- Around line 321-333: The current write path in DemographicServiceUtil catches
exceptions from userDetailsService.findOrCreateByIdentifier and unconditionally
falls back to legacy/raw identifiers (setting demographicEntity.setCrAppuserId,
setCreatedBy, setUpdatedBy), which violates strict-mode; change the catch and
corresponding flows (also at the other spots flagged) to honor the
piiBackwardCompatibility policy by invoking the existing helper that resolves
the appropriate identifier under strict vs backward-compat modes and only
persisting legacy identifiers when piiBackwardCompatibility allows it;
specifically, in the try/catch around
userDetailsService.findOrCreateByIdentifier and in the other create/save/update
blocks replace the unconditional fallback with a call to the helper (use the
helper method already used elsewhere) to compute the id and then
setCrAppuserId/setCreatedBy/setUpdatedBy accordingly so strict deployments never
persist raw identifiers on mapping failure.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java`:
- Around line 182-191: The current try/catch in DocumentServiceUtil around
userDetailsService.findOrCreateByIdentifier may set crBy/updBy to the raw userId
on mapping failure; change the logic so that if an exception occurs or
mappedUser is null/has null userId you do NOT fall back to the raw userId—leave
documentEntity.crBy and updBy unset (or null) and only set them when
mappedUser.getUserId() is present; keep the catch to log the failure via
log.warn (including the exception) but remove any fallback assignment to userId
to avoid persisting raw identifiers.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DemographicServiceTest.java`:
- Around line 1256-1258: The test helper getCanonicalUserIdString currently
creates a deterministic UUID via UUID.nameUUIDFromBytes(identifier.getBytes()),
but production UserDetailsService.findOrCreateByIdentifier uses
UUID.randomUUID(), causing a mismatch; update the test to mirror production by
generating random UUIDs (use UUID.randomUUID().toString() in the helper) or
change the mock setup in the test to not rely on a specific UUID (e.g., accept
any UUID or capture the generated UUID) so the test behavior matches
UserDetailsService.findOrCreateByIdentifier.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/AppointmentServiceTest.java`:
- Around line 53-56: The static System.setProperty in AppointmentServiceTest
mutates JVM-global state; remove that static block and set the property within
the test context instead (e.g., use JUnit 5 `@DynamicPropertySource` to register
"mosip.prereg.pii.backward.compatibility=false" for the Spring context, or
annotate AppointmentServiceTest with `@TestPropertySource`(properties =
"mosip.prereg.pii.backward.compatibility=false") / use Spring's
TestPropertyValues) so the property is scoped to this test only and does not
affect other tests or JVM-global state; update the AppointmentServiceTest setup
accordingly.
In
`@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java`:
- Around line 111-114: The code currently only checks for null from
normalize(identifier) but accepts whitespace-only inputs that normalize to empty
string; update the validation in UserDetailsService (the block using
normalize(identifier) and variable norm) to treat empty strings as invalid as
well—i.e., after calling normalize(identifier) throw the same
IllegalArgumentException when norm is null or norm.isEmpty() (or otherwise
blank) so whitespace-only identifiers are rejected before hashing/creation.
- Around line 60-86: Both encryptIdentifierIfConfigured and
decryptIdentifierIfConfigured are incorrectly converting raw ciphertext/bytes
to/from UTF-8 Strings which can corrupt binary data; change them to encode
encrypted byte[] to a safe textual form (e.g., Base64) when returning/storing
and decode from Base64 back to byte[] before passing to cryptoUtil.decrypt,
ensuring you use the same encoding in both methods and preserve existing
null/empty handling and LocalDateTime usage in the cryptoUtil calls.
- Around line 116-147: The find-or-create flow in findOrCreate can race: two
threads that both miss userDetailsRepository.findByIdentifierHash(hash) may each
insert and cause a unique-key violation; change the "create" branch to attempt
insert but catch the DB unique constraint exception (e.g.,
DataIntegrityViolationException or the underlying ConstraintViolationException),
then re-query userDetailsRepository.findByIdentifierHash(hash) and return the
found row instead of failing; ensure this handling surrounds the
userDetailsRepository.save(existing or u) calls and keep using
encryptIdentifierIfConfigured(identifier) and UserDetails fields as before so
the method becomes idempotent under concurrency.
In
`@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/DataSyncService.java`:
- Around line 438-439: The call to authUserDetails().getUserId() used when
invoking serviceUtil.reverseDateSyncSave can NPE when the security principal is
absent; modify the invocation in DataSyncService so you compute a safe userId
first (call authUserDetails(), check for null and check getUserId() for
null/empty) and pass a fallback constant (the prior system user id used for
pre-registration) when auth info is missing before calling reverseDateSyncSave;
update the local variable reverseDatasyncReponse assignment to use that safe
userId.
In
`@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/util/DataSyncServiceUtil.java`:
- Around line 892-906: getCanonicalUserId currently returns the raw userId on
mapping failure, which can reintroduce plaintext PII; change the fallback to a
non-PII value (e.g., return maskIdentifier(userId) or a configured pseudonymous
constant like ANONYMOUS_USER_ID) instead of the original userId, preserving the
existing log call; update the same fallback behavior in the other similar
method(s) that use userDetailsService.findOrCreateByIdentifier (the block around
the reverseDateSyncSave logic) so neither createdBy/crBy nor any persistence
uses the original plaintext identifier.
---
Nitpick comments:
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Line 16: The schema defines identifier_hash VARCHAR(128) NOT NULL UNIQUE which
already creates a unique index, so remove the redundant index creation for
idx_prereg_user_details_hash (and any duplicate index statements at lines
corresponding to the other occurrences mentioned) to avoid double indexing;
update the DDL to drop or omit the CREATE INDEX/ADD INDEX statements that
reference idx_prereg_user_details_hash while keeping the UNIQUE constraint on
identifier_hash in the user_details table definition.
- Around line 28-29: The partial index idx_prereg_user_details_active on table
user_details uses user_id as its key while filtering on identifier_encrypted IS
NOT NULL, which is redundant because user_id is already primary-key indexed;
either drop this index or change it to index the column(s) actually used by
queries that include the predicate (for example index identifier_encrypted or
the combination of identifier_encrypted plus any frequently queried columns),
and update DDL to remove or replace CREATE INDEX CONCURRENTLY IF NOT EXISTS
idx_prereg_user_details_active with the appropriate index definition that
matches real query patterns.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java`:
- Around line 269-297: Extract the masking logic from
LoginController.maskIdentifier into a shared utility (e.g.,
IdentifierMaskUtils.maskIdentifier) and replace duplicated implementations in
LoginService, AppointmentServiceImpl, DemographicService, and other classes to
call this single static utility method; ensure the utility preserves current
behavior for emails, phone numbers (with optional '+'), UUIDs and fallback
masking, keep null/blank handling, add unit tests for the utility, and update
imports/usages across callers so all components use
IdentifierMaskUtils.maskIdentifier.
- Around line 290-294: Replace the broad catch in LoginController's UUID masking
logic with a catch for IllegalArgumentException only: when calling
UUID.fromString(trimmed) inside the method that returns masked UUID (the block
returning "***" + trimmed.substring(...)), change the catch(Exception ignored)
to catch(IllegalArgumentException ignored) so only invalid-UUID errors are
handled and other exceptions aren't accidentally suppressed; keep the existing
handling (silently ignore or log) consistent with current behavior.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java`:
- Around line 669-679: The getUserLookupIds duplication should be removed from
ApplicationService and the three helper methods (resolveCanonicalUserId,
maskIdentifier, getUserLookupIds) should be extracted into a shared utility
class (e.g., UserIdUtils or add to existing UserDetailsService); create the new
class with those methods as public static (or instance) helpers, update
ApplicationService to call UserIdUtils.resolveCanonicalUserId(...) and
UserIdUtils.getUserLookupIds(...) (or inject UserDetailsService and call the
moved methods), and remove the duplicate implementations from other services so
all callers reference the single shared implementation.
- Around line 436-444: Refactor the authorization check in ApplicationService by
normalizing values first (call authUserDetails().getUserId(),
resolveCanonicalUserId(authUserId), and trim
applicationEntity.getEffectiveCrBy() into a local effectiveCrBy variable) and
then replace the nested negated condition with a clear sequence: if
effectiveCrBy equals canonicalAuthUserId -> allow; else if
piiBackwardCompatibility && effectiveCrBy equals trimmed authUserId -> allow;
else throw PreIdInvalidForUserIdException using
ApplicationErrorCodes.PRG_APP_015 and
ApplicationErrorMessages.INVALID_APPLICATION_ID_FOR_USER. Ensure consistent
indentation and remove double-negation to improve readability around
authUserDetails(), resolveCanonicalUserId(), effectiveCrBy,
piiBackwardCompatibility, and the exception throw.
- Around line 622-637: The resolveCanonicalUserId method is duplicated in
ApplicationService and DemographicService; extract it into a shared utility
(e.g., IdentityUtils or UserIdResolver) and replace both copies with calls to
that utility. Move the logic that checks null/empty, calls
userDetailsService.findOrCreateByIdentifier(userId), trims
mappedUser.getUserId(), and returns "" on failure into a single static or
instance method (e.g., IdentityUtils.resolveCanonicalUserId(userId,
userDetailsService, logger)), preserve the existing logging call (include
maskIdentifier) by either accepting a Logger and maskIdentifier helper or
keeping maskIdentifier as a shared utility, and update ApplicationService and
DemographicService to delegate to the new utility (inject userDetailsService or
pass it as an argument) so behavior and logging remain identical.
- Around line 639-667: The maskIdentifier method is duplicated across multiple
classes; extract it to a single shared utility and replace duplicates with calls
to that utility. Create a public static String maskIdentifier(String) in
CommonServiceUtil with the exact logic currently in
ApplicationService.maskIdentifier, then remove the duplicate private methods
from ApplicationService, DemographicService, LoginService,
AppointmentServiceImpl, DataSyncServiceUtil, and LoginController and update
those classes to call CommonServiceUtil.maskIdentifier(value) (adding imports as
needed); run compilation/tests to ensure signatures and visibility match and
behavior remains identical.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java`:
- Around line 493-521: The maskIdentifier implementation in
AppointmentServiceImpl is duplicated across services; extract this logic into a
single shared utility (e.g., IdentifierMasker.maskIdentifier or
MaskUtils.maskIdentifier) in a common/shared module, make the method public
static, move the exact logic from AppointmentServiceImpl.maskIdentifier into
that utility, update AppointmentServiceImpl and other services to call the new
utility method instead of their local copies, and update/merge unit tests to
reference the centralized method and remove duplicated implementations to keep
masking policy consistent.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java`:
- Around line 978-988: The getUserLookupIds method is duplicated from
ApplicationService along with resolveCanonicalUserId and maskIdentifier;
consolidate these three helpers into a single utility class (e.g., UserIdUtils)
containing resolveCanonicalUserId, maskIdentifier, and getUserLookupIds as
static methods, move the shared logic there, update DemographicService and
ApplicationService to call UserIdUtils.resolveCanonicalUserId,
UserIdUtils.maskIdentifier, and UserIdUtils.getUserLookupIds (removing the local
copies), and ensure visibility, null-handling and piiBackwardCompatibility
behavior are preserved and covered by existing tests or add small unit tests for
the new utility methods.
- Around line 948-976: The maskIdentifier method in DemographicService is a
duplicate of the one in ApplicationService; extract the logic into a shared
utility class (e.g., PiiMaskingUtil) or an existing shared service (e.g.,
UserDetailsService) and replace the local method with a call to that utility.
Create a public static maskIdentifier(String) in PiiMaskingUtil (or a public
method on UserDetailsService), move the masking logic there, update
DemographicService to call PiiMaskingUtil.maskIdentifier(value) (or
userDetailsService.maskIdentifier(value)), and remove the duplicate private
maskIdentifier from DemographicService; ensure imports and any tests/refactors
reference the single shared implementation.
- Around line 932-946: The resolveCanonicalUserId method is duplicated in
DemographicService and ApplicationService; extract it into a shared utility
(e.g., a new CanonicalUserUtil or UserIdMapper) and have both classes call that
utility. Move the logic that uses
userDetailsService.findOrCreateByIdentifier(...) and maskIdentifier(...) into
the utility, or alternatively keep the dependency on userDetailsService in the
callers and expose a utility method like
mapToCanonicalUserId(UserDetailsService, String) that performs the try/catch and
logging; update DemographicService.resolveCanonicalUserId and the one in
ApplicationService to delegate to the new utility, adjust imports, ensure the
logger and maskIdentifier are accessible (or pass a masking function/logger into
the utility), and run tests to verify no behavioral changes.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.java`:
- Around line 478-483: In the LoginService class, remove the unreachable
small-length phone branch inside the trimmed.matches("\\+?\\d{10,12}") block:
delete the inner if (digits.length() <= 4) { return (hasPlus ? "+" : "") +
"****"; } since digits is already constrained to 10..12 by the regex; leave the
remaining masking logic in this block intact so the method compiles and behavior
is unchanged for valid 10–12 digit inputs.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.java`:
- Around line 144-155: The create/update branches in OTPManager duplicate the
canonical user resolution logic: extract a private helper method (e.g.,
resolveCanonicalUpdater) that calls
userDetailsService.findOrCreateByIdentifier(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)),
sets the updater value using otpTxn.setUpdBy(...) with the
mappedUser.getUserId().toString() fallback to the raw property, and logs
failures via logger.warn(...) with the exception; replace the duplicated blocks
in both create and update paths with a call to this helper to keep behavior
consistent and reduce maintenance.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java`:
- Around line 292-297: The documentEntitySetter contains redundant assignments:
remove the initial copyDocumentEntity.setCrBy(sourceEntity.getCrBy()) and
copyDocumentEntity.setUpdBy(sourceEntity.getUpdBy()) since they are immediately
overwritten by copyDocumentEntity.setCrBy(sourceEntity.getEffectiveCrBy()) and
copyDocumentEntity.setUpdBy(sourceEntity.getEffectiveUpdBy()); keep only the
effective-crBy/updBy assignments (and the existing setLangCode) so the method
uses sourceEntity.getEffectiveCrBy()/getEffectiveUpdBy() and avoid the duplicate
setCrBy/setUpdBy calls.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java`:
- Around line 598-606: The NullAuthority helper class should be made private
static final to limit its scope to this test class; locate the static inner
class named NullAuthority (implements GrantedAuthority) and change its
declaration from package-private static to private static final so it remains
test-scoped and immutable.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DocumentServiceTest.java`:
- Around line 161-177: Extract the repeated DocumentEntity setup in
DocumentServiceTest into a test helper/builder to reduce noise: create a factory
method (e.g., buildTestDocumentEntity or DocumentEntityBuilder) that
encapsulates constructing DocumentEntity and setting fields used here
(demographicEntity, documentId, docName, docCatCode, docTypeCode, docFileFormat,
statusCode, langCode, crBy, crDtime, updBy, updDtime using
DateUtils.parseDateToLocalDateTime(new Date()), encryptedDateTime, docId,
docHash using HashUtill.hashUtill(cephBytes), refNumber) and replace the inline
block in tests with a call to that helper; keep the helper in test sources so
other tests can reuse it and update instantiations in DocumentServiceTest to use
the new builder API.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.java`:
- Around line 88-90: The test suite needs explicit unit tests verifying the new
canonical mapping path in dtoToEntity: add two tests in DocumentServiceUtilTest
that exercise dtoToEntity using the mocked UserDetailsService—(1) a success case
where UserDetailsService returns a UserDetails with the canonical identifier and
assert the resulting entity fields crBy and updBy are set to that canonical
value, and (2) a failure/fallback case where UserDetailsService returns null or
throws and assert dtoToEntity falls back to the previous behavior (e.g., uses
the original dto values or a default) for crBy/updBy; locate the call to
dtoToEntity and the UserDetailsService mock in the test class and update
assertions accordingly to validate both canonical mapping and fallback.
In
`@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/AvailibityEntity.java`:
- Around line 90-96: The AvailibityEntity exposes getEffectiveCrBy() and
getEffectiveUpdBy() which are inconsistent with InterfaceDataSyncEntity's
getEffectiveCreatedBy() and getEffectiveUpdatedBy(); rename AvailibityEntity's
methods to getEffectiveCreatedBy() and getEffectiveUpdatedBy(), update any
internal field mappings (crBy -> createdBy/upBy -> updatedBy or keep fields but
ensure getters map correctly), and search/replace all usages of
getEffectiveCrBy() and getEffectiveUpdBy() to the new method names to maintain
API consistency across entities.
In
`@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java`:
- Around line 39-84: Add two edge-case unit tests in UserDetailsServiceTest: one
that verifies findOrCreateByIdentifier and getDecryptedIdentifier handle
non-text/binary ciphertext bytes by mocking cryptoUtil.encrypt to return
non-UTF8 bytes (e.g., arbitrary byte array) and asserting decryption and
save/lookup still behave correctly; and another that verifies
findOrCreateByIdentifier rejects blank/whitespace identifiers by calling
userDetailsService.findOrCreateByIdentifier with blank inputs and asserting no
save on userDetailsRepository and that an appropriate empty/exceptional result
is returned. Use existing mocks (userDetailsRepository, cryptoUtil) and methods
(findOrCreateByIdentifier, getDecryptedIdentifier, findById) to set up
repository returns and verify interactions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
db_scripts/mosip_prereg/ddl/user_details.sql (1)
17-19:⚠️ Potential issue | 🔴 Critical
NOT NULLconstraints conflict with current save-path behavior.Lines 17 and 19 require encrypted fields always present, but
UserDetailsServicecan persistidentifier_encrypted = null(seepre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java, snippets around Lines 60-74 and 137-147). This will cause insert failures in fallback/encryption-failure paths.💡 Suggested DDL fix
CREATE TABLE IF NOT EXISTS prereg.user_details ( user_id UUID PRIMARY KEY DEFAULT gen_random_uuid(), identifier_hash VARCHAR(128) NOT NULL UNIQUE, - identifier_encrypted TEXT NOT NULL, + identifier_encrypted TEXT, cr_dtimes TIMESTAMP NOT NULL, - encrypted_dtimes TIMESTAMP NOT NULL + encrypted_dtimes TIMESTAMP, + CONSTRAINT chk_user_details_encryption_pair CHECK ( + (identifier_encrypted IS NULL AND encrypted_dtimes IS NULL) OR + (identifier_encrypted IS NOT NULL AND encrypted_dtimes IS NOT NULL) + ) );#!/bin/bash set -euo pipefail echo "== DDL nullability ==" rg -n "identifier_encrypted|encrypted_dtimes" db_scripts/mosip_prereg/ddl/user_details.sql echo "" echo "== Service behavior allowing null encrypted value ==" rg -n "encryptIdentifierIfConfigured|setIdentifierEncrypted|setEncryptedDtimes|return null" \ pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db_scripts/mosip_prereg/ddl/user_details.sql` around lines 17 - 19, The DDL currently enforces NOT NULL on identifier_encrypted and encrypted_dtimes which conflicts with UserDetailsService behavior that can persist null encrypted values (see methods encryptIdentifierIfConfigured, setIdentifierEncrypted, setEncryptedDtimes); update the user_details.sql column definitions for identifier_encrypted and encrypted_dtimes to allow NULL (remove the NOT NULL constraint) so inserts succeed in fallback/encryption-failure paths and preserve existing application behavior.
🧹 Nitpick comments (3)
pre-registration/pre-registration-application-service/pom.xml (2)
98-98: Use${kernel.core.version}property instead of hardcoded version.The parent POM defines
kernel.core.version=1.3.0. Using the property reference ensures version consistency and simplifies future upgrades.♻️ Suggested refactor
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-core</artifactId> - <version>1.3.0</version> + <version>${kernel.core.version}</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/pom.xml` at line 98, Replace the hardcoded version element in the pre-registration module POM by referencing the parent property: update the <version> element currently set to "1.3.0" to use the property ${kernel.core.version} so the module uses the kernel.core.version defined in the parent POM; locate the <version> tag in pre-registration-application-service/pom.xml and change its value to the property reference to maintain consistency with the parent.
248-248: Consider using a property instead of hardcoded version.Similar to the
kernel-coredependency, thekernel-auth-adapterversion is hardcoded. For consistency with the rest of the project's version management approach, consider using${kernel.bom.version}or defining a dedicated property.♻️ Suggested refactor
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-auth-adapter</artifactId> - <version>1.3.0</version> + <version>${kernel.bom.version}</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/pom.xml` at line 248, The dependency declaration for kernel-auth-adapter currently hardcodes <version>1.3.0</version>; change it to use the existing BOM/property strategy by replacing the literal with the project property (e.g., ${kernel.bom.version} or a new property like ${kernel.auth.adapter.version}) so it matches how kernel-core is versioned; update the pom's dependency element that references kernel-auth-adapter to reference the chosen property and, if you introduce a new property, add it to the <properties> block (symbol references: the kernel-auth-adapter dependency and the <version> element).pre-registration/pre-registration-datasync-service/pom.xml (1)
169-169: Consider using a property instead of hardcoded version.The
kernel-auth-adapterversion is hardcoded as1.3.0while other dependencies in this file use property references. For easier maintenance during future upgrades, consider using${kernel.bom.version}or defining a dedicated property.♻️ Suggested refactor
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-auth-adapter</artifactId> - <version>1.3.0</version> + <version>${kernel.bom.version}</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-datasync-service/pom.xml` at line 169, Replace the hardcoded <version>1.3.0</version> for the kernel-auth-adapter dependency with a Maven property reference to centralize version management (e.g., use ${kernel.bom.version} or a new <kernel.auth.version> property); update the dependency entry that references kernel-auth-adapter and add or reuse the property in the <properties> section so future upgrades can be done by changing the single property value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Around line 25-29: Remove the redundant full index creation for
idx_prereg_user_details_hash on prereg.user_details because
UNIQUE(identifier_hash) already creates the same index; delete the CREATE INDEX
IF NOT EXISTS idx_prereg_user_details_hash ... ON
prereg.user_details(identifier_hash) statement, but keep the partial index
statement CREATE INDEX IF NOT EXISTS idx_prereg_user_details_active ... ON
prereg.user_details(user_id) WHERE identifier_encrypted IS NOT NULL as-is.
---
Duplicate comments:
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Around line 17-19: The DDL currently enforces NOT NULL on identifier_encrypted
and encrypted_dtimes which conflicts with UserDetailsService behavior that can
persist null encrypted values (see methods encryptIdentifierIfConfigured,
setIdentifierEncrypted, setEncryptedDtimes); update the user_details.sql column
definitions for identifier_encrypted and encrypted_dtimes to allow NULL (remove
the NOT NULL constraint) so inserts succeed in fallback/encryption-failure paths
and preserve existing application behavior.
---
Nitpick comments:
In `@pre-registration/pre-registration-application-service/pom.xml`:
- Line 98: Replace the hardcoded version element in the pre-registration module
POM by referencing the parent property: update the <version> element currently
set to "1.3.0" to use the property ${kernel.core.version} so the module uses the
kernel.core.version defined in the parent POM; locate the <version> tag in
pre-registration-application-service/pom.xml and change its value to the
property reference to maintain consistency with the parent.
- Line 248: The dependency declaration for kernel-auth-adapter currently
hardcodes <version>1.3.0</version>; change it to use the existing BOM/property
strategy by replacing the literal with the project property (e.g.,
${kernel.bom.version} or a new property like ${kernel.auth.adapter.version}) so
it matches how kernel-core is versioned; update the pom's dependency element
that references kernel-auth-adapter to reference the chosen property and, if you
introduce a new property, add it to the <properties> block (symbol references:
the kernel-auth-adapter dependency and the <version> element).
In `@pre-registration/pre-registration-datasync-service/pom.xml`:
- Line 169: Replace the hardcoded <version>1.3.0</version> for the
kernel-auth-adapter dependency with a Maven property reference to centralize
version management (e.g., use ${kernel.bom.version} or a new
<kernel.auth.version> property); update the dependency entry that references
kernel-auth-adapter and add or reuse the property in the <properties> section so
future upgrades can be done by changing the single property value.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03b0fad5-b433-4b84-ad60-9ce4a5f18d8b
📒 Files selected for processing (6)
db_scripts/mosip_prereg/ddl.sqldb_scripts/mosip_prereg/ddl/user_details.sqlpre-registration/pom.xmlpre-registration/pre-registration-application-service/pom.xmlpre-registration/pre-registration-core/pom.xmlpre-registration/pre-registration-datasync-service/pom.xml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java`:
- Line 151: In ApplicationConsumedStatusUpdater, avoid calling
resolveCanonicalUserId(auditUserId) for each row/document; resolve the canonical
user ID once at the start of the batch execution and reuse it when setting
updatedBy (e.g., replace repeated calls in places where
demographicEntityConsumed.setUpdatedBy(resolveCanonicalUserId(auditUserId)) and
similar lines are used); compute a local final variable like resolvedAuditUser =
resolveCanonicalUserId(auditUserId) in the batch-level method that drives the
per-record updates and use resolvedAuditUser for updatedBy assignments to
eliminate redundant work.
- Around line 226-239: The resolveCanonicalUserId method currently throws
IllegalStateException when userDetails mapping fails; change it to perform a
backward-compatible fallback like CommonServiceUtil: call
userDetailsService.findOrCreateByIdentifier(trimmedUserId), and if the returned
UserDetails or userId is null, log a warning/INFO and return the original
trimmedUserId instead of throwing; keep the UUID-short-circuit (isUuid) and
ensure you reference resolveCanonicalUserId,
userDetailsService.findOrCreateByIdentifier, and UserDetails.getUserId() while
implementing the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66860152-5e43-488f-a5b9-d6ec8c471799
📒 Files selected for processing (1)
pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java
mayuradesh
requested changes
Mar 18, 2026
mayuradesh
left a comment
mayuradesh
left a comment
Contributor
There was a problem hiding this comment.
- Move common code like adding record to UserDetails table, creating a list of userIds based on compatibility mode etc to common Utils.
- From all places use this common code.
- There are too many needless IF conditions which can cause silent failures in code, review all and include only MUST conditions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java (1)
76-90: Encryption/decryption round-trip appears correct but relies on CryptoUtil implementation details.Based on the
CryptoUtilcontext snippets, theencrypt()method returns bytes of a Base64-encoded string (frombody.getResponse().getData().getBytes()), anddecrypt()expects UTF-8 string bytes that it converts back to a String for the DTO. The current implementation should work correctly for this specific CryptoUtil behavior.The past review comment about UTF-8 corruption assumed raw binary ciphertext, but the crypto service returns Base64-encoded responses. However, this coupling is implicit and could break if CryptoUtil's response format changes.
Consider adding a comment documenting this dependency:
// Note: CryptoUtil returns Base64-encoded strings from the crypto service, // so UTF-8 round-trip is safe here. If CryptoUtil changes to return raw // binary, this must be updated to use explicit Base64 encoding.Also applies to: 100-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java` around lines 76 - 90, The encryptIdentifierIfConfigured method relies on CryptoUtil.encrypt returning a Base64-encoded string as UTF-8 bytes, which makes the current new String(byte[], UTF_8) safe but fragile; update the method (encryptIdentifierIfConfigured) to add a clear inline comment above the try-block documenting this dependency: note that CryptoUtil returns Base64-encoded strings (UTF-8 bytes) and that if CryptoUtil changes to return raw binary ciphertext this code must switch to explicit Base64 encoding/decoding, and apply the same comment to the corresponding decrypt/round-trip code paths referenced in this class.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java (1)
432-437: Duplicated validation logic; consider reusinguserValidationmethod.Lines 432-437 implement ownership validation inline, duplicating logic from the
userValidationmethod at lines 547-559. Consider extracting a common helper or invoking the existing method to reduce duplication.♻️ Consider extracting shared validation
The inline validation:
String authUserId = authUserDetails().getUserId(); String effectiveCrBy = applicationEntity.getEffectiveCrBy() == null ? "" : applicationEntity.getEffectiveCrBy().trim(); if (!userDetailsService.matchesUser(authUserId, effectiveCrBy, piiBackwardCompatibility)) { throw new PreIdInvalidForUserIdException(...); }Could potentially be consolidated with the
userValidationmethod to avoid code duplication. Currently there's a commented-out//userValidation(applicationEntity);at line 431 suggesting this was considered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java` around lines 432 - 437, Replace the duplicated inline ownership check with a single reusable validation call: remove the authUserId/effectiveCrBy/matchesUser block and invoke the existing userValidation(applicationEntity) helper (or extract a new private helper that calls userDetailsService.matchesUser and throws PreIdInvalidForUserIdException) so the ownership check is centralized; ensure the helper uses authUserDetails().getUserId(), applicationEntity.getEffectiveCrBy() with the same trim/null handling, and the same piiBackwardCompatibility flag to preserve behavior.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java (1)
477-488: RedundantupdByassignment at line 477.Line 477 sets
applicationEntity.setUpdBy(authUserDetails().getUserId())with the raw user ID, then lines 480-481 immediately overwrite it with the canonical user ID. The initial assignment is unnecessary.♻️ Remove redundant assignment
- applicationEntity.setUpdBy(authUserDetails().getUserId()); applicationEntity.setCrDtime(LocalDateTime.now(ZoneId.of("UTC"))); try { applicationEntity.setUpdBy( userDetailsService.resolveCanonicalUserIdOrIdentifier(authUserDetails().getUserId())); return applicationRepostiory.save(applicationEntity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java` around lines 477 - 488, Remove the redundant pre-overwrite of updBy: in AppointmentServiceImpl remove the initial call applicationEntity.setUpdBy(authUserDetails().getUserId()) and keep only the canonical assignment using userDetailsService.resolveCanonicalUserIdOrIdentifier(authUserDetails().getUserId()) before saving via applicationRepostiory.save; ensure no other logic depends on the raw userId being set earlier and that exception handling around applicationRepostiory.save remains unchanged.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java (1)
408-415: Side-effect mutation of input list incompareUploadedDocListAndValidMandatoryDocList.The
forEachwithremovemutates thevalidMandatoryDocForApplicantlist passed as a parameter. While functionally correct, this side effect can be surprising to callers and may cause issues if the caller expects the list to remain unchanged.♻️ Consider using a non-mutating approach
private boolean compareUploadedDocListAndValidMandatoryDocList(List<String> uploadedDocs, List<String> validMandatoryDocForApplicant) { if (validMandatoryDocForApplicant.isEmpty()) { return true; } - uploadedDocs.forEach(validMandatoryDocForApplicant::remove); - return validMandatoryDocForApplicant.isEmpty(); + return uploadedDocs.containsAll(validMandatoryDocForApplicant); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java` around lines 408 - 415, The method compareUploadedDocListAndValidMandatoryDocList mutates the input validMandatoryDocForApplicant by calling remove in uploadedDocs.forEach; instead, avoid side-effects by performing the check on a copy or using a non-mutating containment check (e.g., create a new List/Set from validMandatoryDocForApplicant or convert uploadedDocs to a Set and call containsAll) so the original validMandatoryDocForApplicant is not modified; update the implementation in compareUploadedDocListAndValidMandatoryDocList to operate on that copy/Set and return the emptiness/containment result.pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java (1)
94-96: PreferassertEqualsoverassertTruefor clearer failure messages.Using
assertEqualsprovides a more descriptive failure message showing expected vs actual values.♻️ Use assertEquals for clearer assertion
Optional<String> decrypted = userDetailsService.getDecryptedIdentifier(userId); assertTrue(decrypted.isPresent()); - assertTrue("TestUser123".equals(decrypted.get())); + assertEquals("TestUser123", decrypted.get());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java` around lines 94 - 96, Replace the boolean equality check that uses assertTrue("TestUser123".equals(decrypted.get())) with an assertEquals call so failures show expected vs actual; keep the presence check assertTrue(decrypted.isPresent()) and change the comparison to assertEquals("TestUser123", decrypted.get()) in UserDetailsServiceTest (reference: variable decrypted).pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java (2)
304-323: Redundant initial assignments before canonical resolution.Lines 304-305 and 313 set
crAppuserId,createdBy, andupdatedBytouserId, which are then immediately overwritten at lines 321-323 with the effective (canonical) user ID. Consider removing the initial assignments or consolidating the logic.♻️ Remove redundant assignments
- demographicEntity.setCrAppuserId(userId); - demographicEntity.setCreatedBy(userId); demographicEntity.setCreateDateTime(LocalDateTime.now(ZoneId.of("UTC"))); demographicEntity.setStatusCode(statuscode); log.info(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "Hashing start time : " + DateUtils.getUTCCurrentDateTimeString()); demographicEntity.setDemogDetailHash(HashUtill.hashUtill(demographicEntity.getApplicantDetailJson())); log.info(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "Hashing end time : " + DateUtils.getUTCCurrentDateTimeString()); - demographicEntity.setUpdatedBy(userId); demographicEntity.setUpdateDateTime(LocalDateTime.now(ZoneId.of("UTC"))); demographicEntity.setEncryptedDateTime(encryptionDateTime); String effectiveUserId = userDetailsService.resolveCanonicalUserIdOrIdentifier(userId); // ... logging ... demographicEntity.setCrAppuserId(effectiveUserId); demographicEntity.setCreatedBy(effectiveUserId); demographicEntity.setUpdatedBy(effectiveUserId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java` around lines 304 - 323, In DemographicServiceUtil, remove the redundant early assignments of crAppuserId/createdBy/updatedBy to userId (the calls demographicEntity.setCrAppuserId(userId); demographicEntity.setCreatedBy(userId); demographicEntity.setUpdatedBy(userId)) since they are overwritten after resolving the canonical user via userDetailsService.resolveCanonicalUserIdOrIdentifier; keep the other initialization (timestamps, status, hashing) as-is and ensure that only the resolved effectiveUserId is used in demographicEntity.setCrAppuserId(effectiveUserId), setCreatedBy(effectiveUserId), and setUpdatedBy(effectiveUserId) to avoid unnecessary temporary values.
712-722: Redundant initial assignments insaveAndUpdateApplicationEntity.Similar to
prepareDemographicEntityForCreate, lines 712-715 setcrBy,updBytouserId, then lines 720-722 overwrite with the effective user ID.♻️ Consolidate user ID assignments
applicationEntity.setApplicationStatusCode(applicationStatusCode); applicationEntity.setBookingType(bookingTypeCode); applicationEntity.setBookingStatusCode(bookingStatusCode); - applicationEntity.setCrBy(userId); applicationEntity.setCrDtime(LocalDateTime.now(ZoneId.of("UTC"))); - applicationEntity.setUpdBy(userId); applicationEntity.setUpdDtime(LocalDateTime.now(ZoneId.of("UTC"))); String effectiveUserId = userDetailsService.resolveCanonicalUserIdOrIdentifier(userId); log.info(...); applicationEntity.setCrBy(effectiveUserId); applicationEntity.setUpdBy(effectiveUserId); applicationEntity.setContactInfo(effectiveUserId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java` around lines 712 - 722, In saveAndUpdateApplicationEntity remove the redundant initial assignments of applicationEntity.setCrBy(userId) and applicationEntity.setUpdBy(userId) so you only set the timestamps (applicationEntity.setCrDtime / setUpdDtime) before resolving the canonical user; then call userDetailsService.resolveCanonicalUserIdOrIdentifier(userId) and assign the resulting effectiveUserId once to applicationEntity.setCrBy, applicationEntity.setUpdBy and applicationEntity.setContactInfo, keeping the existing log that uses maskIdentifier and isCanonicalApplied; this mirrors the consolidated pattern used in prepareDemographicEntityForCreate and avoids overwriting values.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.java (1)
105-134: Unit test mocks bothfindOrCreateByIdentifierandmatchesUser, missing validation of actual production behavior.The test helper
getCanonicalUserIdStringusesUUID.nameUUIDFromBytes(deterministic), while productionUserDetailsService.findOrCreateByIdentifiercreates new users withUUID.randomUUID(). Both methods are mocked in this test, so the internally consistent mocking validates the contract between methods but not the actual implementation. Although production behavior is deterministic for the same identifier (due to hash-based lookup), adding an integration test would validate the real UUID generation and matching flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.java` around lines 105 - 134, The test currently stubs both userDetailsService.findOrCreateByIdentifier and userDetailsService.matchesUser with a deterministic UUID (getCanonicalUserIdString), which hides real production behavior; update the setup so findOrCreateByIdentifier returns a UserDetails whose userId is generated with UUID.randomUUID() (to mirror production) and stop stubbing matchesUser (or replace the stub by using a spy on UserDetailsService) so the real matchesUser logic is exercised; adjust ApplicationServiceTest.setUp() to reflect these changes (modify the Mockito.when(...) for findOrCreateByIdentifier and remove or convert the Mockito.when(...) for matchesUser), or alternatively add an integration test that does not mock UserDetailsService to validate the end-to-end UUID generation and matching flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.java`:
- Around line 140-145: The code incorrectly canonicalizes a system client ID:
replace the call that obtains and canonicalizes the system property
(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID) +
userDetailsService.resolveCanonicalUserIdOrIdentifier(...)) and stop using the
resolved canonical value for audit fields like otpTxn.setUpdBy; instead set
otpTxn.setUpdBy to the original client id (the raw
environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)) or, if the
intent was to audit the actual end user, use requestDTO.getRequest().getUserId()
and canonicalize that via
userDetailsService.resolveCanonicalUserIdOrIdentifier(...) before
assigning—apply the same change for the similar block around lines 156-161.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java`:
- Around line 31-33: Remove the duplicate import of
org.springframework.beans.factory.annotation.Value in the DemographicServiceUtil
class: keep a single import for Value alongside the existing imports (e.g.,
org.springframework.beans.factory.annotation.Qualifier) and delete the redundant
line to avoid duplicate imports.
---
Nitpick comments:
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java`:
- Around line 432-437: Replace the duplicated inline ownership check with a
single reusable validation call: remove the authUserId/effectiveCrBy/matchesUser
block and invoke the existing userValidation(applicationEntity) helper (or
extract a new private helper that calls userDetailsService.matchesUser and
throws PreIdInvalidForUserIdException) so the ownership check is centralized;
ensure the helper uses authUserDetails().getUserId(),
applicationEntity.getEffectiveCrBy() with the same trim/null handling, and the
same piiBackwardCompatibility flag to preserve behavior.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java`:
- Around line 477-488: Remove the redundant pre-overwrite of updBy: in
AppointmentServiceImpl remove the initial call
applicationEntity.setUpdBy(authUserDetails().getUserId()) and keep only the
canonical assignment using
userDetailsService.resolveCanonicalUserIdOrIdentifier(authUserDetails().getUserId())
before saving via applicationRepostiory.save; ensure no other logic depends on
the raw userId being set earlier and that exception handling around
applicationRepostiory.save remains unchanged.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java`:
- Around line 408-415: The method compareUploadedDocListAndValidMandatoryDocList
mutates the input validMandatoryDocForApplicant by calling remove in
uploadedDocs.forEach; instead, avoid side-effects by performing the check on a
copy or using a non-mutating containment check (e.g., create a new List/Set from
validMandatoryDocForApplicant or convert uploadedDocs to a Set and call
containsAll) so the original validMandatoryDocForApplicant is not modified;
update the implementation in compareUploadedDocListAndValidMandatoryDocList to
operate on that copy/Set and return the emptiness/containment result.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java`:
- Around line 304-323: In DemographicServiceUtil, remove the redundant early
assignments of crAppuserId/createdBy/updatedBy to userId (the calls
demographicEntity.setCrAppuserId(userId);
demographicEntity.setCreatedBy(userId); demographicEntity.setUpdatedBy(userId))
since they are overwritten after resolving the canonical user via
userDetailsService.resolveCanonicalUserIdOrIdentifier; keep the other
initialization (timestamps, status, hashing) as-is and ensure that only the
resolved effectiveUserId is used in
demographicEntity.setCrAppuserId(effectiveUserId),
setCreatedBy(effectiveUserId), and setUpdatedBy(effectiveUserId) to avoid
unnecessary temporary values.
- Around line 712-722: In saveAndUpdateApplicationEntity remove the redundant
initial assignments of applicationEntity.setCrBy(userId) and
applicationEntity.setUpdBy(userId) so you only set the timestamps
(applicationEntity.setCrDtime / setUpdDtime) before resolving the canonical
user; then call userDetailsService.resolveCanonicalUserIdOrIdentifier(userId)
and assign the resulting effectiveUserId once to applicationEntity.setCrBy,
applicationEntity.setUpdBy and applicationEntity.setContactInfo, keeping the
existing log that uses maskIdentifier and isCanonicalApplied; this mirrors the
consolidated pattern used in prepareDemographicEntityForCreate and avoids
overwriting values.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.java`:
- Around line 105-134: The test currently stubs both
userDetailsService.findOrCreateByIdentifier and userDetailsService.matchesUser
with a deterministic UUID (getCanonicalUserIdString), which hides real
production behavior; update the setup so findOrCreateByIdentifier returns a
UserDetails whose userId is generated with UUID.randomUUID() (to mirror
production) and stop stubbing matchesUser (or replace the stub by using a spy on
UserDetailsService) so the real matchesUser logic is exercised; adjust
ApplicationServiceTest.setUp() to reflect these changes (modify the
Mockito.when(...) for findOrCreateByIdentifier and remove or convert the
Mockito.when(...) for matchesUser), or alternatively add an integration test
that does not mock UserDetailsService to validate the end-to-end UUID generation
and matching flow.
In
`@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java`:
- Around line 76-90: The encryptIdentifierIfConfigured method relies on
CryptoUtil.encrypt returning a Base64-encoded string as UTF-8 bytes, which makes
the current new String(byte[], UTF_8) safe but fragile; update the method
(encryptIdentifierIfConfigured) to add a clear inline comment above the
try-block documenting this dependency: note that CryptoUtil returns
Base64-encoded strings (UTF-8 bytes) and that if CryptoUtil changes to return
raw binary ciphertext this code must switch to explicit Base64
encoding/decoding, and apply the same comment to the corresponding
decrypt/round-trip code paths referenced in this class.
In
`@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java`:
- Around line 94-96: Replace the boolean equality check that uses
assertTrue("TestUser123".equals(decrypted.get())) with an assertEquals call so
failures show expected vs actual; keep the presence check
assertTrue(decrypted.isPresent()) and change the comparison to
assertEquals("TestUser123", decrypted.get()) in UserDetailsServiceTest
(reference: variable decrypted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9a8e103-a77f-42c8-adb8-1353609083df
📒 Files selected for processing (20)
pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DemographicServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/LoginServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/OTPManagerTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/controller/LoginControllerTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.javapre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.javapre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.javapre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/util/DataSyncServiceUtil.java
🚧 Files skipped from review as they are similar to previous changes (5)
- pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java
- pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java
- pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java
- pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java
- pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DemographicServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java (3)
124-136: Add verification thatsave()is not called to match test intent.The test name indicates it should verify no repair occurs, but there's no assertion confirming that
save()is never invoked on the repository.♻️ Suggested improvement
+import static org.mockito.Mockito.never; `@Test` public void testResolveCanonicalUserIdReturnsExistingUuidWithoutRepairingRecord() { UserDetails mapped = new UserDetails(); mapped.setUserId(UUID.randomUUID()); mapped.setCrDtimes(LocalDateTime.now()); mapped.setIdentifierEncrypted(""); when(userDetailsRepository.findByIdentifierHash(any())).thenReturn(Optional.of(mapped)); Optional<String> resolved = userDetailsService.resolveCanonicalUserId("TestUser"); assertTrue(resolved.isPresent()); assertEquals(mapped.getUserId().toString(), resolved.get()); + verify(userDetailsRepository, never()).save(any()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java` around lines 124 - 136, The test testResolveCanonicalUserIdReturnsExistingUuidWithoutRepairingRecord should verify that no repair/save occurs: after stubbing userDetailsRepository.findByIdentifierHash(...) to return the mapped UserDetails and calling userDetailsService.resolveCanonicalUserId("TestUser"), add a Mockito verification that userDetailsRepository.save(...) is never called (verify(userDetailsRepository, never()).save(any())). Reference the test method testResolveCanonicalUserIdReturnsExistingUuidWithoutRepairingRecord, the userDetailsRepository.save method, and resolveCanonicalUserId to locate where to insert the verification.
27-40: Consider using@ExtendWith(MockitoExtension.class)for proper resource management.
MockitoAnnotations.openMocks(this)returns anAutoCloseablethat should be closed to avoid resource leaks. The JUnit 5 idiomatic approach is to use the Mockito extension instead.♻️ Suggested refactor
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) public class UserDetailsServiceTest { `@Mock` private UserDetailsRepository userDetailsRepository; `@Mock` private CryptoUtil cryptoUtil; `@InjectMocks` private UserDetailsService userDetailsService; - public UserDetailsServiceTest() { - MockitoAnnotations.openMocks(this); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java` around lines 27 - 40, The test class UserDetailsServiceTest currently calls MockitoAnnotations.openMocks(this) in its constructor which returns an AutoCloseable that must be closed; replace this manual lifecycle management by annotating the test class with `@ExtendWith`(MockitoExtension.class) and remove the constructor/openMocks call so Mockito manages mocks automatically for fields userDetailsRepository, cryptoUtil and the `@InjectMocks` userDetailsService.
95-95: PreferassertEqualsfor better failure messages.Using
assertTrue(expected.equals(actual))provides unhelpful failure messages.assertEqualsshows both expected and actual values.♻️ Suggested fix
- assertTrue("TestUser123".equals(decrypted.get())); + assertEquals("TestUser123", decrypted.get());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java` at line 95, Replace the brittle assertion in UserDetailsServiceTest that uses assertTrue("TestUser123".equals(decrypted.get())) with an assertEquals call to provide clearer failure messages; locate the assertion referencing the decrypted variable in the test method and change it to assertEquals with "TestUser123" as the expected value and decrypted.get() as the actual value (using the same test class/method and import style already used in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java`:
- Around line 54-60: The test testFindByIdentifierDelegatesToRepo currently
ignores the returned Optional res; update it to assert that res.isPresent() is
true and that res.get() equals the mocked UserDetails instance you set up.
Locate the test method testFindByIdentifierDelegatesToRepo and add assertions
after calling userDetailsService.findByIdentifier("TestUser") to check
Optional<UserDetails> res is present and that the contained object equals the
mock (the same instance created as UserDetails mock and returned by
userDetailsRepository.findByIdentifierHash(any())); keep the existing
verify(userDetailsRepository).findByIdentifierHash(any()) assertion.
---
Nitpick comments:
In
`@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java`:
- Around line 124-136: The test
testResolveCanonicalUserIdReturnsExistingUuidWithoutRepairingRecord should
verify that no repair/save occurs: after stubbing
userDetailsRepository.findByIdentifierHash(...) to return the mapped UserDetails
and calling userDetailsService.resolveCanonicalUserId("TestUser"), add a Mockito
verification that userDetailsRepository.save(...) is never called
(verify(userDetailsRepository, never()).save(any())). Reference the test method
testResolveCanonicalUserIdReturnsExistingUuidWithoutRepairingRecord, the
userDetailsRepository.save method, and resolveCanonicalUserId to locate where to
insert the verification.
- Around line 27-40: The test class UserDetailsServiceTest currently calls
MockitoAnnotations.openMocks(this) in its constructor which returns an
AutoCloseable that must be closed; replace this manual lifecycle management by
annotating the test class with `@ExtendWith`(MockitoExtension.class) and remove
the constructor/openMocks call so Mockito manages mocks automatically for fields
userDetailsRepository, cryptoUtil and the `@InjectMocks` userDetailsService.
- Line 95: Replace the brittle assertion in UserDetailsServiceTest that uses
assertTrue("TestUser123".equals(decrypted.get())) with an assertEquals call to
provide clearer failure messages; locate the assertion referencing the decrypted
variable in the test method and change it to assertEquals with "TestUser123" as
the expected value and decrypted.get() as the actual value (using the same test
class/method and import style already used in the file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06ba67bf-6a07-4ee8-b9e7-628730030712
📒 Files selected for processing (2)
pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.javapre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java
Contributor
Author
|
Signed-off-by: Mohanachandran S <165888272+mohanachandran-s@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: rajapandi1234 <138785181+rajapandi1234@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: rajapandi1234 <138785181+rajapandi1234@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Mohanachandran S <165888272+mohanachandran-s@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Sohan Kumar Dey <72375959+Sohandey@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Sohan Kumar Dey <72375959+Sohandey@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Sohan Kumar Dey <72375959+Sohandey@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Sohan Kumar Dey <72375959+Sohandey@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: VSIVAKALYAN <103260988+VSIVAKALYAN@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: VSIVAKALYAN <103260988+VSIVAKALYAN@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Nandhukumar <nandhukumare@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Nandhukumar <nandhukumare@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Nandhukumar <nandhukumare@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Nandhukumar <nandhukumare@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
* MOSIP-39585 - Updated the apitest commons version Signed-off-by: Mohanachandran S <165888272+mohanachandran-s@users.noreply.github.com> * Update push-trigger.yml Signed-off-by: Mohanachandran S <165888272+mohanachandran-s@users.noreply.github.com> --------- Signed-off-by: Mohanachandran S <165888272+mohanachandran-s@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: dhanendra06 <dhanendra.tech@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: kameshsr <kameshsr1338@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Rakshith B <79500257+Rakshithb1@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Nandhukumar <nandhukumare@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
* MOSIP-36404 - Added admin role to GetPRIDByDate api Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in> * MOSIP-39993 - Added the value mapping property file Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in> --------- Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
* MOSIP-39719 Signed-off-by: Nandhukumar <nandhukumare@gmail.com> * MOSIP-39047 Signed-off-by: Nandhukumar <nandhukumare@gmail.com> * MOSIP-39047 Signed-off-by: Nandhukumar <nandhukumare@gmail.com> * MOSIP-39047 Signed-off-by: Nandhukumar <nandhukumare@gmail.com> * MOSIP-39047 Signed-off-by: Nandhukumar <nandhukumare@gmail.com> * MOSIP-39047 Signed-off-by: Nandhukumar <nandhukumare@gmail.com> --------- Signed-off-by: Nandhukumar <nandhukumare@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
GOKULRAJ136
force-pushed
the
MOSIP-PII-new
branch
from
26a1516 to
c0c3c77
Compare
Signed-off-by: Gokulraj C <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Contributor
Author
|
@coderabbitai review the new changes |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pre-registration/pre-registration-application-service/pom.xml (1)
19-24:⚠️ Potential issue | 🟡 MinorSibling kernel/commons artifacts remain on 1.2.1-SNAPSHOT.
kernel-khazana,kernel-idobjectvalidator,kernel-templatemanager-velocity,kernel-qrcodegenerator-zxing,kernel-transliteration-icu4j, andkernel-pdfgenerator(line 167) are still1.2.1-SNAPSHOTwhile the BOM is now1.3.0. Please verify these artifacts are still published and binary-compatible with the 1.3.0 kernel's transitive dependency levels (Spring Boot 3.2.5, Jackson, Hibernate, etc.) to avoid runtime classpath issues. Root cause/verification script noted onpre-registration-core/pom.xml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/pom.xml` around lines 19 - 24, The Maven properties for sibling kernel artifacts (kernel.khazana.version, kernel.idobjectvalidator.version, kernel.templatemanager.velocity.version, kernel.qrcodegenerator.zxing.version, kernel.transliteration.icu4j.version and the kernel-pdfgenerator property referenced at line ~167) are still 1.2.1-SNAPSHOT while kernel.bom.version is 1.3.0; update these properties to the aligned 1.3.0 release (or to the correct published version that matches kernel.bom.version) and/or verify those artifacts are published and binary-compatible with the 1.3.0 BOM transitive deps (use the verification script referenced in pre-registration-core/pom.xml) so the module uses consistent kernel dependency versions.pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/RegistrationBookingEntityConsumed.java (1)
82-84:⚠️ Potential issue | 🟡 MinorStale Javadoc on
upBy.The comment on line 82 still says
/** Created by. */, but this field is the updated-by column (upd_by). While you updated thecrByJavadoc (line 74) to document the "legacy plaintext or user identifier" semantics, the correspondingupBycomment was missed and is also factually wrong.📝 Proposed fix
- /** Created by. */ + /** Updated by (legacy plaintext or user identifier) */ `@Column`(name = "upd_by") private String upBy;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/RegistrationBookingEntityConsumed.java` around lines 82 - 84, The Javadoc for the field upBy is stale and incorrectly says "Created by."; update the comment above the field upBy (mapped to column "upd_by") to describe it as the "updated-by" value and, like crBy, note it stores the legacy plaintext or user identifier semantics; ensure the new Javadoc mirrors the style and level of detail used for crBy so readers understand upBy represents who last updated the record.pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/DataSyncService.java (1)
182-194:⚠️ Potential issue | 🟠 MajorSame auth-context NPE risk still present in all
finally-block audit calls.The PR correctly guards
authUserDetails().getUserId()at Lines 438–449, but thefinallyblocks inretrieveAllPreRegIds(Lines 186/191),retrieveAllAppointmentsSyncV2(Lines 252/257),getPreRegistrationData(Lines 343/347),fetchPreRegistrationData(Lines 405/409), andstoreConsumedPreRegistrations(Lines 467/471) still dereferenceauthUserDetails().getUserId()/getUsername()directly. If the security principal is absent (same scenario that motivated the fix), these calls will NPE from insidefinallyand mask the original exception captured in thecatch, defeatingDataSyncExceptionCatcher.handle(...).Consider extracting a small helper and reusing it everywhere auth info is needed for audit:
🔧 Suggested helper + application
+ private String safeUserId() { + try { + AuthUserDetails u = authUserDetails(); + return (u != null && u.getUserId() != null) ? u.getUserId() : "user"; + } catch (Exception ex) { + return "user"; + } + } + + private String safeUserName() { + try { + AuthUserDetails u = authUserDetails(); + return (u != null && u.getUsername() != null) ? u.getUsername() : "user"; + } catch (Exception ex) { + return "user"; + } + }Then replace every
authUserDetails().getUserId()/authUserDetails().getUsername()in the fivefinallyblocks withsafeUserId()/safeUserName(). This also removes the duplicated try/catch added instoreConsumedPreRegistrations(Lines 438–449) and centralizes the fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/DataSyncService.java` around lines 182 - 194, Introduce a small safe accessor in DataSyncService (e.g., safeUserId() and safeUserName()) that checks authUserDetails() for null and returns a safe fallback (empty string or AuditLogVariables.NO_ID.toString()) instead of calling authUserDetails().getUserId()/getUsername() directly; then replace all direct dereferences in the finally blocks of retrieveAllPreRegIds, retrieveAllAppointmentsSyncV2, getPreRegistrationData, fetchPreRegistrationData, and storeConsumedPreRegistrations with these helpers and remove the duplicated try/catch added in storeConsumedPreRegistrations so finally-block audits cannot NPE and the original exception from DataSyncExceptionCatcher.handle(...) is preserved.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java (1)
184-194:⚠️ Potential issue | 🟡 MinorRedundant raw-
userIdassignment tocrBy/updBybefore it gets overwritten.Lines 185-186 set
crBy/updByto the rawuserId, then lines 193-194 overwrite them witheffectiveUserId. The dead assignments are harmless today but:
- They obscure intent and make it easy to accidentally reintroduce raw-PII persistence during a future refactor.
- If
resolveEffectiveUserIdis ever short-circuited/early-returned, the raw value survives.🔧 Proposed fix
documentEntity.setCrDtime(LocalDateTime.now(ZoneId.of("UTC"))); - documentEntity.setCrBy(userId); - documentEntity.setUpdBy(userId); documentEntity.setUpdDtime(LocalDateTime.now(ZoneId.of("UTC"))); String effectiveUserId = resolveEffectiveUserId(userId); log.info(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "Resolved effective user id for document write. preRegistrationId=" + preRegistrationId + ", maskedUserId=" + maskIdentifier(userId) + ", canonicalApplied=" + isCanonicalApplied(userId, effectiveUserId)); documentEntity.setCrBy(effectiveUserId); documentEntity.setUpdBy(effectiveUserId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java` around lines 184 - 194, Remove the redundant assignments of raw userId to documentEntity.crBy and documentEntity.updBy: call resolveEffectiveUserId(userId) first, then set documentEntity.setCrBy(effectiveUserId) and documentEntity.setUpdBy(effectiveUserId); keep setting documentEntity.setCrDtime(...) and setUpdDtime(...) as-is, and move the info log that references maskIdentifier(userId) and isCanonicalApplied(userId, effectiveUserId) to after resolveEffectiveUserId so the log and the final crBy/updBy reflect the effectiveUserId; relevant symbols: DocumentServiceUtil, resolveEffectiveUserId, documentEntity.setCrBy, documentEntity.setUpdBy, maskIdentifier, isCanonicalApplied, LOGGER_SESSIONID/LOGGER_IDTYPE/LOGGER_ID.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java (1)
124-158:⚠️ Potential issue | 🟡 MinorFix indentation and clarify access control semantics in the
getDemographicDatamethod.The null-check block (lines 130–132) has misaligned indentation that obscures the code structure—it should align with the subsequent
List<String> list = ...statement at line 133. Additionally, theelseblock at lines 152–155 throwsUNABLE_TO_FETCH_THE_PRE_REGISTRATIONwhen the caller lacksROLE_INDIVIDUAL, but this error message is misleading since the record was successfully found and the failure is actually due to insufficient permissions. Compare this to other methods in the same file (e.g., line ~210) which perform the role check without an explicitelsebranch—consider whether this method should throw a role-based error (e.g.,UNAUTHORIZEDorINVALID_ROLE) instead of a retrieval error, or align its pattern with the rest of the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java` around lines 124 - 158, In getDemographicData, fix the misaligned null-check block for demographicEntity so its if (demographicEntity == null) { ... } is aligned with the subsequent List<String> list = listAuth(...) line, and replace the misleading else branch that throws RecordNotFoundException when the caller lacks ROLE_INDIVIDUAL with proper access-control handling: either throw an authorization-specific exception (e.g., UNAUTHORIZED/INVALID_ROLE) or remove the explicit else and follow the existing file pattern (perform role check via listAuth(...) and let userValidation(...) enforce access), ensuring you reference the demographicEntity null-check, listAuth(authUserDetails().getAuthorities()), ROLE_INDIVIDUAL, and the current RecordNotFoundException/HashingException code paths when making the change.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.java (1)
403-413:⚠️ Potential issue | 🟡 MinorStub the authenticated user in this negative-path test.
deleteLostOrUpdateApplication(...)now callsuserDetailsService.matchesUser(authUserDetails().getUserId(), ...). In this test,applicationUser.getUserId()is never stubbed, so the mockedmatchesUser(...)lambda can hit a nullauthUserIdbefore the service throwsPreIdInvalidForUserIdException.💡 Minimal fix
SecurityContext securityContext = Mockito.mock(SecurityContext.class); Mockito.when(securityContext.getAuthentication()).thenReturn(authentication); SecurityContextHolder.setContext(securityContext); Mockito.when(SecurityContextHolder.getContext().getAuthentication().getPrincipal()).thenReturn(applicationUser); + Mockito.when(applicationUser.getUserId()).thenReturn("1234"); Mockito.when(serviceUtil.findApplicationById(Mockito.any())).thenReturn(applicationEntity); Mockito.when(validationUtil.requstParamValidator(Mockito.any())).thenReturn(true); assertNotNull(applicationService.deleteLostOrUpdateApplication(applicationId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.java` around lines 403 - 413, The test fails because deleteLostOrUpdateApplication calls userDetailsService.matchesUser(authUserDetails().getUserId(), ...) but the mocked AuthUserDetails applicationUser has no getUserId() stub and can be null; to fix, stub applicationUser.getUserId() to return a non-null id (e.g., the same user id on applicationEntity) before invoking applicationService.deleteLostOrUpdateApplication so userDetailsService.matchesUser receives a valid authUserId; keep the existing SecurityContextHolder principal setup and ensure the stub is applied to the mocked AuthUserDetails instance used in the test.
🧹 Nitpick comments (18)
pre-registration/pre-registration-application-service/pom.xml (2)
95-99: Drop the explicitkernel-coreversion or use the property.Since
kernel-bomis imported independencyManagement,kernel-core's version should come from the BOM automatically. Hardcoding1.3.0here duplicates the BOM version and will silently drift the next time the BOM is bumped. The parent POM already defines${kernel.core.version}if you prefer an explicit reference.♻️ Proposed change
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-core</artifactId> - <version>1.3.0</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/pom.xml` around lines 95 - 99, The POM currently hardcodes the kernel-core version in the dependency block (groupId io.mosip.kernel, artifactId kernel-core, <version>1.3.0</version>); remove the explicit <version> element so the version is resolved from the imported kernel-bom in dependencyManagement, or replace the hardcoded value with the parent property ${kernel.core.version} to keep it synced with the BOM.
245-249: Same hardcoded-version concern forkernel-auth-adapter.Prefer
${kernel.bom.version}(or a dedicated property) over a literal1.3.0, to keep the kernel version set managed from a single place.♻️ Proposed change
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-auth-adapter</artifactId> - <version>1.3.0</version> + <version>${kernel.bom.version}</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/pom.xml` around lines 245 - 249, The dependency for artifactId "kernel-auth-adapter" uses a hardcoded version "1.3.0"; change it to use the shared property (e.g. replace the <version>1.3.0</version> with <version>${kernel.bom.version}</version> or a dedicated property) so the kernel version is managed centrally; update any existing properties block if needed to define kernel.bom.version and ensure the dependency block in the POM references that property instead of the literal.pre-registration/pre-registration-datasync-service/pom.xml (1)
169-169: Avoid hardcoding1.3.0; reuse a property.Hardcoding the kernel release version here is prone to drift when the BOM is bumped. Prefer
${kernel.bom.version}(or a dedicatedkernel.auth.adapter.versionproperty) so all kernel versions stay aligned from a single source.♻️ Proposed change
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-auth-adapter</artifactId> - <version>1.3.0</version> + <version>${kernel.bom.version}</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-datasync-service/pom.xml` at line 169, Replace the hardcoded <version>1.3.0</version> in the pom entry with a property reference so the kernel adapter version follows the BOM; update the <version> element to use either ${kernel.bom.version} or a new property ${kernel.auth.adapter.version} (and add that property to the project properties if creating a new one) so the adapter version stays aligned with the BOM and can be centrally managed; locate the <version> element inside the dependency or plugin declaration that currently contains "1.3.0" to make the change.pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/DocumentEntity.java (1)
120-127: Method placement splits the field declaration block.
getEffectiveCrBy()/getEffectiveUpdBy()are injected between theupdDtimefield (line 118) and theencryptedDateTimefield (line 132), which breaks the field-grouping convention in this entity. Move them to the end of the class, after all field declarations, for readability.Also, because
@Dataalready generatesgetCrBy()/getUpdBy(), these methods are pure aliases. That's fine as a semantic marker for the canonical-user migration, but a one-line comment noting "pass-through; canonical resolution handled in service layer" would help future maintainers avoid duplicating or "enhancing" this getter with DB lookups.♻️ Suggested relocation
`@Column`(name = "upd_dtimes") private LocalDateTime updDtime; - public String getEffectiveCrBy() { - return this.crBy; - } - - public String getEffectiveUpdBy() { - return this.updBy; - } - /** * Encrypted Date Time */ @@ `@Column`(name = "doc_ref_id") private String refNumber; + + public String getEffectiveCrBy() { + return this.crBy; + } + + public String getEffectiveUpdBy() { + return this.updBy; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/DocumentEntity.java` around lines 120 - 127, The two methods getEffectiveCrBy() and getEffectiveUpdBy() are interrupting the field declaration block; relocate both methods to the end of the class after all field declarations (move them below encryptedDateTime and any remaining fields) so fields remain grouped; keep them as simple pass-through aliases to the Lombok-generated getCrBy()/getUpdBy(), and add a one-line comment above each method like "pass-through; canonical resolution handled in service layer" to clarify they are intentionally thin wrappers.pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/AvailibityEntity.java (1)
90-96: Move new methods below field declarations; consider documenting the pass-through semantics.Placing
getEffectiveCrBy()/getEffectiveUpdBy()between theupBy/updDatefields and theisDeleted/delTimefields splits the field block and hurts readability. Please relocate them to the bottom of the class (after all@Column-annotated fields).Also note: since Lombok
@Getteralready generatesgetCrBy()/getUpBy(), these new methods are currently pure aliases. If the intent is ever to resolve a canonical UUID through a resolver (as the Javadoc "returns canonical user id if present else legacy value" wording used inApplicationEntitysuggests), that logic belongs in the user/service layer rather than the JPA entity. Keeping the stub here is fine for backward-compat, but a brief inline comment clarifying "pass-through; canonicalization handled inUserDetailsService" would help future maintainers.♻️ Suggested relocation
/** Updated date time. */ `@Column`(name = "upd_dtimes") private LocalDateTime updDate; - public String getEffectiveCrBy() { - return this.crBy; - } - - public String getEffectiveUpdBy() { - return this.upBy; - } - /** Is deleted. */ `@Column`(name = "is_deleted") private boolean isDeleted; /** Delete date time. */ `@Column`(name = "del_dtimes") private LocalDateTime delTime; + public String getEffectiveCrBy() { + return this.crBy; + } + + public String getEffectiveUpdBy() { + return this.upBy; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/AvailibityEntity.java` around lines 90 - 96, The two methods getEffectiveCrBy() and getEffectiveUpdBy() are interrupting the field block (between upBy/updDate and isDeleted/delTime) and are mere pass-through aliases to Lombok-generated getCrBy()/getUpBy(); move these methods to the bottom of the class after all `@Column-annotated` fields to keep field declarations contiguous, and add a brief inline comment above each method stating that they are pass-through stubs (e.g., "pass-through; canonicalization handled in UserDetailsService") so future maintainers know canonical ID resolution happens in the user/service layer rather than the JPA entity.pre-registration/pre-registration-application-service/src/main/resources/application.properties (1)
216-216: Consider relocating the property for better grouping.
mosip.prereg.pii.backward.compatibilityis currently inserted in the middle of themosip.preregistration.booking.*block. Moving it next to othermosip.prereg.*/ PII-related settings (or into a dedicated section) would improve readability. Purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/resources/application.properties` at line 216, The property mosip.prereg.pii.backward.compatibility is placed inside the mosip.preregistration.booking.* block; move this property out of that booking block and relocate it next to other mosip.prereg.* or PII-related settings (or create a dedicated PII section) so it is grouped logically with related PII configuration keys for improved readability and maintenance.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java (1)
187-188: Minor optimization: uselog.isDebugEnabled()to avoid unnecessary masking work.Good use of
maskIdentifier()to protect the user ID in debug logs. One optimization worth considering:The masking computation runs unconditionally, even when debug logging is disabled. Wrap the log statement with
log.isDebugEnabled()to skip the work on hot paths where debug level is not active.(Note:
maskIdentifier()is null-safe and already handles null/blank values gracefully, and the@Validatedannotation on the method parameter ensures the request DTO structure is validated before method entry, so no additional null-checks are needed.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.java` around lines 187 - 188, The debug log in LoginController currently always calls userDetailsService.maskIdentifier(userIdOtpRequest.getRequest().getUserId()), causing unnecessary work when debug logging is off; wrap that log invocation in a log.isDebugEnabled() check so the masking is only performed when debug is enabled, referencing the existing log, maskIdentifier, userDetailsService, and userIdOtpRequest symbols.pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/RegistrationBookingEntity.java (1)
91-97:getEffectiveCrBy/getEffectiveUpdByare currently indistinguishable from the Lombok-generated getters.Because the class is annotated with
@Data,getCrBy()/getUpBy()already exist and return the exact same value. If the intent is to later branch on the PII backward-compatibility flag (e.g., resolve a UUID to a masked/plaintext identifier depending on mode), that logic is missing here — the methods are currently pure pass-throughs and add no behavior overgetCrBy()/getUpBy(). The same pattern is repeated inDemographicEntityandInterfaceDataSyncEntity.Please confirm whether these accessors are intentional intent-documenting facades (in which case a brief Javadoc explaining why callers should prefer them would help), or whether the resolution/masking logic was meant to live here and was inadvertently omitted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/RegistrationBookingEntity.java` around lines 91 - 97, The getEffectiveCrBy and getEffectiveUpdBy methods in RegistrationBookingEntity are no-ops duplicating Lombok getters; either document them as intentional facades or implement the missing PII-resolution logic. Update RegistrationBookingEntity (and mirror changes in DemographicEntity and InterfaceDataSyncEntity) to either 1) add a brief Javadoc to getEffectiveCrBy/getEffectiveUpdBy stating callers should use these facades (so they can change behavior later), or 2) implement the intended logic: check the PII backward-compatibility flag or service (e.g., the existing configuration/method used elsewhere like isPiiBackwardCompatibilityEnabled() or a PiiResolver service) and return either the raw creator/updater id or the masked/resolved identifier accordingly; ensure you reference and reuse the same flag/service used elsewhere in the codebase so behavior is consistent.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java (1)
109-110: Consider a default forpiiBackwardCompatibility.
@Value("${mosip.prereg.pii.backward.compatibility}")without a default will fail context startup if the property is missing from any environment. Adding a safe default (e.g.,:false) makes new deployments opt-in to backward compatibility explicitly.- `@Value`("${mosip.prereg.pii.backward.compatibility}") + `@Value`("${mosip.prereg.pii.backward.compatibility:false}") private boolean piiBackwardCompatibility;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java` around lines 109 - 110, The field piiBackwardCompatibility in DocumentServiceUtil uses `@Value`("${mosip.prereg.pii.backward.compatibility}") with no default which will break context startup if the property is absent; update the annotation to supply a safe default (for example change to `@Value`("${mosip.prereg.pii.backward.compatibility:false}") or equivalent) so the boolean defaults to false when the property is missing and deployments only opt into backward compatibility explicitly.pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/entity/ProcessedPreRegEntity.java (1)
89-108: Move thegetEffective*methods below the field declarations.
isDeletedanddelTimefields (lines 101-108) are declared after the newgetEffectiveCrBy/getEffectiveUpdBymethods, splitting field declarations with method bodies. Relocate the two methods to follow all field declarations for cleaner structure. This also matches how Lombok typically expects class layout when used with@Getter/@Setter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/entity/ProcessedPreRegEntity.java` around lines 89 - 108, In ProcessedPreRegEntity move the two accessor methods getEffectiveCrBy and getEffectiveUpdBy so they appear after all field declarations (including isDeleted and delTime) instead of between fields; locate the methods named getEffectiveCrBy and getEffectiveUpdBy and cut/paste them to below the field declarations (e.g., after the delTime field) to keep fields grouped together and match the expected class layout.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DocumentServiceTest.java (1)
164-180: Inconsistent indentation insetUp.Line 164 starts at column 0 while lines 165-180 use tab indentation. Not functionally wrong, but inconsistent with the rest of the method and will cause diff noise. Consider reindenting this block to match the surrounding code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DocumentServiceTest.java` around lines 164 - 180, The DocumentEntity setup block is mis-indented (line starting with documentEntity = ... is at column 0) causing inconsistent formatting; open the setUp method and reindent the entire block that initializes documentEntity (all calls to documentEntity.set...) to match the surrounding method indentation style (use the same tabs/spaces as other lines in setUp) so the formatter/linters and diffs remain clean; ensure you update only whitespace so no logic changes in the DocumentEntity initialization occur.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/RegAppointmentRepository.java (1)
13-14: Inconsistent parameter binding: positional?1mixed with@Param("preRegId").The JPQL uses positional binding
?1while the method argument is annotated with@Param("preRegId"). With positional binding,@Paramis effectively unused. Pick one style:🔧 Option A — named binding (recommended with `@Param`)
- `@Query`("SELECT e FROM RegistrationBookingEntity e WHERE e.preregistrationId = ?1") - RegistrationBookingEntity getRegistrationAppointmentByPreRegistrationId(`@Param`("preRegId") String preRegId); + `@Query`("SELECT e FROM RegistrationBookingEntity e WHERE e.preregistrationId = :preRegId") + RegistrationBookingEntity getRegistrationAppointmentByPreRegistrationId(`@Param`("preRegId") String preRegId);🔧 Option B — positional binding (drop `@Param`)
- RegistrationBookingEntity getRegistrationAppointmentByPreRegistrationId(`@Param`("preRegId") String preRegId); + RegistrationBookingEntity getRegistrationAppointmentByPreRegistrationId(String preRegId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/RegAppointmentRepository.java` around lines 13 - 14, The `@Query` on RegistrationBookingEntity/getRegistrationAppointmentByPreRegistrationId mixes positional binding (?1) with a named `@Param`("preRegId"); fix by using a single style—recommended: change the JPQL to use a named parameter (:preRegId) and keep the `@Param`("preRegId") on the method parameter (or alternatively remove `@Param` and the `@Query` should remain positional); update the `@Query` string to "SELECT e FROM RegistrationBookingEntity e WHERE e.preregistrationId = :preRegId" ensuring the method parameter name and `@Param` match.pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/util/DataSyncServiceUtil.java (1)
927-927: RedundantOptional != nullcheck.By contract,
Optionalitself should never benull. IfUserDetailsService.resolveUserUuidreturnsnull, that is a stronger API smell to address at the source. Either trust the Optional contract (drop the null check) or treatnullexplicitly like empty — currently the mix is misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/util/DataSyncServiceUtil.java` at line 927, The if condition mixes a redundant null check with an Optional; remove the null check and rely on the Optional contract: change the branch that currently checks "userUuid != null && userUuid.isPresent()" to only check "userUuid.isPresent()" (or better use userUuid.ifPresent(...) or userUuid.orElse(...) as appropriate) where userUuid is the result from UserDetailsService.resolveUserUuid in DataSyncServiceUtil; this eliminates the misleading null handling and keeps Optional semantics consistent.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java (1)
237-239: Avoid raw(Collection)cast — produces unchecked-conversion warnings across all affected tests.The cast
(Collection) authorities(repeated at lines 284, 334, 378, 422) drops the type parameter onAuthUserDetails.getAuthorities(). UsedoReturnor an explicit parameterized type to avoid unchecked warnings:🔧 Option — use `doReturn` to sidestep the wildcard variance issue
- List<GrantedAuthority> authorities = new ArrayList<>(); - authorities.add(new SimpleGrantedAuthority("ROLE_INDIVIDUAL")); - when(applicationUser.getAuthorities()).thenReturn((Collection) authorities); + List<GrantedAuthority> authorities = new ArrayList<>(); + authorities.add(new SimpleGrantedAuthority("ROLE_INDIVIDUAL")); + Mockito.doReturn(authorities).when(applicationUser).getAuthorities();Applies to lines 239, 284, 334, 378, 422.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java` around lines 237 - 239, The tests use an unchecked raw cast when stubbing applicationUser.getAuthorities() (e.g., when(applicationUser.getAuthorities()).thenReturn((Collection) authorities)) which triggers warnings; replace those stubs by using doReturn(authorities).when(applicationUser).getAuthorities() (or return a properly parameterized Collection<GrantedAuthority>) for each occurrence (the stubs around applicationUser.getAuthorities() in AppointmentServiceImplTest) to eliminate the unchecked-conversion warnings.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationIdentityMigrationServiceTest.java (2)
14-14: Redundant self-package import.
ApplicationIdentityMigrationServiceis in the same package (io.mosip.preregistration.application.service) as this test class, so the explicit import on Line 14 is unnecessary and can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationIdentityMigrationServiceTest.java` at line 14, Remove the redundant self-package import of ApplicationIdentityMigrationService from the test; since ApplicationIdentityMigrationService is declared in the same package io.mosip.preregistration.application.service as ApplicationIdentityMigrationServiceTest, delete the import statement referencing ApplicationIdentityMigrationService to avoid an unnecessary import.
118-128: Add coverage for the backward-compatibility branch ofresolveEffectiveUserId.
resolveEffectiveUserIdUsesCanonicalUuidInStrictModeonly exercisespiiBackwardCompatibility = false. Please add a companion test that sets the flag totrueand verifies thatresolveEffectiveUserIddelegates touserDetailsService.resolveUserUuidOrIdentifier(...)(and does not hard-fail whenresolveUserUuidreturnsOptional.empty()), so both configured modes are regression-protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationIdentityMigrationServiceTest.java` around lines 118 - 128, Add a new test in ApplicationIdentityMigrationServiceTest that enables the backward-compatibility mode on the ApplicationIdentityMigrationService (set piiBackwardCompatibility = true via the existing setter/constructor/config used in tests), mock userDetailsService.resolveUserUuid(userId) to return Optional.empty(), mock userDetailsService.resolveUserUuidOrIdentifier(userId) to return a fallback identifier, call applicationIdentityMigrationService.resolveEffectiveUserId(userId), assert the returned value equals the fallback identifier, and verify that userDetailsService.resolveUserUuidOrIdentifier(...) was invoked (and the flow did not hard-fail when resolveUserUuid returned Optional.empty()).pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java (1)
480-487: Initial raw-id assignment on Line 480 is dead code; drop it to avoid confusion.
applicationEntity.setUpdBy(authUserDetails().getUserId())at Line 480 is unconditionally overwritten byapplicationEntity.setUpdBy(resolveEffectiveUserId(...))at Line 483 before the entity is saved. Keeping the raw assignment is misleading (it implies a fallback that never applies) and was presumably left over from a prior iteration. Also, similar toDemographicService, a failure insidemigrateRawUserToEffectiveUserat Line 485–486 will now throwAppointmentExecptioneven though theApplicationEntityhas already been persisted — consider whether migration errors should invalidate the whole appointment update, or be isolated/logged.♻️ Proposed cleanup
- applicationEntity.setUpdBy(authUserDetails().getUserId()); applicationEntity.setCrDtime(LocalDateTime.now(ZoneId.of("UTC"))); try { applicationEntity.setUpdBy(resolveEffectiveUserId(authUserDetails().getUserId())); ApplicationEntity savedApplicationEntity = applicationRepostiory.save(applicationEntity); applicationIdentityMigrationService.migrateRawUserToEffectiveUser(preRegistrationId, savedApplicationEntity.getEffectiveUpdBy()); return savedApplicationEntity;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java` around lines 480 - 487, Remove the dead initial assignment applicationEntity.setUpdBy(authUserDetails().getUserId()) since it is immediately overwritten by applicationEntity.setUpdBy(resolveEffectiveUserId(...)) before save; instead keep only the resolveEffectiveUserId call prior to applicationRepostiory.save. Additionally, wrap the call to applicationIdentityMigrationService.migrateRawUserToEffectiveUser(preRegistrationId, savedApplicationEntity.getEffectiveUpdBy()) in a try/catch so that migration failures do not abort a successfully persisted ApplicationEntity—log the exception (using the existing logger) and decide whether to retry or surface a non-fatal alert rather than throwing AppointmentExecption, mirroring the non-fatal handling used in DemographicService.pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java (1)
142-214: Consider caching per-item owner UUID resolutions to reduce repository round-trips.
resolveUserUuidis invoked per record/document forgetCrAppuserId(),getEffectiveCreatedBy(), andgetEffectiveCrBy()on eachdemoEntity/applicantDoc/regAppointmentObj. In practice, these are typically the same end-user identifier for a given pre-reg item, so you may be performing 2-4× redundant lookups per item. SinceauditUserIdis already resolved once, you can similarly memoize the per-item creator withinupdateConsumedStatus(e.g., resolvedemoEntity.getCrAppuserId()/getEffectiveCreatedBy()once before calling the helpers and pass the resolved value, falling back to per-doc resolution only when the raw creator differs). Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java` around lines 142 - 214, The code repeatedly calls resolveUserUuid per-item inside addApplicantDemographicConsumed, addApplicantDocumentConsumed, and addRegistrationAppointmentConsumed causing redundant repository lookups; modify updateConsumedStatus to resolve the item's creator once (resolve demoEntity.getCrAppuserId(), demoEntity.getEffectiveCreatedBy(), and regAppointmentObj.getEffectiveCrBy()/applicantDoc.getEffectiveCrBy() as applicable) store them in a local map or variables, and pass the resolved creator UUIDs into the helper methods (addApplicantDemographicConsumed, addApplicantDocumentConsumed, addRegistrationAppointmentConsumed) as additional parameters; inside those methods use the passed resolved values (falling back to calling resolveUserUuid only if the passed value is null or different) and remove the per-field resolveUserUuid calls to eliminate duplicate lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c186525-90ee-437c-965c-49fcd2c0c201
📒 Files selected for processing (53)
db_scripts/mosip_prereg/ddl.sqldb_scripts/mosip_prereg/ddl/user_details.sqlpre-registration/pom.xmlpre-registration/pre-registration-application-service/pom.xmlpre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/controller/LoginController.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/ApplicationRepostiory.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/RegAppointmentRepository.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationIdentityMigrationService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DocumentService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.javapre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.javapre-registration/pre-registration-application-service/src/main/resources/application.propertiespre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationIdentityMigrationServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/ApplicationServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DemographicServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DocumentServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/LoginServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/OTPManagerTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/controller/LoginControllerTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/AppointmentServiceTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DemographicServiceUtilTest.javapre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.javapre-registration/pre-registration-application-service/src/test/resources/application.propertiespre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/AvailibityEntity.javapre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/entity/RegistrationBookingEntityConsumed.javapre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.javapre-registration/pre-registration-core/pom.xmlpre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/ApplicationEntity.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/DemographicEntity.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/DocumentEntity.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/RegistrationBookingEntity.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/UserDetails.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/repository/UserDetailsRepository.javapre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.javapre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.javapre-registration/pre-registration-datasync-service/pom.xmlpre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/entity/InterfaceDataSyncEntity.javapre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/entity/ProcessedPreRegEntity.javapre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/DataSyncService.javapre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/util/DataSyncServiceUtil.javapre-registration/pre-registration-datasync-service/src/main/resources/application.propertiespre-registration/pre-registration-datasync-service/src/test/java/io/mosip/preregistration/datasync/test/config/TestSecurityConfig.javapre-registration/pre-registration-datasync-service/src/test/java/io/mosip/preregistration/datasync/test/service/DataSyncServiceTest.javapre-registration/pre-registration-datasync-service/src/test/java/io/mosip/preregistration/datasync/test/service/util/DataSyncServiceUtilTest.javapre-registration/pre-registration-datasync-service/src/test/resources/application.properties
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
…harden UserLookupException handling across services Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
mayuradesh
approved these changes
Jun 5, 2026
Signed-off-by: Gokulraj C <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: Gokulraj C <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores