Draft
Conversation
* PII Issue Fixes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fix toString issues Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Added UserValidation to applicant service Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * UUID Changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Cache changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed appointment booking validation error Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed UUID Issues Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed demographic retrieve details issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed get all applications API Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed processed_prereg_list table cr_by data issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixes nexus build issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fix captcha service nexus build Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixes nexus build for batchjob service Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * "Fixes nexus build issue for all pom's" Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Parent pom fixes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert Pom changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert pom changes for nexus build Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert parent pom changes 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>
Updated user validation paths
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
* Revert "Updated user validation paths" Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Added user_details.sql Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> --------- Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
WalkthroughAdds a canonical user identity registry (DB + JPA entity + service) and integrates it across services to resolve/mask user identifiers, update audit fields to canonical IDs, and adjust authorization checks; includes tests, DDL inclusion, property flags, and related repo/query updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User as Authenticated User
participant Service as PreReg Service
participant UDS as UserDetailsService
participant Repo as UserDetailsRepository
participant Crypto as CryptoUtil
participant DB as Database
User->>Service: request (e.g., create/update)
Service->>UDS: findOrCreateByIdentifier(authUserId)
UDS->>UDS: normalize & hash identifier
UDS->>Repo: findByIdentifierHash(hash)
Repo->>DB: query user_details
alt found
DB-->>Repo: user_details row
Repo-->>UDS: UserDetails
UDS-->>Service: canonical UUID
else not found
DB-->>Repo: empty
UDS->>Crypto: encrypt(identifier)
Crypto-->>UDS: encryptedPayload
UDS->>Repo: save(new UserDetails with UUID, hash, encryptedPayload)
Repo->>DB: insert row
Repo-->>UDS: saved entity
UDS-->>Service: canonical UUID
end
Service->>Service: use canonical UUID for crBy/updBy and authorization checks
Service->>DB: persist application/demographic/booking
DB-->>Service: ack/response
Service-->>User: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (5)
pre-registration/pre-registration-datasync-service/src/test/java/io/mosip/preregistration/datasync/test/service/DataSyncServiceTest.java (1)
338-339: Tighten this stub to validate the new user-id behavior.Using
Mockito.any()for the third arg hides regressions in the exactcreatedByvalue passed fromDataSyncService.Proposed test assertion strengthening
- when(serviceUtil.reverseDateSyncSave(Mockito.any(), Mockito.any(), Mockito.any())) + when(serviceUtil.reverseDateSyncSave(Mockito.any(), Mockito.any(), Mockito.eq("testUser"))) .thenReturn(reverseDatasyncReponse);Mockito.verify(serviceUtil).reverseDateSyncSave(Mockito.any(), Mockito.any(), Mockito.eq("testUser"));🤖 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/test/java/io/mosip/preregistration/datasync/test/service/DataSyncServiceTest.java` around lines 338 - 339, The test currently stubs serviceUtil.reverseDateSyncSave with Mockito.any() for the createdBy argument which masks regressions in the user-id; update the test in DataSyncServiceTest to assert the exact createdBy value by replacing the third Mockito.any() with Mockito.eq("testUser") in the stub or add a post-call verification like Mockito.verify(serviceUtil).reverseDateSyncSave(Mockito.any(), Mockito.any(), Mockito.eq("testUser")) so DataSyncService passes the expected user id.db_scripts/mosip_prereg/ddl/user_details.sql (1)
16-25: Remove redundant index onidentifier_hash.The
UNIQUEconstraint on line 16 automatically creates a backing index in PostgreSQL. The explicit index at lines 24-25 duplicates it and adds unnecessary disk and write-performance overhead.Proposed simplification
-CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_prereg_user_details_hash -ON prereg.user_details(identifier_hash);🤖 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 16 - 25, Remove the redundant explicit index idx_prereg_user_details_hash on prereg.user_details(identifier_hash) because the UNIQUE constraint on column identifier_hash already creates the necessary backing index; delete the CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_prereg_user_details_hash ... statement so you only rely on the UNIQUE constraint for lookups and avoid duplicate index overhead.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.java (1)
142-151: Extract canonical audit-user resolution into one helper.The mapping/fallback block is duplicated. Consolidating it will keep behavior consistent and reduce maintenance risk.
♻️ Proposed refactor
+ private String resolveCanonicalAuditUserId() { + String clientIdentifier = environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID); + try { + UserDetails mappedUser = userDetailsService.findOrCreateByIdentifier(clientIdentifier); + if (mappedUser != null && mappedUser.getUserId() != null) { + return mappedUser.getUserId().toString(); + } + } catch (Exception e) { + logger.warn(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, + "UserDetails mapping failed for OTP audit identity", e); + } + return clientIdentifier; + }- otpTxn.setUpdBy(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); - 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(resolveCanonicalAuditUserId());- txn.setCrBy(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)); // Map the client id to canonical user id, and store canonical id into cr_by - 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(resolveCanonicalAuditUserId());Also applies to: 162-173
🤖 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 142 - 151, Extract the duplicated user-to-audit-user resolution in OTPManager into a single private helper (e.g., resolveAuditUserId or getAuditUserId) that encapsulates calling userDetailsService.findOrCreateByIdentifier(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)), returning mappedUser.getUserId().toString() when available and falling back to environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID); replace both duplicated blocks (the one setting otpTxn.setUpdBy at lines ~142-151 and the similar block at ~162-173) with a single call to otpTxn.setUpdBy(resolveAuditUserId()); preserve the existing catch behavior by logging via logger.warn(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "UserDetails mapping failed for otp update", e) inside the helper.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java (1)
292-296: Remove redundantcrBy/updByassignments in copy path.Lines 292-293 are immediately overwritten by Lines 295-296; keep only the effective assignments.
♻️ 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 - 296, In DocumentServiceUtil remove the redundant assignments that set crBy/updBy directly before they are overwritten: in the copy path delete the calls copyDocumentEntity.setCrBy(sourceEntity.getCrBy()) and copyDocumentEntity.setUpdBy(sourceEntity.getUpdBy()), leaving only the effective-value assignments copyDocumentEntity.setCrBy(sourceEntity.getEffectiveCrBy()) and copyDocumentEntity.setUpdBy(sourceEntity.getEffectiveUpdBy()) to avoid redundant writes.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java (1)
319-325: Extract canonical user resolution into one helper.The same mapping logic appears in multiple methods; centralizing it will reduce drift and make fallback behavior consistent.
Also applies to: 723-729, 755-758
🤖 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 319 - 325, Extract the repeated "resolve canonical user id" logic into a single private helper (e.g., resolveCanonicalUserId or getCanonicalUserId) that calls userDetailsService.findOrCreateByIdentifier(userId), checks mappedUser and mappedUser.getUserId(), and returns the canonicalUserId (or null/Optional). Replace the duplicated blocks in DemographicServiceUtil (locations around the existing code that set demographicEntity.setCrAppuserId, setCreatedBy, setUpdatedBy and the other occurrences at the noted ranges) to call this helper and then set the demographicEntity fields only when the helper returns a non-null/non-empty id to preserve existing fallback behavior.
🤖 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 24-29: The two CREATE INDEX CONCURRENTLY statements for
idx_prereg_user_details_hash and idx_prereg_user_details_active on
prereg.user_details cannot run inside the implicit transaction used by psql -f
and will error; fix by either (A) removing the CONCURRENTLY keyword so the index
creation runs inside the script transaction, or (B) execute those specific
CREATE INDEX CONCURRENTLY statements outside the transaction (e.g., separate
psql -c invocations during deployment) so they run independently of the main
ddl.sql transaction block.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/ApplicationService.java`:
- Around line 502-514: The code maps a passed userId to a canonical UUID via
userDetailsService.findOrCreateByIdentifier but then queries only by
canonicalUserId (applicationRepository.findByCreatedBy), which will miss legacy
rows stored with the raw identifier; update the lookup in ApplicationService to
query for both identifiers (canonical and original) when mapping succeeds —
e.g., call a repository method that accepts both values (findByCreatedByIn or a
findByCreatedByOrCreatedByRaw style) or add an OR clause so you search by
canonicalUserId and userId; apply the same change to the other similar block
(the block around 626-639) so both places return legacy and canonical records.
- Around line 438-440: The log statements in ApplicationService that currently
include raw user identifiers (e.g., the log.warn call that logs "Failed to map
auth user to canonical UUID: " + authUserId and the other occurrences around the
mapping and failure paths) must stop printing raw authUserId; replace the raw
value with a sanitized representation (mask or irreversible hash). Implement or
call a utility such as maskSensitive(String) or hashIdentifier(String) and use
that in the log calls (e.g., replace authUserId with maskSensitive(authUserId))
in the ApplicationService methods where the three occurrences exist so logs
contain only non-reversible/sanitized IDs while preserving traceability.
- Around line 429-442: The current check trims without null-safety and rejects
legacy/null creator values; update the condition around
authUserId/canonicalAuthUserId and applicationEntity.getEffectiveCrBy() to be
null-safe and allow legacy/null creators: compute safe strings (e.g.,
safeCanonical = canonicalAuthUserId == null ? null : canonicalAuthUserId.trim();
safeCreator = applicationEntity.getEffectiveCrBy() == null ? null :
applicationEntity.getEffectiveCrBy().trim()) and only throw
PreIdInvalidForUserIdException when safeCanonical is non-null and does not equal
safeCreator (handle case-insensitive if desired); apply same null-safe logic in
the other occurrence around lines 570-583 (same methods/variables: authUserId,
canonicalAuthUserId, applicationEntity.getEffectiveCrBy()).
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/AppointmentServiceImpl.java`:
- Around line 186-189: Guard against null before calling trim on
applicationEntity.getCrBy() and relax the strict canonical-only comparison: in
AppointmentServiceImpl compute String expectedCrBy =
applicationEntity.getCrBy(); first check expectedCrBy != null before invoking
expectedCrBy.trim(); then change the failing condition so you only throw
AppointmentExecption when expectedCrBy is non-null and the trimmed value does
not equal the trimmed canonicalAuthUserId; for legacy records where expectedCrBy
is null or not a UUID, implement a fallback comparison (e.g., compare the raw
legacy id token against canonicalAuthUserId in its normalized form or accept
legacy by skipping the canonical check) so valid legacy records are not rejected
while still throwing AppointmentExecption using
AppointmentErrorCodes.INVALID_APP_ID_FOR_USER only when both checks definitively
fail.
- Around line 480-482: In AppointmentServiceImpl, do not swallow the retry save
exception in the catch(Exception ex2) block; instead capture ex2 details by
logging them (e.g., using the class logger) and rethrow or wrap ex2 into a
meaningful exception so the caller can see the failure; update the catch block
around the retry save logic to log ex2 with context (method name, retry attempt)
and then throw the original or a new checked/unchecked exception that preserves
ex2 as the cause.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java`:
- Around line 874-875: The log statements in
DemographicService#getDemographicData are outputting raw identifiers
(preregUserId, authUserId) — replace those direct uses with a sanitized form
(e.g., masked or hashed) before logging; create or call a small utility (e.g.,
maskIdentifier or sanitizeId) and use it in the log calls that reference
LOGGER_SESSIONID/LOGGER_IDTYPE/LOGGER_ID at the points around the current
message and the other occurrences flagged (the blocks around lines showing
preregUserId/authUserId at 874, 885-887, 913-915) so logs contain non-sensitive
masked identifiers instead of raw values.
- Around line 895-905: The code currently calls
userDetailsService.findOrCreateByIdentifier(trimmedPreregUserId) even when
trimmedPreregUserId is empty, which can create invalid mappings; in
DemographicService before calling userDetailsService.findOrCreateByIdentifier,
guard against blank/empty trimmedPreregUserId (use String.isBlank()/isEmpty())
and only attempt the lookup/mapping when it contains a non-blank value,
otherwise skip the try block so no empty identifier is passed to
findOrCreateByIdentifier; keep existing comparison to trimmedCanonicalAuthUserId
and only invoke mapping when trimmedPreregUserId is non-blank and not equal to
trimmedCanonicalAuthUserId.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/CommonServiceUtil.java`:
- Around line 200-210: The code in CommonServiceUtil calls
userDetailsService.findOrCreateByIdentifier(trimmedPreregUserId) during an
authorization/read path which can create canonical user rows from blank or
untrusted prereg IDs; change this to avoid mutation by (1) adding a guard to
only attempt mapping when trimmedPreregUserId is non-empty and passes validation
(e.g., matches expected identifier format) and also is demonstrably different
from trimmedCanonicalAuthUserId, and (2) replace the mutating call to
findOrCreateByIdentifier with a read-only lookup (e.g., a
findByIdentifier/findIfExists method) on userDetailsService or implement a new
read-only method, ensuring only non-blank, validated prereg IDs invoke that
lookup so no new user_details rows are created from untrusted input.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java`:
- Around line 317-328: The current catch block in DemographicServiceUtil (around
userDetailsService.findOrCreateByIdentifier) falls back to writing the raw
userId into persisted fields (demographicEntity.setCrAppuserId, setCreatedBy,
setUpdatedBy), reintroducing plaintext PII; change the fallback so that on
exception you do NOT write the raw userId but instead set those fields to a
non-PII-safe value (null or a system placeholder/opaque surrogate) or leave them
unset and rely on downstream processes to populate a canonical id; update the
catch logic for the other similar blocks (the ones around lines 721-732 and
753-761) to follow the same pattern and ensure logging still records the failure
but never persists the raw identifier.
In
`@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java`:
- Around line 111-114: The current null check after calling
normalize(identifier) allows strings made only of whitespace to pass; update the
validation in UserDetailsService (after calling normalize(identifier)) to reject
blank values too by throwing IllegalArgumentException when norm is null or blank
(e.g., use norm.isBlank() or a StringUtils.isBlank check) so whitespace-only
identifiers are not hashed/persisted.
- Around line 116-133: findOrCreateByIdentifier is racy: multiple threads can
pass the find-then-save path for the same identifierHash causing duplicate
entries; make it safe by relying on a DB unique constraint on
UserDetails.identifierHash and converting the method to an
upsert/try-insert-with-retry pattern. Specifically, ensure UserDetails has a
unique constraint on identifierHash, then change the logic in
findOrCreateByIdentifier to (a) try to create and save a new UserDetails (using
UUID.randomUUID(),
setIdentifierEncrypted(encryptIdentifierIfConfigured(identifier)),
setIdentifierHash(hash)) and return it, (b) if save throws a
duplicate-key/DataIntegrityViolationException catch it, then reload the existing
UserDetails via userDetailsRepository.findByIdentifierHash(hash) and proceed to
update identifierEncrypted if missing (use encryptIdentifierIfConfigured) and
save; for updating the existing record consider wrapping update/save in a
transaction to avoid lost updates. Use the repository methods
userDetailsRepository.findByIdentifierHash and userDetailsRepository.save and
the model UserDetails as the referenced symbols.
- Around line 65-69: encryptIdentifierIfConfigured() is currently converting
ciphertext bytes to a UTF-8 String and decryptIdentifierIfConfigured() is
converting the stored String back to bytes via getBytes(UTF_8), which is unsafe
for arbitrary ciphertext; update encryptIdentifierIfConfigured() to return
Base64.getEncoder().encodeToString(encrypted) after cryptoUtil.encrypt(...) and
update decryptIdentifierIfConfigured() to decode the stored value with
Base64.getDecoder().decode(encryptedValue) before passing to
cryptoUtil.decrypt(...), ensuring you change only the string/byte conversions
and keep the existing calls to cryptoUtil.encrypt/cryptoUtil.decrypt and the
surrounding null/empty checks.
- Around line 65-66: The encryption/decryption timestamp is mismatched because
UserDetails uses LocalDateTime.now() for both operations; update the UserDetails
flow to persist and reuse the same timestamp used at encryption instead of
generating a new one: add an encryptedDateTime (or similar) field to the
UserDetails entity, set it when calling cryptoUtil.encrypt (the call in
UserDetailsService where byte[] encrypted = cryptoUtil.encrypt(...,
LocalDateTime.now())), persist that timestamp with the entity, and then pass
that persisted encryptedDateTime into cryptoUtil.decrypt (the call in
UserDetailsService where byte[] plain = cryptoUtil.decrypt(...,
LocalDateTime.now())); alternatively, ensure the exact timestamp instance
created during encryption is stored and reused for the corresponding decrypt
call.
In
`@pre-registration/pre-registration-core/src/test/java/io/mosip/preregistration/core/common/service/UserDetailsServiceTest.java`:
- Around line 51-56: The test testFindByIdentifierDelegatesToRepo should also
assert the returned Optional value: after stubbing
userDetailsRepository.findByIdentifierHash(...) to return Optional.of(mock) and
calling userDetailsService.findByIdentifier("TestUser"), add an assertion that
the returned Optional equals Optional.of(mock) (or that res.isPresent() and
res.get() == mock) to ensure delegation returns the repository value; update the
test to use the same mock UserDetails instance and verify both the repository
interaction and the actual returned Optional from
userDetailsService.findByIdentifier.
In
`@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/DataSyncService.java`:
- Around line 438-439: The call in DataSyncService that passes
authUserDetails().getUserId() into serviceUtil.reverseDateSyncSave can NPE when
the security context/principal is absent; update the code to first safely
resolve the authenticated user id (e.g., check
SecurityContextHolder.getContext() / authUserDetails() for null or use an
Optional) and provide a sensible fallback (such as a configured SYSTEM_USER_ID
or null) before calling reverseDateSyncSave with
reverseDataSyncRequest.getRequesttime(), reverseDataSyncRequest.getRequest(),
and the safe user id; ensure the fallback path is clearly audible in logs so
internal/system-triggered flows continue without throwing.
In
`@pre-registration/pre-registration-datasync-service/src/main/java/io/mosip/preregistration/datasync/service/util/DataSyncServiceUtil.java`:
- Around line 899-901: The warn log in DataSyncServiceUtil.reverseDateSyncSave
is printing the raw userId which can leak PII; change the log call to avoid
including the raw identifier (e.g., remove the concatenated userId, or replace
it with a non-reversible safe token such as a masked/truncated value or a hash)
and pass that safe value to log.warn along with the exception (keep
LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID). Ensure you do not construct the
message by concatenating the raw userId anywhere in reverseDateSyncSave.
---
Nitpick comments:
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Around line 16-25: Remove the redundant explicit index
idx_prereg_user_details_hash on prereg.user_details(identifier_hash) because the
UNIQUE constraint on column identifier_hash already creates the necessary
backing index; delete the CREATE INDEX CONCURRENTLY IF NOT EXISTS
idx_prereg_user_details_hash ... statement so you only rely on the UNIQUE
constraint for lookups and avoid duplicate index overhead.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/OTPManager.java`:
- Around line 142-151: Extract the duplicated user-to-audit-user resolution in
OTPManager into a single private helper (e.g., resolveAuditUserId or
getAuditUserId) that encapsulates calling
userDetailsService.findOrCreateByIdentifier(environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID)),
returning mappedUser.getUserId().toString() when available and falling back to
environment.getProperty(PreRegLoginConstant.MOSIP_PRE_REG_CLIENTID); replace
both duplicated blocks (the one setting otpTxn.setUpdBy at lines ~142-151 and
the similar block at ~162-173) with a single call to
otpTxn.setUpdBy(resolveAuditUserId()); preserve the existing catch behavior by
logging via logger.warn(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "UserDetails
mapping failed for otp update", e) inside the helper.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java`:
- Around line 319-325: Extract the repeated "resolve canonical user id" logic
into a single private helper (e.g., resolveCanonicalUserId or
getCanonicalUserId) that calls
userDetailsService.findOrCreateByIdentifier(userId), checks mappedUser and
mappedUser.getUserId(), and returns the canonicalUserId (or null/Optional).
Replace the duplicated blocks in DemographicServiceUtil (locations around the
existing code that set demographicEntity.setCrAppuserId, setCreatedBy,
setUpdatedBy and the other occurrences at the noted ranges) to call this helper
and then set the demographicEntity fields only when the helper returns a
non-null/non-empty id to preserve existing fallback behavior.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java`:
- Around line 292-296: In DocumentServiceUtil remove the redundant assignments
that set crBy/updBy directly before they are overwritten: in the copy path
delete the calls copyDocumentEntity.setCrBy(sourceEntity.getCrBy()) and
copyDocumentEntity.setUpdBy(sourceEntity.getUpdBy()), leaving only the
effective-value assignments
copyDocumentEntity.setCrBy(sourceEntity.getEffectiveCrBy()) and
copyDocumentEntity.setUpdBy(sourceEntity.getEffectiveUpdBy()) to avoid redundant
writes.
In
`@pre-registration/pre-registration-datasync-service/src/test/java/io/mosip/preregistration/datasync/test/service/DataSyncServiceTest.java`:
- Around line 338-339: The test currently stubs serviceUtil.reverseDateSyncSave
with Mockito.any() for the createdBy argument which masks regressions in the
user-id; update the test in DataSyncServiceTest to assert the exact createdBy
value by replacing the third Mockito.any() with Mockito.eq("testUser") in the
stub or add a post-call verification like
Mockito.verify(serviceUtil).reverseDateSyncSave(Mockito.any(), Mockito.any(),
Mockito.eq("testUser")) so DataSyncService passes the expected user id.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
db_scripts/mosip_prereg/ddl/user_details.sqlpre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/entity/OtpTransaction.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/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/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/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-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/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/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/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.java
Comment on lines
+116
to
+133
| Optional<UserDetails> found = userDetailsRepository.findByIdentifierHash(hash); | ||
| if (found.isPresent()) { | ||
| UserDetails existing = found.get(); | ||
| if (existing.getIdentifierEncrypted() == null || existing.getIdentifierEncrypted().isBlank()) { | ||
| String encrypted = encryptIdentifierIfConfigured(identifier); | ||
| if (encrypted != null) { | ||
| existing.setIdentifierEncrypted(encrypted); | ||
| return userDetailsRepository.save(existing); | ||
| } | ||
| } | ||
| return existing; | ||
| } | ||
| UserDetails u = new UserDetails(); | ||
| u.setUserId(UUID.randomUUID()); | ||
| u.setIdentifierHash(hash); | ||
| u.setIdentifierEncrypted(encryptIdentifierIfConfigured(identifier)); | ||
| return userDetailsRepository.save(u); | ||
| } |
There was a problem hiding this comment.
Make findOrCreateByIdentifier safe under concurrent requests.
The check-then-insert flow is racy and can fail idempotency under concurrent calls for the same identifier.
🔧 Suggested fix
+import org.springframework.dao.DataIntegrityViolationException;
...
String hash = sha256Hex(norm);
Optional<UserDetails> found = userDetailsRepository.findByIdentifierHash(hash);
if (found.isPresent()) {
...
}
UserDetails u = new UserDetails();
u.setUserId(UUID.randomUUID());
u.setIdentifierHash(hash);
u.setIdentifierEncrypted(encryptIdentifierIfConfigured(identifier));
-return userDetailsRepository.save(u);
+try {
+ return userDetailsRepository.save(u);
+} catch (DataIntegrityViolationException ex) {
+ // Another transaction created the same hash concurrently
+ return userDetailsRepository.findByIdentifierHash(hash)
+ .orElseThrow(() -> ex);
+}🤖 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 116 - 133, findOrCreateByIdentifier is racy: multiple threads can
pass the find-then-save path for the same identifierHash causing duplicate
entries; make it safe by relying on a DB unique constraint on
UserDetails.identifierHash and converting the method to an
upsert/try-insert-with-retry pattern. Specifically, ensure UserDetails has a
unique constraint on identifierHash, then change the logic in
findOrCreateByIdentifier to (a) try to create and save a new UserDetails (using
UUID.randomUUID(),
setIdentifierEncrypted(encryptIdentifierIfConfigured(identifier)),
setIdentifierHash(hash)) and return it, (b) if save throws a
duplicate-key/DataIntegrityViolationException catch it, then reload the existing
UserDetails via userDetailsRepository.findByIdentifierHash(hash) and proceed to
update identifierEncrypted if missing (use encryptIdentifierIfConfigured) and
save; for updating the existing record consider wrapping update/save in a
transaction to avoid lost updates. Use the repository methods
userDetailsRepository.findByIdentifierHash and userDetailsRepository.save and
the model UserDetails as the referenced symbols.
Comment on lines
+51
to
+56
| public void testFindByIdentifierDelegatesToRepo() { | ||
| UserDetails mock = new UserDetails(); | ||
| when(userDetailsRepository.findByIdentifierHash(any())).thenReturn(Optional.of(mock)); | ||
| Optional<UserDetails> res = userDetailsService.findByIdentifier("TestUser"); | ||
| verify(userDetailsRepository).findByIdentifierHash(any()); | ||
| } |
There was a problem hiding this comment.
Strengthen the delegation test with a return-value assertion.
testFindByIdentifierDelegatesToRepo verifies the repository call but doesn’t assert the returned Optional, so regressions in return behavior can still pass.
✅ Suggested test tightening
`@Test`
public void testFindByIdentifierDelegatesToRepo() {
UserDetails mock = new UserDetails();
when(userDetailsRepository.findByIdentifierHash(any())).thenReturn(Optional.of(mock));
Optional<UserDetails> res = userDetailsService.findByIdentifier("TestUser");
verify(userDetailsRepository).findByIdentifierHash(any());
+ assertTrue(res.isPresent());
+ assertEquals(mock, res.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 51 - 56, The test testFindByIdentifierDelegatesToRepo should also
assert the returned Optional value: after stubbing
userDetailsRepository.findByIdentifierHash(...) to return Optional.of(mock) and
calling userDetailsService.findByIdentifier("TestUser"), add an assertion that
the returned Optional equals Optional.of(mock) (or that res.isPresent() and
res.get() == mock) to ensure delegation returns the repository value; update the
test to use the same mock UserDetails instance and verify both the repository
interaction and the actual returned Optional from
userDetailsService.findByIdentifier.
Comment on lines
438
to
+439
| reverseDatasyncReponse = serviceUtil.reverseDateSyncSave(reverseDataSyncRequest.getRequesttime(), | ||
| reverseDataSyncRequest.getRequest(), "user"); | ||
| reverseDataSyncRequest.getRequest(), authUserDetails().getUserId()); |
There was a problem hiding this comment.
Guard Line 439 against missing authentication context.
Using authUserDetails().getUserId() here can throw when security context/principal is absent (e.g., internal/system-triggered flow), causing reverse sync failure where it previously succeeded.
Proposed hardening
- reverseDatasyncReponse = serviceUtil.reverseDateSyncSave(reverseDataSyncRequest.getRequesttime(),
- reverseDataSyncRequest.getRequest(), authUserDetails().getUserId());
+ String actorUserId = "system";
+ if (SecurityContextHolder.getContext() != null
+ && SecurityContextHolder.getContext().getAuthentication() != null
+ && SecurityContextHolder.getContext().getAuthentication().getPrincipal() instanceof AuthUserDetails) {
+ AuthUserDetails principal = (AuthUserDetails) SecurityContextHolder.getContext()
+ .getAuthentication().getPrincipal();
+ if (principal.getUserId() != null) {
+ actorUserId = principal.getUserId();
+ }
+ }
+ reverseDatasyncReponse = serviceUtil.reverseDateSyncSave(reverseDataSyncRequest.getRequesttime(),
+ reverseDataSyncRequest.getRequest(), actorUserId);🤖 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 438 - 439, The call in DataSyncService that passes
authUserDetails().getUserId() into serviceUtil.reverseDateSyncSave can NPE when
the security context/principal is absent; update the code to first safely
resolve the authenticated user id (e.g., check
SecurityContextHolder.getContext() / authUserDetails() for null or use an
Optional) and provide a sensible fallback (such as a configured SYSTEM_USER_ID
or null) before calling reverseDateSyncSave with
reverseDataSyncRequest.getRequesttime(), reverseDataSyncRequest.getRequest(),
and the safe user id; ensure the fallback path is clearly audible in logs so
internal/system-triggered flows continue without throwing.
mayuradesh
reviewed
Mar 2, 2026
|
|
||
| -- ========== CREATE TABLE: Canonical User Registry ========== | ||
|
|
||
| CREATE TABLE IF NOT EXISTS prereg.user_details ( |
* Create NOTICE Signed-off-by: rajapandi1234 <138785181+rajapandi1234@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Add files via upload Signed-off-by: rajapandi1234 <138785181+rajapandi1234@users.noreply.github.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * MOSIP-44419 (#1023) Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * MOSIP-33663 (#1021) Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * PII Issue Fixes (#1024) * PII Issue Fixes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fix toString issues Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Added UserValidation to applicant service Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * UUID Changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Cache changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed appointment booking validation error Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed UUID Issues Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed demographic retrieve details issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed get all applications API Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed processed_prereg_list table cr_by data issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixes nexus build issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fix captcha service nexus build Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixes nexus build for batchjob service Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * "Fixes nexus build issue for all pom's" Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Parent pom fixes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert Pom changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert pom changes for nexus build Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert parent pom changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> --------- Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed User Encryption PII Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Updated user validation paths Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Updated User Validations Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert "Updated user validation paths" (#1027) Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Added user_details table (#1028) * Revert "Updated user validation paths" Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Added user_details.sql Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> --------- Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Backward Compatibility Changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Backward Compatibility Changes for OTPManager Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fix v1/sync issue Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * DataSyncServiceUtil changes regardigng the sync Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Revert DataSync Changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Dual backward compatibility fixes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * ApplicationConsumedStatusUpdater for document and appointment Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Added "cr_dtimes" and "encrypted_dtimes" columns for user_details table Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Logger value masking Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Logger value masking for userID Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Logger value masking for preregUserId Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * MOSIP-44331 : Updated descriptions (#1022) * MOSIP-44331 : Updated description Signed-off-by: Rachana S P <rachana.p@cyberpwn.com> * MOSIP-44331 : Pre-reg description updated Signed-off-by: Rachana S P <rachana.p@cyberpwn.com> --------- Signed-off-by: Rachana S P <rachana.p@cyberpwn.com> Co-authored-by: Rachana S P <rachana.p@cyberpwn.com> Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Updated DB Scripts to resolve review comments Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * kernel pom version changes Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Fixed Consumed tables PII issues Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com> * Core Changes 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: SradhaMohanty5899 <mohantysradha10@gmail.com> Signed-off-by: Rachana S P <rachana.p@cyberpwn.com> Co-authored-by: rajapandi1234 <138785181+rajapandi1234@users.noreply.github.com> Co-authored-by: Sradha Mohanty <mohantysradha10@gmail.com> Co-authored-by: Rachana S P <153977086+rachanaspsoratur@users.noreply.github.com> Co-authored-by: Rachana S P <rachana.p@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pre-registration/pre-registration-application-service/pom.xml (1)
114-133:⚠️ Potential issue | 🟡 MinorAlign kernel module versions to 1.3.0 or verify intentional version mismatch.
kernel-bomandkernel-coreare at1.3.0, but other kernel dependencies (kernel-idobjectvalidator,kernel-templatemanager-velocity,kernel-qrcodegenerator-zxing,kernel-khazana,kernel-transliteration-icu4j) remain at1.2.1-SNAPSHOT. This version mismatch between kernel modules could cause runtime incompatibilities if they have interdependencies with breaking changes in 1.3.0.Verify whether these modules should be upgraded to align with the kernel BOM version, or confirm that this version mismatch is intentional.
🤖 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 114 - 133, The listed kernel dependencies (kernel-idobjectvalidator, kernel-templatemanager-velocity, kernel-qrcodegenerator-zxing, kernel-khazana, kernel-transliteration-icu4j) are at 1.2.1-SNAPSHOT while kernel-bom and kernel-core are 1.3.0; update these dependency versions in the pom (or centralize via a BOM/property) to 1.3.0 to align module versions, or explicitly document/guard the intentional mismatch (e.g., add a comment or property explaining why those artifacts must remain 1.2.1-SNAPSHOT) and run a build/test to confirm compatibility.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java (2)
498-506:⚠️ Potential issue | 🟡 MinorSame issue: Missing
findByIdstub and user context.The
deleteBookingAndUpdateApplicationExecptionTesthas the same missing stubs as the previous exception test.🔧 Suggested fix
when(applicationRepostiory.save(applicationEntity)).thenReturn(applicationEntity); when(applicationRepostiory.getOne("98765432101234")).thenReturn(applicationEntity); + when(applicationRepostiory.findById(prid)).thenReturn(java.util.Optional.of(applicationEntity)); AuthUserDetails applicationUser = mock(AuthUserDetails.class); Authentication authentication = mock(Authentication.class); SecurityContext securityContext = mock(SecurityContext.class); when(securityContext.getAuthentication()).thenReturn(authentication); SecurityContextHolder.setContext(securityContext); when(SecurityContextHolder.getContext().getAuthentication().getPrincipal()).thenReturn(applicationUser); + when(applicationUser.getUserId()).thenReturn("test-user"); + List<GrantedAuthority> authorities = new ArrayList<>(); + authorities.add(new SimpleGrantedAuthority("ROLE_INDIVIDUAL")); + when(applicationUser.getAuthorities()).thenReturn((Collection) authorities);🤖 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 498 - 506, The test deleteBookingAndUpdateApplicationExecptionTest is missing a stub for applicationRepostiory.findById and a proper user principal in the security context; add a when(applicationRepostiory.findById("98765432101234")) stub to return Optional.of(applicationEntity) (matching the existing applicationEntity used by getOne/save) and ensure the SecurityContextHolder is set to a mocked SecurityContext/Authentication that returns a mocked AuthUserDetails from getPrincipal() so the code path relying on SecurityContextHolder.getContext().getAuthentication().getPrincipal() executes correctly.
452-460:⚠️ Potential issue | 🟡 MinorMissing
findByIdstub and user context for exception test.The
deleteAppointmentAppointmentExecptionTestis missing theapplicationRepostiory.findById(prid)stub and the authority/userId mocking that other tests have. SinceuserValidationcallsfindById().orElseThrow(), this test may fail or pass for incorrect reasons depending on how the exception is caught.🔧 Suggested fix
when(applicationRepostiory.save(applicationEntity)).thenReturn(applicationEntity); when(applicationRepostiory.getOne("98765432101234")).thenReturn(applicationEntity); + when(applicationRepostiory.findById(prid)).thenReturn(java.util.Optional.of(applicationEntity)); AuthUserDetails applicationUser = mock(AuthUserDetails.class); Authentication authentication = mock(Authentication.class); SecurityContext securityContext = mock(SecurityContext.class); when(securityContext.getAuthentication()).thenReturn(authentication); SecurityContextHolder.setContext(securityContext); when(SecurityContextHolder.getContext().getAuthentication().getPrincipal()).thenReturn(applicationUser); + when(applicationUser.getUserId()).thenReturn("test-user"); + List<GrantedAuthority> authorities = new ArrayList<>(); + authorities.add(new SimpleGrantedAuthority("ROLE_INDIVIDUAL")); + when(applicationUser.getAuthorities()).thenReturn((Collection) authorities);🤖 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 452 - 460, The test deleteAppointmentAppointmentExecptionTest is missing the applicationRepostiory.findById(prid) stub and the authenticated user context used by userValidation; add a when(applicationRepostiory.findById(prid)).thenReturn(Optional.of(applicationEntity)) (or Optional.empty() if testing not-found) and mock the SecurityContextHolder principal (AuthUserDetails) to return the same userId/authorities used in other tests so userValidation runs consistently; update the test to set applicationUser behaviors (e.g., getUserId/getAuthorities) and ensure SecurityContextHolder.getContext().getAuthentication().getPrincipal() returns that mocked AuthUserDetails.
♻️ Duplicate comments (3)
pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java (3)
100-114:⚠️ Potential issue | 🟠 MajorDecryption uses a fresh timestamp instead of the encryption timestamp.
At line 105,
LocalDateTime.now()generates a new timestamp for decryption, but it should match the timestamp used during encryption (line 81). TheUserDetailsentity has anencryptedDtimesfield that should be passed todecrypt()instead.🔧 Suggested fix - pass the stored timestamp
The method signature needs to accept the stored timestamp:
-private Optional<String> decryptIdentifierIfConfigured(String encryptedValue) { +private Optional<String> decryptIdentifierIfConfigured(String encryptedValue, LocalDateTime encryptedDtimes) { if (encryptedValue == null || encryptedValue.isBlank()) { return Optional.empty(); } try { - byte[] plain = cryptoUtil.decrypt(encryptedValue.getBytes(StandardCharsets.UTF_8), LocalDateTime.now()); + byte[] plain = cryptoUtil.decrypt(encryptedValue.getBytes(StandardCharsets.UTF_8), encryptedDtimes);Then update the caller in
getDecryptedIdentifier()to passuser.get().getEncryptedDtimes().🤖 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 100 - 114, decryptIdentifierIfConfigured currently calls cryptoUtil.decrypt with LocalDateTime.now(), which misaligns decryption timestamp; change decryptIdentifierIfConfigured to accept the stored encryption timestamp (from UserDetails.encryptedDtimes) and pass that to cryptoUtil.decrypt instead of LocalDateTime.now(); update the caller getDecryptedIdentifier to call decryptIdentifierIfConfigured(encryptedValue, user.get().getEncryptedDtimes()); ensure you handle null/blank encryptedDtimes (return Optional.empty() or parse it into the proper LocalDateTime format expected by cryptoUtil.decrypt) and retain the existing null/empty checks and exception handling.
233-261:⚠️ Potential issue | 🟠 MajorRace condition in
findOrCreateByIdentifierunder concurrent requests.The check-then-insert pattern (find at line 234, create at lines 251-260) is susceptible to race conditions when multiple threads attempt to create the same user concurrently. The unique constraint on
identifier_hashwill cause one thread to fail with a constraint violation.🔧 Suggested fix - catch constraint violation and retry lookup
+import org.springframework.dao.DataIntegrityViolationException; ... UserDetails u = new UserDetails(); u.setUserId(UUID.randomUUID()); u.setIdentifierHash(hash); u.setCrDtimes(LocalDateTime.now()); String encrypted = encryptIdentifierRequired(norm); u.setIdentifierEncrypted(encrypted); u.setEncryptedDtimes(LocalDateTime.now()); LOGGER.info("Creating new canonical user mapping for masked identifier {}", maskIdentifier(norm)); - return userDetailsRepository.save(u); + try { + return userDetailsRepository.save(u); + } catch (DataIntegrityViolationException ex) { + // Another transaction created the same hash concurrently - retry lookup + return userDetailsRepository.findByIdentifierHash(hash) + .orElseThrow(() -> ex); + }🤖 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 233 - 261, The find-or-create flow in findOrCreateByIdentifier uses a check-then-insert pattern and can race under concurrent requests; modify the logic around userDetailsRepository.save(existing) / save(u) (inside findOrCreateByIdentifier) to handle unique constraint violations by catching the persistence exception (e.g., DataIntegrityViolationException / ConstraintViolationException / DuplicateKeyException depending on your JPA/DB layer), and on catching it re-query userDetailsRepository.findByIdentifierHash(hash) and return the found entity (or rethrow only if still absent); ensure this retry path is applied for both the branch that repairs an existing record and the branch that creates a new UserDetails so concurrent inserts return the canonical row instead of failing.
226-230:⚠️ Potential issue | 🟡 MinorReject blank identifiers before hashing/persisting.
The current validation allows whitespace-only strings (e.g.,
" ") to pass, which would create a canonical record for an empty normalized identifier.🔧 Suggested fix
String norm = normalize(identifier); - if (norm == null) { + if (norm == null || norm.isBlank()) { throw new IllegalArgumentException("identifier is required"); }🤖 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 226 - 230, In findOrCreateByIdentifier, the normalized identifier returned by normalize can be a whitespace-only string and currently passes validation; update the guard to reject blank (whitespace-only) normalized identifiers by checking if norm is null OR norm.isBlank() (or norm.trim().isEmpty() if targeting pre-Java11) and throw an IllegalArgumentException (e.g., "identifier is required") when that check fails so no canonical record is created for empty/whitespace-only identifiers; reference normalize() and findOrCreateByIdentifier() when making the change.
🧹 Nitpick comments (8)
pre-registration/pre-registration-application-service/pom.xml (2)
98-98: Prefer using a property instead of hardcoding the version.The version
1.3.0is hardcoded here, but the parent POM already defineskernel.core.version=1.3.0. Consider using${kernel.core.version}for maintainability - if the version changes, you'd only need to update it in one place.♻️ 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 text node "1.3.0" in the <version> element with the parent POM property reference ${kernel.core.version}; locate the <version> element in the pre-registration module pom (the one currently containing <version>1.3.0</version>) and change its value to ${kernel.core.version}, keeping the element intact so Maven resolves the property from the parent.
248-248: Prefer using a property for consistency.Similar to
kernel-core, the version is hardcoded instead of using a property. Consider definingkernel.auth.adapter.versionor reusingkernel.core.versionif they should stay aligned.♻️ Suggested refactor
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-auth-adapter</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 248, The pom currently hardcodes the adapter version (<version>1.3.0</version>) which should be replaced by a Maven property for consistency; change the version element for the auth adapter artifact to reference a property (e.g., ${kernel.auth.adapter.version}) and add that property to the POM properties (or reuse ${kernel.core.version} if they must be aligned) so future updates are centralized and consistent.pre-registration/pre-registration-datasync-service/pom.xml (1)
169-169: Prefer using a property for maintainability.The version
1.3.0is hardcoded. Per the context snippet, the parent POM defineskernel.core.version=1.3.0. Using${kernel.core.version}would keep versions synchronized and simplify future upgrades.♻️ Suggested refactor
<dependency> <groupId>io.mosip.kernel</groupId> <artifactId>kernel-auth-adapter</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-datasync-service/pom.xml` at line 169, The POM hardcodes <version>1.3.0</version>; replace the literal with the parent property reference ${kernel.core.version} so the module uses the centralized kernel.core.version value defined in the parent POM; update the <version> element in the pre-registration-datasync-service pom.xml to use ${kernel.core.version} (ensuring the parent property name kernel.core.version is used exactly) so future upgrades stay synchronized.pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.java (1)
230-265: Tests create localDocumentServiceUtilinstances that bypass Spring injection.The tests at lines 231 and 267 use the Spring-autowired
documentServiceUtil(which hasUserDetailsServicemocked), but several other tests in this file (e.g., lines 160, 191, 302, 315, 333, 348, 369, 393, 418, 435) create localnew DocumentServiceUtil()instances. If those code paths eventually call methods that depend onUserDetailsService, they'll get NPEs.This isn't a current issue since
dtoToEntityand similar methods tested here don't directly callUserDetailsService, but it's worth noting for future test maintenance.Also applies to: 267-298
🤖 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 230 - 265, Several tests create local instances with new DocumentServiceUtil() which bypass Spring injection and can cause NPEs if methods later call UserDetailsService; update those tests to use the autowired documentServiceUtil (the existing field named documentServiceUtil) or, if a local instance is required, inject the mocked UserDetailsService into that instance before calling methods like dtoToEntity so UserDetailsService-dependent calls won't NPE. Locate uses of new DocumentServiceUtil() in this test class and replace them with the autowired documentServiceUtil or call the setter/constructor to set the mocked UserDetailsService (same mock used for the autowired instance) to ensure consistent wiring in tests.db_scripts/mosip_prereg/ddl/user_details.sql (1)
28-29: Partial indexidx_prereg_user_details_activemay have limited utility.The partial index filters on
WHERE identifier_encrypted IS NOT NULL, but sinceidentifier_encryptedis defined asNOT NULLin the table definition, this condition will always be true for all rows, making it equivalent to a regular index onuser_id.Either:
- Remove the
WHEREclause since all rows satisfy it, or- If you intend to allow nullable
identifier_encryptedin the future, update the table definition to match.🔧 Suggested fix (if column remains NOT NULL)
-CREATE INDEX IF NOT EXISTS idx_prereg_user_details_active -ON prereg.user_details(user_id) WHERE identifier_encrypted IS NOT NULL; +CREATE INDEX IF NOT EXISTS idx_prereg_user_details_active +ON prereg.user_details(user_id);🤖 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 prereg.user_details uses WHERE identifier_encrypted IS NOT NULL but identifier_encrypted is defined NOT NULL, so make the intent explicit: either drop the WHERE clause and create a regular index on user_id (remove the WHERE identifier_encrypted IS NOT NULL predicate from CREATE INDEX idx_prereg_user_details_active), or if you intended a partial index for nullable identifiers instead, modify the prereg.user_details column definition for identifier_encrypted to allow NULL and then keep the WHERE clause; update whichever of these two (index creation or column definition) matches the intended data model.pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java (1)
224-227: Consider using a domain-specific exception instead of IllegalStateException.
IllegalStateExceptionis a generic JVM exception. A custom exception (e.g.,CanonicalUserResolutionException) would provide clearer semantics and allow callers to handle this specific failure distinctly.However, given the try-catch in the forEach loop captures this and adds the preRegId to
errorredPreRegIds, the current behavior is functionally correct for partial failure handling.🤖 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 224 - 227, Replace the generic IllegalStateException in resolveCanonicalUserId with a domain-specific exception (e.g., CanonicalUserResolutionException): create a CanonicalUserResolutionException (unchecked) with constructors that accept message and cause, update resolveCanonicalUserId to orElseThrow(() -> new CanonicalUserResolutionException("Failed to resolve canonical user id for consumed table write")) and add necessary imports; keep existing caller handling unchanged so the forEach try/catch still collects errorredPreRegIds.pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java (2)
712-722: Same redundant double-assignment pattern in saveAndUpdateApplicationEntity.Lines 712-714 set
crBy,updBy, andcontactInfoto rawuserId, then lines 720-722 overwrite them witheffectiveUserId.♻️ Suggested cleanup
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(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "Resolved effective user id for applications write. applicationId=" + preId + ", maskedUserId=" + maskIdentifier(userId) + ", canonicalApplied=" + isCanonicalApplied(userId, effectiveUserId)); 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, avoid the redundant double-assignment to applicationEntity fields: remove the initial sets of applicationEntity.setCrBy(userId), setUpdBy(userId) and setContactInfo(userId) (lines that occur before resolving effectiveUserId) and only assign applicationEntity.setCrBy(effectiveUserId), applicationEntity.setUpdBy(effectiveUserId) and applicationEntity.setContactInfo(effectiveUserId) after calling userDetailsService.resolveCanonicalUserIdOrIdentifier(userId); keep the timestamp assignments (setCrDtime/setUpdDtime) as-is but ensure crBy/updBy/contactInfo are assigned once using effectiveUserId to eliminate the overwrite.
304-324: Redundant field assignments before canonicalization.Lines 304-313 set
crAppuserId,createdBy, andupdatedByto the rawuserId, but these are immediately overwritten at lines 321-323 with the resolvedeffectiveUserId. The initial assignments are unnecessary.♻️ Suggested cleanup
demographicEntity.setApplicantDetailJson(encryptedDemographicDetails); demographicEntity.setLangCode(demographicRequest.getLangCode()); - 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); log.info(LOGGER_SESSIONID, LOGGER_IDTYPE, LOGGER_ID, "Resolved effective user id for demographic create. maskedUserId=" + maskIdentifier(userId) + ", canonicalApplied=" + isCanonicalApplied(userId, effectiveUserId)); 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 - 324, Remove the redundant early assignments to demographicEntity.setCrAppuserId(userId), demographicEntity.setCreatedBy(userId) and demographicEntity.setUpdatedBy(userId) that occur before resolving the canonical user; these are immediately overwritten by effectiveUserId from userDetailsService.resolveCanonicalUserIdOrIdentifier(userId). Keep the hashing, timestamps, encryptionDateTime and the info log that resolves and masks the user, but delete the three initial setXXX(userId) calls so only the canonical effectiveUserId (set via demographicEntity.setCrAppuserId(effectiveUserId), setCreatedBy(effectiveUserId), setUpdatedBy(effectiveUserId)) is persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-test/src/main/resources/preReg/AuditLog/AuditLog.yml`:
- Line 5: The YAML file contains a tab character at the start of the description
line which breaks parsing; replace the leading tab on the `description` line
with spaces to match the file's indentation style (e.g., two spaces) so the
`description: Validate that adding a contact...` entry is space-indented and
YAML-parseable.
In `@api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.yml`:
- Line 37: The YAML test case description incorrectly says "Proof of Address"
but the case is for POB (Proof of Birth); update the description string in
UpdateDocRefID.yml (the description field for the UpdateDocRefID POB test case)
to say "Proof of Birth" (or otherwise reference POB) so it accurately reflects
the test scenario.
- Around line 66-70: The test case Prereg_UpdateDocRefID_inValid_documentId is
missing the role field and should include role: batch to match other negative
tests; update the YAML entry for Prereg_UpdateDocRefID_inValid_documentId (the
block containing endPoint, uniqueIdentifier TC_prereg_UpdateDocRefID_05,
description, restMethod) by adding a role: batch line so authorization is
consistent with the other cases.
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Around line 14-20: The table DDL conflicts with runtime repair logic in
UserDetailsService.findOrCreateByIdentifier(): either make the table columns
nullable so repair updates can succeed or remove the repair code; to fix
quickly, update the DDL to allow nulls for identifier_encrypted, cr_dtimes, and
encrypted_dtimes (remove NOT NULL on those columns) so the partial index
idx_prereg_user_details_active (which filters WHERE identifier_encrypted IS NOT
NULL) remains valid and the existing repair/update logic in
findOrCreateByIdentifier() (referenced by that method name) can operate;
alternatively, if you choose the other option, remove the null-check/repair
blocks in UserDetailsService.findOrCreateByIdentifier() so the table can keep
NOT NULL constraints and ensure any migration populates those fields before
applying NOT NULL.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.java`:
- Around line 61-62: The paginated repository method
findByCreatedByInAndStatusCodeOrderByCreateDateTime uses an equality match on
statusCode but should be a NOT-EQUALS filter like the non-paginated version;
rename the method to findByCreatedByInAndStatusCodeNotOrderByCreateDateTime (and
keep the same parameters List<String> userIds, String statusCode, Pageable
pageable) so Spring Data generates a "statusCode <> ?" predicate for pagination
as well.
- Around line 46-47: The derived repository method
findByCreatedByInAndStatusCode in DemographicRepository reverses semantics
(equals vs not-equals) compared to the original NamedQuery; update it to
preserve the NOT EQUAL behavior by renaming it to
findByCreatedByInAndStatusCodeNot and/or add an explicit `@Query` on the method
(e.g., using "e.statusCode <> :statusCode") so the repository excludes records
with the supplied statusCode (service calls use StatusCodes.CONSUMED.getCode());
ensure the method signature still accepts `@Param`("userIds") List<String> userIds
and `@Param`("statusCode") String statusCode to match callers.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java`:
- Around line 499-501: The current calls to
demographicRepository.findByCreatedByInAndStatusCode and
findByCreatedByInAndStatusCodeOrderByCreateDateTime now include records with the
given statusCode (e.g., CONSUMED) whereas the original queries excluded that
status; restore the original exclusion semantics by either changing the
repository method signatures to use Spring Data negative naming (e.g.,
findByCreatedByInAndStatusCodeNot and
findByCreatedByInAndStatusCodeNotOrderByCreateDateTime) or add explicit `@Query`
annotations to those methods with "WHERE d.statusCode <> :statusCode" to ensure
CONSUMED records are excluded when the service passes
StatusCodes.CONSUMED.getCode().
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/LoginService.java`:
- Around line 151-152: The log statement in the callsendOtp method of
LoginService uses maskIdentifier(userOtpRequest.getRequest().getUserId()) and
the language variable but only supplies one placeholder; update the log.info
call to include a second {} placeholder for the language and pass language as
the second argument so the message interpolates both values (keep maskIdentifier
as the first argument and language as the second).
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/DemographicServiceTest.java`:
- Around line 597-601: The test's repository stubs in DemographicServiceTest are
now mocking demographicRepository.findByCreatedByInAndStatusCode(...) and
findByCreatedByInAndStatusCodeOrderByCreateDateTime(...) to return entities that
match status "Consumed" (equals), which changes behavior from the prior query
that excluded CONSUMED records; inspect the getAllApplicationDetails method to
confirm whether it should include or exclude CONSUMED, and then update the test
stubs to reflect the intended behavior—if CONSUMED should be excluded, change
the mocks to use the repository method that filters out "Consumed" (e.g., a
not-equals variant or the method previously used) or adjust returned test data
to have non-Consumed statuses; if inclusion is intended, update the test
assertion comments to state the semantic change.
In
`@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java`:
- Around line 119-130: The cache key expression in
UserDetailsService.findByIdentifier can cause an NPE because `@Cacheable`'s key
"#identifier.toLowerCase().trim()" will be evaluated even when
condition="#identifier != null" prevents caching; update the annotation to
either replace condition with unless = "#identifier == null" so the key isn't
evaluated when identifier is null, or make the key null-safe (e.g., use a
ternary-style guard in the key expression to return null when identifier is
null) to avoid calling toLowerCase() on a null identifier.
---
Outside diff comments:
In `@pre-registration/pre-registration-application-service/pom.xml`:
- Around line 114-133: The listed kernel dependencies (kernel-idobjectvalidator,
kernel-templatemanager-velocity, kernel-qrcodegenerator-zxing, kernel-khazana,
kernel-transliteration-icu4j) are at 1.2.1-SNAPSHOT while kernel-bom and
kernel-core are 1.3.0; update these dependency versions in the pom (or
centralize via a BOM/property) to 1.3.0 to align module versions, or explicitly
document/guard the intentional mismatch (e.g., add a comment or property
explaining why those artifacts must remain 1.2.1-SNAPSHOT) and run a build/test
to confirm compatibility.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/AppointmentServiceImplTest.java`:
- Around line 498-506: The test deleteBookingAndUpdateApplicationExecptionTest
is missing a stub for applicationRepostiory.findById and a proper user principal
in the security context; add a
when(applicationRepostiory.findById("98765432101234")) stub to return
Optional.of(applicationEntity) (matching the existing applicationEntity used by
getOne/save) and ensure the SecurityContextHolder is set to a mocked
SecurityContext/Authentication that returns a mocked AuthUserDetails from
getPrincipal() so the code path relying on
SecurityContextHolder.getContext().getAuthentication().getPrincipal() executes
correctly.
- Around line 452-460: The test deleteAppointmentAppointmentExecptionTest is
missing the applicationRepostiory.findById(prid) stub and the authenticated user
context used by userValidation; add a
when(applicationRepostiory.findById(prid)).thenReturn(Optional.of(applicationEntity))
(or Optional.empty() if testing not-found) and mock the SecurityContextHolder
principal (AuthUserDetails) to return the same userId/authorities used in other
tests so userValidation runs consistently; update the test to set
applicationUser behaviors (e.g., getUserId/getAuthorities) and ensure
SecurityContextHolder.getContext().getAuthentication().getPrincipal() returns
that mocked AuthUserDetails.
---
Duplicate comments:
In
`@pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/service/UserDetailsService.java`:
- Around line 100-114: decryptIdentifierIfConfigured currently calls
cryptoUtil.decrypt with LocalDateTime.now(), which misaligns decryption
timestamp; change decryptIdentifierIfConfigured to accept the stored encryption
timestamp (from UserDetails.encryptedDtimes) and pass that to cryptoUtil.decrypt
instead of LocalDateTime.now(); update the caller getDecryptedIdentifier to call
decryptIdentifierIfConfigured(encryptedValue, user.get().getEncryptedDtimes());
ensure you handle null/blank encryptedDtimes (return Optional.empty() or parse
it into the proper LocalDateTime format expected by cryptoUtil.decrypt) and
retain the existing null/empty checks and exception handling.
- Around line 233-261: The find-or-create flow in findOrCreateByIdentifier uses
a check-then-insert pattern and can race under concurrent requests; modify the
logic around userDetailsRepository.save(existing) / save(u) (inside
findOrCreateByIdentifier) to handle unique constraint violations by catching the
persistence exception (e.g., DataIntegrityViolationException /
ConstraintViolationException / DuplicateKeyException depending on your JPA/DB
layer), and on catching it re-query
userDetailsRepository.findByIdentifierHash(hash) and return the found entity (or
rethrow only if still absent); ensure this retry path is applied for both the
branch that repairs an existing record and the branch that creates a new
UserDetails so concurrent inserts return the canonical row instead of failing.
- Around line 226-230: In findOrCreateByIdentifier, the normalized identifier
returned by normalize can be a whitespace-only string and currently passes
validation; update the guard to reject blank (whitespace-only) normalized
identifiers by checking if norm is null OR norm.isBlank() (or
norm.trim().isEmpty() if targeting pre-Java11) and throw an
IllegalArgumentException (e.g., "identifier is required") when that check fails
so no canonical record is created for empty/whitespace-only identifiers;
reference normalize() and findOrCreateByIdentifier() when making the change.
---
Nitpick comments:
In `@db_scripts/mosip_prereg/ddl/user_details.sql`:
- Around line 28-29: The partial index idx_prereg_user_details_active on
prereg.user_details uses WHERE identifier_encrypted IS NOT NULL but
identifier_encrypted is defined NOT NULL, so make the intent explicit: either
drop the WHERE clause and create a regular index on user_id (remove the WHERE
identifier_encrypted IS NOT NULL predicate from CREATE INDEX
idx_prereg_user_details_active), or if you intended a partial index for nullable
identifiers instead, modify the prereg.user_details column definition for
identifier_encrypted to allow NULL and then keep the WHERE clause; update
whichever of these two (index creation or column definition) matches the
intended data model.
In `@pre-registration/pre-registration-application-service/pom.xml`:
- Line 98: Replace the hardcoded version text node "1.3.0" in the <version>
element with the parent POM property reference ${kernel.core.version}; locate
the <version> element in the pre-registration module pom (the one currently
containing <version>1.3.0</version>) and change its value to
${kernel.core.version}, keeping the element intact so Maven resolves the
property from the parent.
- Line 248: The pom currently hardcodes the adapter version
(<version>1.3.0</version>) which should be replaced by a Maven property for
consistency; change the version element for the auth adapter artifact to
reference a property (e.g., ${kernel.auth.adapter.version}) and add that
property to the POM properties (or reuse ${kernel.core.version} if they must be
aligned) so future updates are centralized and consistent.
In
`@pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DemographicServiceUtil.java`:
- Around line 712-722: In saveAndUpdateApplicationEntity, avoid the redundant
double-assignment to applicationEntity fields: remove the initial sets of
applicationEntity.setCrBy(userId), setUpdBy(userId) and setContactInfo(userId)
(lines that occur before resolving effectiveUserId) and only assign
applicationEntity.setCrBy(effectiveUserId),
applicationEntity.setUpdBy(effectiveUserId) and
applicationEntity.setContactInfo(effectiveUserId) after calling
userDetailsService.resolveCanonicalUserIdOrIdentifier(userId); keep the
timestamp assignments (setCrDtime/setUpdDtime) as-is but ensure
crBy/updBy/contactInfo are assigned once using effectiveUserId to eliminate the
overwrite.
- Around line 304-324: Remove the redundant early assignments to
demographicEntity.setCrAppuserId(userId), demographicEntity.setCreatedBy(userId)
and demographicEntity.setUpdatedBy(userId) that occur before resolving the
canonical user; these are immediately overwritten by effectiveUserId from
userDetailsService.resolveCanonicalUserIdOrIdentifier(userId). Keep the hashing,
timestamps, encryptionDateTime and the info log that resolves and masks the
user, but delete the three initial setXXX(userId) calls so only the canonical
effectiveUserId (set via demographicEntity.setCrAppuserId(effectiveUserId),
setCreatedBy(effectiveUserId), setUpdatedBy(effectiveUserId)) is persisted.
In
`@pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/service/util/DocumentServiceUtilTest.java`:
- Around line 230-265: Several tests create local instances with new
DocumentServiceUtil() which bypass Spring injection and can cause NPEs if
methods later call UserDetailsService; update those tests to use the autowired
documentServiceUtil (the existing field named documentServiceUtil) or, if a
local instance is required, inject the mocked UserDetailsService into that
instance before calling methods like dtoToEntity so UserDetailsService-dependent
calls won't NPE. Locate uses of new DocumentServiceUtil() in this test class and
replace them with the autowired documentServiceUtil or call the
setter/constructor to set the mocked UserDetailsService (same mock used for the
autowired instance) to ensure consistent wiring in tests.
In
`@pre-registration/pre-registration-batchjob/src/main/java/io/mosip/preregistration/batchjob/impl/ApplicationConsumedStatusUpdater.java`:
- Around line 224-227: Replace the generic IllegalStateException in
resolveCanonicalUserId with a domain-specific exception (e.g.,
CanonicalUserResolutionException): create a CanonicalUserResolutionException
(unchecked) with constructors that accept message and cause, update
resolveCanonicalUserId to orElseThrow(() -> new
CanonicalUserResolutionException("Failed to resolve canonical user id for
consumed table write")) and add necessary imports; keep existing caller handling
unchanged so the forEach try/catch still collects errorredPreRegIds.
In `@pre-registration/pre-registration-datasync-service/pom.xml`:
- Line 169: The POM hardcodes <version>1.3.0</version>; replace the literal with
the parent property reference ${kernel.core.version} so the module uses the
centralized kernel.core.version value defined in the parent POM; update the
<version> element in the pre-registration-datasync-service pom.xml to use
${kernel.core.version} (ensuring the parent property name kernel.core.version is
used exactly) so future upgrades stay synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f60b7219-6302-4bcb-a5b8-8607f9089a10
📒 Files selected for processing (78)
api-test/src/main/resources/preReg/AddLostUinApplication/AddLostUinApplication.ymlapi-test/src/main/resources/preReg/AddUpdateRegistration/AddUpdateRegistration.ymlapi-test/src/main/resources/preReg/AuditLog/AuditLog.ymlapi-test/src/main/resources/preReg/BookMultipleAppointment/BookMultipleAppointment.ymlapi-test/src/main/resources/preReg/CancelApplicationsAppointment/CancelApplicationsAppointment.ymlapi-test/src/main/resources/preReg/CancelAppointment/CancelAppointment.ymlapi-test/src/main/resources/preReg/CopyDocument/CopyDocument.ymlapi-test/src/main/resources/preReg/DeleteAllDocForaPRID/DeleteAllDocForaPRID.ymlapi-test/src/main/resources/preReg/DeleteBooking/DeleteBooking.ymlapi-test/src/main/resources/preReg/DeleteLostUinApplication/DeleteLostUinApplication.ymlapi-test/src/main/resources/preReg/DeletePreRegistration/DeletePreRegistration.ymlapi-test/src/main/resources/preReg/DeleteSpecificDocForaPRID/DeleteSpecificDocForaPRID.ymlapi-test/src/main/resources/preReg/FetchApplicationByPrid/FetchApplicationByPrid.ymlapi-test/src/main/resources/preReg/FetchAppointmentDetailsByPrid/FetchAppointmentDetailsByPrid.ymlapi-test/src/main/resources/preReg/FetchAvailabilityData/FetchAvailabilityData.ymlapi-test/src/main/resources/preReg/GetAllApplications/GetAllApplications.ymlapi-test/src/main/resources/preReg/GetAllApplicationsWithapplicationId/GetAllApplicationsWithapplicationId.ymlapi-test/src/main/resources/preReg/GetAllDocForPrId/GetAllDocForPrId.ymlapi-test/src/main/resources/preReg/GetAllPreRegApplications/GetAllPreRegApplications.ymlapi-test/src/main/resources/preReg/GetApplicationStatusByApplicationID/GetApplicationStatusByApplicationID.ymlapi-test/src/main/resources/preReg/GetApplicationStatusWithPrId/GetApplicationStatusWithPrId.ymlapi-test/src/main/resources/preReg/GetApplicationWithPrId/GetApplicationWithPrId.ymlapi-test/src/main/resources/preReg/GetAppointmentDetails/GetAppointmentDetails.ymlapi-test/src/main/resources/preReg/GetAvailableSlotForRegCentreId/GetAvailableSlotForRegCentreId.ymlapi-test/src/main/resources/preReg/GetBookingsForRegCenter/GetBookingsForRegCenter.ymlapi-test/src/main/resources/preReg/GetPRIDByDateRange/GetPRIDByDateRange.ymlapi-test/src/main/resources/preReg/GetPRIDForRegCent/GetPRIDForRegCent.ymlapi-test/src/main/resources/preReg/GetPreRegDemographicDataByPrid/GetPreRegDemographicDataByPrid.ymlapi-test/src/main/resources/preReg/GetPreRegInfoByPrid/GetPreRegInfoByPrid.ymlapi-test/src/main/resources/preReg/GetSpecificDocumentforaPRID/GetSpecificDocumentforaPRID.ymlapi-test/src/main/resources/preReg/GetUpdatedDateTimeByPrid/GetUpdatedDateTimeByPrid.ymlapi-test/src/main/resources/preReg/InvalidateToken/InvalidateToken.ymlapi-test/src/main/resources/preReg/SendNotifications/SendNotifications.ymlapi-test/src/main/resources/preReg/SendOtp/SendOtp.ymlapi-test/src/main/resources/preReg/Transliteration/Translate.ymlapi-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.ymlapi-test/src/main/resources/preReg/UpdatePreRegStatus/updatePreRegStatus.ymlapi-test/src/main/resources/preReg/UpdatePreregistration/UpdatePreregistration.ymlapi-test/src/main/resources/preReg/ValidateOtp/ValidateOtp.ymlapi-test/src/main/resources/preReg/bookAppointment/bookAppointment.ymlapi-test/src/main/resources/preReg/bookAppointmentByPRID/bookAppointmentByPRID.ymlapi-test/src/main/resources/preReg/createPrereg/createPrereg.ymlapi-test/src/main/resources/preReg/uploadDocument/uploadDocument.ymldb_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/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/main/resources/application.propertiespre-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/AppointmentServiceTest.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/impl/ApplicationConsumedStatusUpdater.javapre-registration/pre-registration-core/pom.xmlpre-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/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/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/service/util/DataSyncServiceUtilTest.javapre-registration/pre-registration-datasync-service/src/test/resources/application.properties
✅ Files skipped from review due to trivial changes (49)
- api-test/src/main/resources/preReg/GetBookingsForRegCenter/GetBookingsForRegCenter.yml
- pre-registration/pre-registration-application-service/src/main/resources/application.properties
- api-test/src/main/resources/preReg/GetAvailableSlotForRegCentreId/GetAvailableSlotForRegCentreId.yml
- api-test/src/main/resources/preReg/BookMultipleAppointment/BookMultipleAppointment.yml
- pre-registration/pre-registration-datasync-service/src/test/resources/application.properties
- api-test/src/main/resources/preReg/CancelApplicationsAppointment/CancelApplicationsAppointment.yml
- db_scripts/mosip_prereg/ddl.sql
- api-test/src/main/resources/preReg/GetApplicationWithPrId/GetApplicationWithPrId.yml
- api-test/src/main/resources/preReg/FetchApplicationByPrid/FetchApplicationByPrid.yml
- api-test/src/main/resources/preReg/GetApplicationStatusByApplicationID/GetApplicationStatusByApplicationID.yml
- api-test/src/main/resources/preReg/GetAllApplicationsWithapplicationId/GetAllApplicationsWithapplicationId.yml
- api-test/src/main/resources/preReg/AddLostUinApplication/AddLostUinApplication.yml
- api-test/src/main/resources/preReg/Transliteration/Translate.yml
- pre-registration/pre-registration-datasync-service/src/test/java/io/mosip/preregistration/datasync/test/service/util/DataSyncServiceUtilTest.java
- api-test/src/main/resources/preReg/DeleteBooking/DeleteBooking.yml
- api-test/src/main/resources/preReg/FetchAvailabilityData/FetchAvailabilityData.yml
- api-test/src/main/resources/preReg/DeletePreRegistration/DeletePreRegistration.yml
- api-test/src/main/resources/preReg/FetchAppointmentDetailsByPrid/FetchAppointmentDetailsByPrid.yml
- api-test/src/main/resources/preReg/DeleteAllDocForaPRID/DeleteAllDocForaPRID.yml
- api-test/src/main/resources/preReg/InvalidateToken/InvalidateToken.yml
- api-test/src/main/resources/preReg/GetPreRegInfoByPrid/GetPreRegInfoByPrid.yml
- api-test/src/main/resources/preReg/GetPRIDForRegCent/GetPRIDForRegCent.yml
- pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/test/controller/LoginControllerTest.java
- pre-registration/pom.xml
- api-test/src/main/resources/preReg/GetUpdatedDateTimeByPrid/GetUpdatedDateTimeByPrid.yml
- api-test/src/main/resources/preReg/GetAllDocForPrId/GetAllDocForPrId.yml
- api-test/src/main/resources/preReg/GetAllPreRegApplications/GetAllPreRegApplications.yml
- api-test/src/main/resources/preReg/DeleteSpecificDocForaPRID/DeleteSpecificDocForaPRID.yml
- api-test/src/main/resources/preReg/SendOtp/SendOtp.yml
- api-test/src/main/resources/preReg/CancelAppointment/CancelAppointment.yml
- api-test/src/main/resources/preReg/SendNotifications/SendNotifications.yml
- pre-registration/pre-registration-application-service/src/test/java/io/mosip/preregistration/application/service/OTPManagerTest.java
- api-test/src/main/resources/preReg/DeleteLostUinApplication/DeleteLostUinApplication.yml
- api-test/src/main/resources/preReg/CopyDocument/CopyDocument.yml
- api-test/src/main/resources/preReg/bookAppointment/bookAppointment.yml
- api-test/src/main/resources/preReg/GetPRIDByDateRange/GetPRIDByDateRange.yml
- api-test/src/main/resources/preReg/ValidateOtp/ValidateOtp.yml
- pre-registration/pre-registration-core/pom.xml
- api-test/src/main/resources/preReg/bookAppointmentByPRID/bookAppointmentByPRID.yml
- api-test/src/main/resources/preReg/GetAllApplications/GetAllApplications.yml
- api-test/src/main/resources/preReg/UpdatePreRegStatus/updatePreRegStatus.yml
- api-test/src/main/resources/preReg/createPrereg/createPrereg.yml
- api-test/src/main/resources/preReg/GetSpecificDocumentforaPRID/GetSpecificDocumentforaPRID.yml
- api-test/src/main/resources/preReg/uploadDocument/uploadDocument.yml
- api-test/src/main/resources/preReg/UpdatePreregistration/UpdatePreregistration.yml
- api-test/src/main/resources/preReg/GetApplicationStatusWithPrId/GetApplicationStatusWithPrId.yml
- api-test/src/main/resources/preReg/GetPreRegDemographicDataByPrid/GetPreRegDemographicDataByPrid.yml
- api-test/src/main/resources/preReg/AddUpdateRegistration/AddUpdateRegistration.yml
- api-test/src/main/resources/preReg/GetAppointmentDetails/GetAppointmentDetails.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/UserDetails.java
- pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/util/DocumentServiceUtil.java
| Prereg_AddContact_All_Valid_Smoke: | ||
| endPoint: SELECT module_name,cr_by FROM audit.app_audit_log where module_name LIKE '%prereg%' order by action_dtimes desc LIMIT 5 | ||
| role: audit | ||
| description: Validate that adding a contact is successfully processed when all mandatory fields are provided with valid values |
There was a problem hiding this comment.
Replace tab indentation on Line 5 to keep YAML parseable.
Line 5 starts with a tab before description. YAML parsing will fail on this token.
💡 Proposed fix
- description: Validate that adding a contact is successfully processed when all mandatory fields are provided with valid values
+ description: Validate that adding a contact is successfully processed when all mandatory fields are provided with valid values📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description: Validate that adding a contact is successfully processed when all mandatory fields are provided with valid values | |
| description: Validate that adding a contact is successfully processed when all mandatory fields are provided with valid values |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 5-5: syntax error: found character '\t' that cannot start any token
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-test/src/main/resources/preReg/AuditLog/AuditLog.yml` at line 5, The YAML
file contains a tab character at the start of the description line which breaks
parsing; replace the leading tab on the `description` line with spaces to match
the file's indentation style (e.g., two spaces) so the `description: Validate
that adding a contact...` entry is space-indented and YAML-parseable.
| endPoint: /preregistration/v1/documents/document/{documentId}?preRegistrationId={preRegistrationId}&refNumber={refNumber} | ||
| uniqueIdentifier: TC_prereg_UpdateDocRefID_03 | ||
| description: Update the document reference ID for the valid Proof of Date of Birth (POB) | ||
| description: Validate that updating the document reference ID for Proof of Address is successfully processed when all mandatory fields are provided with valid values |
There was a problem hiding this comment.
Fix document type text for the POB scenario.
Line 37 says “Proof of Address”, but this case is ..._POB_... (Proof of Birth).
💡 Proposed fix
- description: Validate that updating the document reference ID for Proof of Address is successfully processed when all mandatory fields are provided with valid values
+ description: Validate that updating the document reference ID for Proof of Birth is successfully processed when all mandatory fields are provided with valid values📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description: Validate that updating the document reference ID for Proof of Address is successfully processed when all mandatory fields are provided with valid values | |
| description: Validate that updating the document reference ID for Proof of Birth is successfully processed when all mandatory fields are provided with valid values |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.yml` at line
37, The YAML test case description incorrectly says "Proof of Address" but the
case is for POB (Proof of Birth); update the description string in
UpdateDocRefID.yml (the description field for the UpdateDocRefID POB test case)
to say "Proof of Birth" (or otherwise reference POB) so it accurately reflects
the test scenario.
Comment on lines
66
to
70
| Prereg_UpdateDocRefID_inValid_documentId: | ||
| endPoint: /preregistration/v1/documents/document/{documentId}?preRegistrationId={preRegistrationId}&refNumber={refNumber} | ||
| uniqueIdentifier: TC_prereg_UpdateDocRefID_05 | ||
| description: Update the document reference ID for the invalid document ID | ||
| role: batch | ||
| description: Validate that updating the document reference ID fails when an invalid document ID is provided | ||
| restMethod: put |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.yml"
awk '
/^ Prereg_UpdateDocRefID_/ {
if (case_name != "") {
printf "%s -> %s\n", case_name, (has_role ? "role=present" : "role=MISSING")
}
case_name=$1
has_role=0
}
/^[[:space:]]+role:/ { has_role=1 }
END {
if (case_name != "") {
printf "%s -> %s\n", case_name, (has_role ? "role=present" : "role=MISSING")
}
}
' "$file"Repository: mosip/pre-registration
Length of output: 512
🏁 Script executed:
cat -n api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.yml | head -100Repository: mosip/pre-registration
Length of output: 5585
🏁 Script executed:
# Check if there's documentation about role defaults
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.yml" \) | xargs rg -l "role.*default|default.*role" | head -5Repository: mosip/pre-registration
Length of output: 48
🏁 Script executed:
# Check other negative test cases in the same file to see if role omission is a pattern
tail -50 api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.ymlRepository: mosip/pre-registration
Length of output: 1760
Add missing role: batch to align with other test cases.
Prereg_UpdateDocRefID_inValid_documentId lacks a role field while all other test cases in the file explicitly define role: batch, including other negative test cases. This inconsistency can cause the test to behave differently across environments if role defaults vary, potentially shifting the failure from the intended invalid document ID validation to authorization issues.
File: api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.yml, lines 66–70
Prereg_UpdateDocRefID_inValid_documentId:
endPoint: /preregistration/v1/documents/document/{documentId}?preRegistrationId={preRegistrationId}&refNumber={refNumber}
uniqueIdentifier: TC_prereg_UpdateDocRefID_05
description: Validate that updating the document reference ID fails when an invalid document ID is provided
+ role: batch
restMethod: put
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-test/src/main/resources/preReg/UpdateDocRefID/UpdateDocRefID.yml` around
lines 66 - 70, The test case Prereg_UpdateDocRefID_inValid_documentId is missing
the role field and should include role: batch to match other negative tests;
update the YAML entry for Prereg_UpdateDocRefID_inValid_documentId (the block
containing endPoint, uniqueIdentifier TC_prereg_UpdateDocRefID_05, description,
restMethod) by adding a role: batch line so authorization is consistent with the
other cases.
Comment on lines
+14
to
+20
| 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, | ||
| cr_dtimes TIMESTAMP NOT NULL, | ||
| encrypted_dtimes TIMESTAMP NOT NULL | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any other DDL or migration scripts that might populate this table with null values
echo "=== Searching for user_details references in migration/ddl scripts ==="
find . -path "*/db_scripts/*" -name "*.sql" -exec grep -l "user_details" {} \;
echo -e "\n=== Checking entity definition for nullable annotations ==="
cat pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/UserDetails.java 2>/dev/null | head -50Repository: mosip/pre-registration
Length of output: 1266
🏁 Script executed:
# Examine the actual DDL file to confirm NOT NULL constraints
echo "=== Full DDL for user_details table ==="
cat db_scripts/mosip_prereg/ddl/user_details.sqlRepository: mosip/pre-registration
Length of output: 1764
🏁 Script executed:
# Find and examine UserDetailsService to verify repair logic
echo "=== Searching for UserDetailsService ==="
find . -name "UserDetailsService.java" -type f
echo -e "\n=== Checking for repair/null-handling logic ==="
find . -name "UserDetailsService.java" -type f -exec cat {} \; 2>/dev/null | head -300Repository: mosip/pre-registration
Length of output: 12086
🏁 Script executed:
# Check the main DDL file for any additional constraints or migration info
echo "=== Checking main DDL file ==="
grep -A 20 -B 5 "user_details" db_scripts/mosip_prereg/ddl.sql 2>/dev/null || echo "Pattern not found or file structure differs"Repository: mosip/pre-registration
Length of output: 336
DDL NOT NULL constraints conflict with repair logic and partial index design.
The DDL declares identifier_encrypted TEXT NOT NULL and cr_dtimes TIMESTAMP NOT NULL, but UserDetailsService.findOrCreateByIdentifier() (lines 252, 255-256) contains explicit null checks and repair logic that attempts to update these fields from null to populated values. Additionally, the partial index idx_prereg_user_details_active filters WHERE identifier_encrypted IS NOT NULL, which is only necessary if the column can contain null values.
This contradiction creates a runtime risk: if the repair logic encounters an existing row with null values, the UPDATE will fail due to the NOT NULL constraint.
Resolve this by either:
- Removing NOT NULL constraints to allow the repair logic to function, or
- Removing the null-handling repair logic if the table will only be populated through the create path (which always sets these fields).
If migrating legacy data, ensure migration scripts populate these fields before applying the NOT NULL constraints.
🤖 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 14 - 20, The table
DDL conflicts with runtime repair logic in
UserDetailsService.findOrCreateByIdentifier(): either make the table columns
nullable so repair updates can succeed or remove the repair code; to fix
quickly, update the DDL to allow nulls for identifier_encrypted, cr_dtimes, and
encrypted_dtimes (remove NOT NULL on those columns) so the partial index
idx_prereg_user_details_active (which filters WHERE identifier_encrypted IS NOT
NULL) remains valid and the existing repair/update logic in
findOrCreateByIdentifier() (referenced by that method name) can operate;
alternatively, if you choose the other option, remove the null-check/repair
blocks in UserDetailsService.findOrCreateByIdentifier() so the table can keep
NOT NULL constraints and ensure any migration populates those fields before
applying NOT NULL.
Comment on lines
+46
to
+47
| public List<DemographicEntity> findByCreatedByInAndStatusCode(@Param("userIds") List<String> userIds, | ||
| @Param("statusCode") String statusCode); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how the new repository methods are used in DemographicService
# Check if CONSUMED is being excluded or included
echo "=== Checking DemographicService usage of new repository methods ==="
rg -n "findByCreatedByInAndStatusCode" --type=java -C 3
echo ""
echo "=== Checking the original NamedQuery definition ==="
rg -n "NamedQuery.*findByCreatedBy" --type=java -C 2
echo ""
echo "=== Checking what status code is passed ==="
rg -n "StatusCodes.CONSUMED" --type=java -C 2 pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.javaRepository: mosip/pre-registration
Length of output: 11452
🏁 Script executed:
#!/bin/bash
# Check if the new repository methods have `@Query` annotations
echo "=== Checking for `@Query` annotations on new methods ==="
rg -n "findByCreatedByInAndStatusCode" --type=java -B 5 pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.javaRepository: mosip/pre-registration
Length of output: 771
Critical semantic mismatch: Query excludes vs. includes the status code.
The original NamedQuery methods use e.statusCode <> :statusCode (NOT EQUAL), excluding records with the specified status. However, the new Spring Data derived query findByCreatedByInAndStatusCode generates statusCode = ? (EQUALS), including only records with that status.
Since the service calls these methods with StatusCodes.CONSUMED.getCode(), the original behavior excluded CONSUMED records, but the new methods will include only CONSUMED records—a critical behavioral reversal.
Rename the methods to findByCreatedByInAndStatusCodeNot and provide @Query annotations to explicitly specify the NOT EQUAL condition, or update the service logic accordingly.
🤖 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/DemographicRepository.java`
around lines 46 - 47, The derived repository method
findByCreatedByInAndStatusCode in DemographicRepository reverses semantics
(equals vs not-equals) compared to the original NamedQuery; update it to
preserve the NOT EQUAL behavior by renaming it to
findByCreatedByInAndStatusCodeNot and/or add an explicit `@Query` on the method
(e.g., using "e.statusCode <> :statusCode") so the repository excludes records
with the supplied statusCode (service calls use StatusCodes.CONSUMED.getCode());
ensure the method signature still accepts `@Param`("userIds") List<String> userIds
and `@Param`("statusCode") String statusCode to match callers.
Comment on lines
+61
to
+62
| public Page<DemographicEntity> findByCreatedByInAndStatusCodeOrderByCreateDateTime( | ||
| @Param("userIds") List<String> userIds, @Param("statusCode") String statusCode, Pageable pageable); |
There was a problem hiding this comment.
Same semantic concern applies to the paginated variant.
This method has the same EQUALS vs. NOT EQUALS semantic issue as noted above.
🤖 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/DemographicRepository.java`
around lines 61 - 62, The paginated repository method
findByCreatedByInAndStatusCodeOrderByCreateDateTime uses an equality match on
statusCode but should be a NOT-EQUALS filter like the non-paginated version;
rename the method to findByCreatedByInAndStatusCodeNotOrderByCreateDateTime (and
keep the same parameters List<String> userIds, String statusCode, Pageable
pageable) so Spring Data generates a "statusCode <> ?" predicate for pagination
as well.
Comment on lines
+499
to
501
| List<String> lookupIds = userDetailsService.getUserLookupIds(userId, piiBackwardCompatibility); | ||
| List<DemographicEntity> demographicEntities = demographicRepository.findByCreatedByInAndStatusCode(lookupIds, | ||
| StatusCodes.CONSUMED.getCode()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what the original getAllApplicationDetails logic was filtering
echo "=== Original method behavior context ==="
ast-grep --pattern $'demographicRepository.findByCreatedBy($_, $_)'Repository: mosip/pre-registration
Length of output: 106
🏁 Script executed:
# Locate and inspect the DemographicService.java file
fd "DemographicService.java" --type fRepository: mosip/pre-registration
Length of output: 202
🏁 Script executed:
# Once located, read the relevant section and broader context around lines 499-501
# First, let's count lines to understand file size
wc -l "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java"Repository: mosip/pre-registration
Length of output: 206
🏁 Script executed:
# Search for the method containing this code and understand its purpose
rg "findByCreatedByInAndStatusCode" --type java -B 5 -A 5Repository: mosip/pre-registration
Length of output: 13101
🏁 Script executed:
# Look for repository method definitions
fd "DemographicRepository" --type fRepository: mosip/pre-registration
Length of output: 337
🏁 Script executed:
# Search for CONSUMED status code usage and historical patterns
rg "CONSUMED.*getCode\|StatusCodes\.CONSUMED" --type java -B 3 -A 3Repository: mosip/pre-registration
Length of output: 48
🏁 Script executed:
# Read the DemographicRepository to see the `@Query` definitions
cat -n "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.java" | head -150Repository: mosip/pre-registration
Length of output: 3513
🏁 Script executed:
# Search for the specific query definitions for the methods in question
rg "@Query" "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.java" -A 2Repository: mosip/pre-registration
Length of output: 48
🏁 Script executed:
# Let's also look at the broader context around lines 499-501 in DemographicService
sed -n '490,515p' "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java"Repository: mosip/pre-registration
Length of output: 1472
🏁 Script executed:
# Search for any git history or comments about the original logic
rg "<>|not equal|exclude.*CONSUMED" --type java -B 2 -A 2Repository: mosip/pre-registration
Length of output: 50379
🏁 Script executed:
# Find the BaseRepository implementation
fd "BaseRepository" --type fRepository: mosip/pre-registration
Length of output: 48
🏁 Script executed:
# Search for XML files that might contain query definitions
fd ".*\.xml" | xargs grep -l "findByCreatedByInAndStatusCode" 2>/dev/null | head -5Repository: mosip/pre-registration
Length of output: 48
🏁 Script executed:
# Check if there are any custom implementations or query definitions
rg "findByCreatedByInAndStatusCode|<>.*statusCode|StatusCode.*<>" --type java --type xml -B 2 -A 2Repository: mosip/pre-registration
Length of output: 8046
🏁 Script executed:
# Look at broader context in DemographicService to understand if this is new behavior
sed -n '470,530p' "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/service/DemographicService.java"Repository: mosip/pre-registration
Length of output: 3279
🏁 Script executed:
# Check if findByCreatedByInAndStatusCode has `@Query` annotation
rg "findByCreatedByInAndStatusCode|findByCreatedByInAndStatusCodeOrderByCreateDateTime" -B 3 -A 0 "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.java"Repository: mosip/pre-registration
Length of output: 546
🏁 Script executed:
# Check the full DemographicEntity NamedQuery definitions
rg "@NamedQuery" "pre-registration/pre-registration-core/src/main/java/io/mosip/preregistration/core/common/entity/DemographicEntity.java" -A 0Repository: mosip/pre-registration
Length of output: 612
🏁 Script executed:
# Search for any `@Query` definitions in DemographicRepository for the In methods
cat "pre-registration/pre-registration-application-service/src/main/java/io/mosip/preregistration/application/repository/DemographicRepository.java"Repository: mosip/pre-registration
Length of output: 2862
Confirm the semantic change in status filtering logic.
The original queries (findByCreatedBy, findByCreatedByOrderByCreateDateTime) used statusCode <>:statusCode to exclude records matching the status code. The new methods (findByCreatedByInAndStatusCode, findByCreatedByInAndStatusCodeOrderByCreateDateTime) have no @Query or @NamedQuery definitions and follow Spring Data naming conventions that would include records matching the status code.
When the code at lines 499-501 calls findByCreatedByInAndStatusCode(lookupIds, StatusCodes.CONSUMED.getCode()), it now includes CONSUMED records instead of excluding them—a complete inversion of the original filtering logic. Either add explicit @Query annotations to restore the original exclusion logic, or confirm this behavior change is intentional.
🤖 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 499 - 501, The current calls to
demographicRepository.findByCreatedByInAndStatusCode and
findByCreatedByInAndStatusCodeOrderByCreateDateTime now include records with the
given statusCode (e.g., CONSUMED) whereas the original queries excluded that
status; restore the original exclusion semantics by either changing the
repository method signatures to use Spring Data negative naming (e.g.,
findByCreatedByInAndStatusCodeNot and
findByCreatedByInAndStatusCodeNotOrderByCreateDateTime) or add explicit `@Query`
annotations to those methods with "WHERE d.statusCode <> :statusCode" to ensure
CONSUMED records are excluded when the service passes
StatusCodes.CONSUMED.getCode().
Comment on lines
151
to
+152
| log.info("In callsendOtp method of login service with userID: {} and langCode", | ||
| userOtpRequest.getRequest().getUserId(), language); | ||
| maskIdentifier(userOtpRequest.getRequest().getUserId()), language); |
There was a problem hiding this comment.
Minor inconsistency in log message format.
The log message has a slight formatting issue - langCode variable is referenced but not interpolated into the message due to missing placeholder.
🔧 Suggested fix
- log.info("In callsendOtp method of login service with userID: {} and langCode",
- maskIdentifier(userOtpRequest.getRequest().getUserId()), language);
+ log.info("In callsendOtp method of login service with userID: {} and langCode {}",
+ maskIdentifier(userOtpRequest.getRequest().getUserId()), language);🤖 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 151 - 152, The log statement in the callsendOtp method of
LoginService uses maskIdentifier(userOtpRequest.getRequest().getUserId()) and
the language variable but only supplies one placeholder; update the log.info
call to include a second {} placeholder for the language and pass language as
the second argument so the message interpolates both values (keep maskIdentifier
as the first argument and language as the second).
Comment on lines
+597
to
+601
| Mockito.when(demographicRepository.findByCreatedByInAndStatusCode(Mockito.anyList(), Mockito.eq("Consumed"))) | ||
| .thenReturn(userEntityDetails); | ||
| Mockito.when( | ||
| demographicRepository.findByCreatedByOrderByCreateDateTime(Mockito.any(), Mockito.any(), Mockito.any())) | ||
| demographicRepository.findByCreatedByInAndStatusCodeOrderByCreateDateTime(Mockito.anyList(), | ||
| Mockito.eq("Consumed"), Mockito.any())) |
There was a problem hiding this comment.
Repository stub confirms semantic change.
The test stubs findByCreatedByInAndStatusCode(anyList(), eq("Consumed")), confirming the new query will return records with CONSUMED status (equals), not exclude them. This is a behavioral change from the original query that excluded CONSUMED records.
Please verify this is the intended behavior for the getAllApplicationDetails 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/test/java/io/mosip/preregistration/application/service/DemographicServiceTest.java`
around lines 597 - 601, The test's repository stubs in DemographicServiceTest
are now mocking demographicRepository.findByCreatedByInAndStatusCode(...) and
findByCreatedByInAndStatusCodeOrderByCreateDateTime(...) to return entities that
match status "Consumed" (equals), which changes behavior from the prior query
that excluded CONSUMED records; inspect the getAllApplicationDetails method to
confirm whether it should include or exclude CONSUMED, and then update the test
stubs to reflect the intended behavior—if CONSUMED should be excluded, change
the mocks to use the repository method that filters out "Consumed" (e.g., a
not-equals variant or the method previously used) or adjust returned test data
to have non-Consumed statuses; if inclusion is intended, update the test
assertion comments to state the semantic change.
Comment on lines
+119
to
+130
| @Cacheable(value = "user-details-cache", key = "#identifier.toLowerCase().trim()", condition = "#identifier != null") | ||
| public Optional<UserDetails> findByIdentifier(String identifier) { | ||
| String norm = normalize(identifier); | ||
| if (norm == null) { | ||
| return Optional.empty(); | ||
| } | ||
| if (isUuid(norm)) { | ||
| return userDetailsRepository.findById(UUID.fromString(norm)); | ||
| } | ||
| String hash = sha256Hex(norm); | ||
| return userDetailsRepository.findByIdentifierHash(hash); | ||
| } |
There was a problem hiding this comment.
Cache key uses case-sensitive condition but case-insensitive key transformation.
The @Cacheable annotation at line 119 uses condition = "#identifier != null" but the key transformation #identifier.toLowerCase().trim() could throw NPE if identifier is null (even though the condition prevents caching, SpEL key is still evaluated).
🔧 Suggested fix - use unless instead of condition, or guard the key
-@Cacheable(value = "user-details-cache", key = "#identifier.toLowerCase().trim()", condition = "#identifier != null")
+@Cacheable(value = "user-details-cache", key = "#identifier != null ? `#identifier.toLowerCase`().trim() : ''", unless = "#identifier == null")🤖 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 119 - 130, The cache key expression in
UserDetailsService.findByIdentifier can cause an NPE because `@Cacheable`'s key
"#identifier.toLowerCase().trim()" will be evaluated even when
condition="#identifier != null" prevents caching; update the annotation to
either replace condition with unless = "#identifier == null" so the key isn't
evaluated when identifier is null, or make the key null-safe (e.g., use a
ternary-style guard in the key expression to return null when identifier is
null) to avoid calling toLowerCase() on a null identifier.
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
3 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
New Features
Privacy & Security
Bug Fixes
Tests