PII issues fixes with backward compatibility for booking service [MOSIP-44379]#1091
Draft
GOKULRAJ136 wants to merge 12 commits into
Draft
PII issues fixes with backward compatibility for booking service [MOSIP-44379]#1091GOKULRAJ136 wants to merge 12 commits into
GOKULRAJ136 wants to merge 12 commits into
Conversation
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>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds canonical user-ID resolution to booking writes via UserDetailsService, wires ApplicationIdentityMigrationService into BookingService create/cancel/delete flows, adds a PII backward-compatibility flag and properties, updates unit tests to mock and validate resolution/failure, and adds a preregistration application-service dependency to the POM. ChangesBooking identity resolution and migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant BookingService
participant BookingServiceUtil
participant UserDetailsService
participant ApplicationIdentityMigrationService
Client->>BookingService: create/cancel/delete booking request (rawUserId, preregId)
BookingService->>BookingServiceUtil: bookingEntitySetter(authUserId, ...)
BookingServiceUtil->>UserDetailsService: getOrCreateInternalUserId(rawUserId)
alt resolved
UserDetailsService-->>BookingServiceUtil: effectiveUserId
BookingServiceUtil-->>BookingService: RegistrationBookingEntity (crBy=effectiveUserId)
else lookup fails
UserDetailsService--xBookingServiceUtil: throws UserLookupException
BookingServiceUtil-->>BookingService: throws AppointmentBookingFailedException
end
BookingService->>ApplicationIdentityMigrationService: migrateRawUserToEffectiveUser(preregId, bookingEntity.getCrBy())
ApplicationIdentityMigrationService-->>BookingService: migration result
BookingService-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pre-registration-booking-service/pom.xmlpre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.javapre-registration-booking-service/src/main/resources/bootstrap.propertiespre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.javapre-registration-booking-service/src/test/resources/application.properties
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java (1)
558-575:⚠️ Potential issue | 🟠 MajorGuard against blank raw fallback in backward-compatibility mode.
On Line [568]-Line [571], fallback returns
userIdeven when it may be null/blank, which can still persist an unattributablecrBy. Fail fast unless a non-blank identifier is available.Proposed fix
private String resolveEffectiveCrBy(String userId) { + if (userId == null || userId.isBlank()) { + throw new AppointmentBookingFailedException( + ErrorCodes.PRG_BOOK_RCI_005.getCode(), + ErrorMessages.APPOINTMENT_BOOKING_FAILED.getMessage()); + } try { UserDetails userDetail = userDetailsService.findOrCreateByIdentifier(userId); if (userDetail != null && userDetail.getUserId() != null) { return userDetail.getUserId().toString(); } } catch (Exception ex) { log.warn("sessionId", "idType", "id", "Failed to resolve canonical user id for booking: " + maskIdentifier(userId)); } - if (piiBackwardCompatibility) { + if (piiBackwardCompatibility) { log.warn("sessionId", "idType", "id", "Falling back to raw identifier for booking due to backward-compatibility mode"); return userId; } throw new AppointmentBookingFailedException(ErrorCodes.PRG_BOOK_RCI_005.getCode(), ErrorMessages.APPOINTMENT_BOOKING_FAILED.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java` around lines 558 - 575, The resolveEffectiveCrBy method currently falls back to returning userId when piiBackwardCompatibility is true even if userId is null/blank; update resolveEffectiveCrBy to validate that the raw identifier is non-blank before returning it (use a String non-blank check or StringUtils.isBlank/hasText), log a clear warning (including maskIdentifier(userId)) when the raw id is blank, and if blank throw AppointmentBookingFailedException with ErrorCodes.PRG_BOOK_RCI_005 and ErrorMessages.APPOINTMENT_BOOKING_FAILED; keep the existing catch block and masking behavior but ensure the fallback only returns a non-empty identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java`:
- Around line 558-575: The resolveEffectiveCrBy method currently falls back to
returning userId when piiBackwardCompatibility is true even if userId is
null/blank; update resolveEffectiveCrBy to validate that the raw identifier is
non-blank before returning it (use a String non-blank check or
StringUtils.isBlank/hasText), log a clear warning (including
maskIdentifier(userId)) when the raw id is blank, and if blank throw
AppointmentBookingFailedException with ErrorCodes.PRG_BOOK_RCI_005 and
ErrorMessages.APPOINTMENT_BOOKING_FAILED; keep the existing catch block and
masking behavior but ensure the fallback only returns a non-empty identifier.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.javapre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
… remove raw PII fallback Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/BookingService.java (1)
318-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove debug
System.out.printlnstatement.This is a debug artifact that should be removed before merging. It prints entity contents to stdout which bypasses structured logging and could interfere with log aggregation.
Proposed fix
- System.out.println("Booking Entity " + bookingEntity); -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/BookingService.java` at line 318, Remove the debug System.out.println in BookingService that prints "Booking Entity " + bookingEntity; replace it with nothing (delete the statement) or, if logging is required, use the class logger (e.g., logger.debug or logger.info) to log bookingEntity in structured/logback-compatible form; locate the System.out.println in the BookingService method where bookingEntity is used and delete the println or change it to logger.debug("Booking Entity: {}", bookingEntity).pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java (1)
560-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
piiBackwardCompatibilityflag is declared but never used.The
piiBackwardCompatibilityfield (line 153) is injected but not referenced inresolveEffectiveCrBy. Given the PR title mentions "backward compatibility", this appears to be incomplete. Additionally, ifgetOrCreateInternalUserIdreturnsnullor blank, that value is stored ascrBy, potentially creating records without a valid creator.Consider validating the return value and using the backward-compatibility flag for fallback behavior:
Proposed fix
private String resolveEffectiveCrBy(String userId) { String maskedUserId = GenericUtil.maskIdentifier(userId); try { String effectiveCrBy = userDetailsService.getOrCreateInternalUserId(userId); - boolean canonicalApplied = effectiveCrBy != null && !effectiveCrBy.isBlank() - && !effectiveCrBy.trim().equals(userId == null ? "" : userId.trim()); - log.info("sessionId", "idType", "id", - "Resolved effective user id for booking write. maskedUserId=" + maskedUserId - + ", canonicalApplied=" + canonicalApplied); - return effectiveCrBy; + if (effectiveCrBy != null && !effectiveCrBy.isBlank()) { + boolean canonicalApplied = !effectiveCrBy.trim().equals(userId == null ? "" : userId.trim()); + log.info("sessionId", "idType", "id", + "Resolved effective user id for booking write. maskedUserId=" + maskedUserId + + ", canonicalApplied=" + canonicalApplied); + return effectiveCrBy; + } + // Resolution returned null/blank - fall through to fallback logic } catch (UserLookupException ex) { log.warn("sessionId", "idType", "id", "Failed to resolve effective booking user id for " + maskedUserId); + } + if (piiBackwardCompatibility && userId != null && !userId.isBlank()) { + log.warn("sessionId", "idType", "id", + "Falling back to raw identifier for booking due to backward-compatibility mode"); + return userId; + } - throw new AppointmentBookingFailedException(ErrorCodes.PRG_BOOK_RCI_005.getCode(), - ErrorMessages.APPOINTMENT_BOOKING_FAILED.getMessage()); - } + throw new AppointmentBookingFailedException(ErrorCodes.PRG_BOOK_RCI_005.getCode(), + ErrorMessages.APPOINTMENT_BOOKING_FAILED.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java` around lines 560 - 576, resolveEffectiveCrBy currently calls userDetailsService.getOrCreateInternalUserId but ignores the injected piiBackwardCompatibility flag and may return null/blank causing invalid crBy; update resolveEffectiveCrBy to validate the returned effectiveCrBy (from getOrCreateInternalUserId) and if it is null/blank then, when piiBackwardCompatibility is true, fall back to using the original userId (after masking/normalizing) otherwise log the failure and throw AppointmentBookingFailedException (ErrorCodes.PRG_BOOK_RCI_005) as already used; keep current logging (use GenericUtil.maskIdentifier for maskedUserId) and ensure canonicalApplied logic reflects the final value used for crBy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/BookingService.java`:
- Line 318: Remove the debug System.out.println in BookingService that prints
"Booking Entity " + bookingEntity; replace it with nothing (delete the
statement) or, if logging is required, use the class logger (e.g., logger.debug
or logger.info) to log bookingEntity in structured/logback-compatible form;
locate the System.out.println in the BookingService method where bookingEntity
is used and delete the println or change it to logger.debug("Booking Entity:
{}", bookingEntity).
In
`@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java`:
- Around line 560-576: resolveEffectiveCrBy currently calls
userDetailsService.getOrCreateInternalUserId but ignores the injected
piiBackwardCompatibility flag and may return null/blank causing invalid crBy;
update resolveEffectiveCrBy to validate the returned effectiveCrBy (from
getOrCreateInternalUserId) and if it is null/blank then, when
piiBackwardCompatibility is true, fall back to using the original userId (after
masking/normalizing) otherwise log the failure and throw
AppointmentBookingFailedException (ErrorCodes.PRG_BOOK_RCI_005) as already used;
keep current logging (use GenericUtil.maskIdentifier for maskedUserId) and
ensure canonicalApplied logic reflects the final value used for crBy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7af31152-ff6f-420c-aab4-a01b65a6dfc7
📒 Files selected for processing (4)
pre-registration-booking-service/pom.xmlpre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/BookingService.javapre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.javapre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java
Signed-off-by: Gokulraj C <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java (1)
152-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
piiBackwardCompatibilityis currently a no-op in creator resolution.Line [153] introduces the flag, but Lines [560-576] never read it. That makes strict and compatibility modes identical and always fails booking on lookup failure, which defeats the backward-compatibility behavior this change set introduces.
Proposed fix
private String resolveEffectiveCrBy(String userId) { String maskedUserId = GenericUtil.maskIdentifier(userId); try { String effectiveCrBy = userDetailsService.getOrCreateInternalUserId(userId); - boolean canonicalApplied = effectiveCrBy != null && !effectiveCrBy.isBlank() - && !effectiveCrBy.trim().equals(userId == null ? "" : userId.trim()); - log.info("sessionId", "idType", "id", - "Resolved effective user id for booking write. maskedUserId=" + maskedUserId - + ", canonicalApplied=" + canonicalApplied); - return effectiveCrBy; + if (effectiveCrBy != null && !effectiveCrBy.isBlank()) { + boolean canonicalApplied = !effectiveCrBy.trim().equals(userId == null ? "" : userId.trim()); + log.info("sessionId", "idType", "id", + "Resolved effective user id for booking write. maskedUserId=" + maskedUserId + + ", canonicalApplied=" + canonicalApplied); + return effectiveCrBy; + } } catch (UserLookupException ex) { log.warn("sessionId", "idType", "id", "Failed to resolve effective booking user id for " + maskedUserId); - throw new AppointmentBookingFailedException(ErrorCodes.PRG_BOOK_RCI_005.getCode(), - ErrorMessages.APPOINTMENT_BOOKING_FAILED.getMessage()); } + if (piiBackwardCompatibility && userId != null && !userId.isBlank()) { + log.warn("sessionId", "idType", "id", + "Falling back to raw identifier for booking due to backward-compatibility mode"); + return userId; + } + throw new AppointmentBookingFailedException(ErrorCodes.PRG_BOOK_RCI_005.getCode(), + ErrorMessages.APPOINTMENT_BOOKING_FAILED.getMessage()); }Also applies to: 560-576
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java` around lines 152 - 153, The piiBackwardCompatibility flag introduced on BookingServiceUtil is never used in the creator resolution logic, so update the creator-resolution code path (the block handling PII lookup/failure around the current creator resolution method in BookingServiceUtil) to consult piiBackwardCompatibility: when piiBackwardCompatibility is true, treat lookup failures as non-fatal and apply the backward-compatibility fallback (proceed with booking or use compatible creator fallback), otherwise keep the strict behavior (throw/return failure on lookup error). Modify the branch that currently always fails on lookup to check BookingServiceUtil.piiBackwardCompatibility and branch accordingly, ensuring logs still record the lookup error while allowing the compatible flow when the flag is enabled.pre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java (1)
582-603:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCompatibility-mode test does not validate mode-specific behavior.
Lines [582-603] toggle
piiBackwardCompatibility, but both tests assert the same output. This won’t catch regressions where compatibility mode should diverge from strict mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java` around lines 582 - 603, The two tests toggle piiBackwardCompatibility but assert the same crBy value, so update bookingEntitySetterWithCompatibilityModeWritesUuidTest to validate the compatibility-mode behavior (use BookingServiceUtil.bookingEntitySetter with piiBackwardCompatibility=true) by asserting that entity.getCrBy() is not the legacy fixed "00000000-0000-0000-0000-000000000001" and instead matches the expected UUID behavior (either assertNotEquals the legacy value or assert the value matches a UUID pattern); if determinism is required, mock or inject the UUID provider used by bookingEntitySetter so you can assert the exact UUID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@pre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.java`:
- Around line 152-153: The piiBackwardCompatibility flag introduced on
BookingServiceUtil is never used in the creator resolution logic, so update the
creator-resolution code path (the block handling PII lookup/failure around the
current creator resolution method in BookingServiceUtil) to consult
piiBackwardCompatibility: when piiBackwardCompatibility is true, treat lookup
failures as non-fatal and apply the backward-compatibility fallback (proceed
with booking or use compatible creator fallback), otherwise keep the strict
behavior (throw/return failure on lookup error). Modify the branch that
currently always fails on lookup to check
BookingServiceUtil.piiBackwardCompatibility and branch accordingly, ensuring
logs still record the lookup error while allowing the compatible flow when the
flag is enabled.
In
`@pre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java`:
- Around line 582-603: The two tests toggle piiBackwardCompatibility but assert
the same crBy value, so update
bookingEntitySetterWithCompatibilityModeWritesUuidTest to validate the
compatibility-mode behavior (use BookingServiceUtil.bookingEntitySetter with
piiBackwardCompatibility=true) by asserting that entity.getCrBy() is not the
legacy fixed "00000000-0000-0000-0000-000000000001" and instead matches the
expected UUID behavior (either assertNotEquals the legacy value or assert the
value matches a UUID pattern); if determinism is required, mock or inject the
UUID provider used by bookingEntitySetter so you can assert the exact UUID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80ede35e-a816-4109-8d3e-4df4c68c0a6e
📒 Files selected for processing (3)
pre-registration-booking-service/pom.xmlpre-registration-booking-service/src/main/java/io/mosip/preregistration/booking/service/util/BookingServiceUtil.javapre-registration-booking-service/src/test/java/io/mosip/preregistration/booking/test/service/util/BookingServiceUtilTest.java
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
1 participant
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
Chores
New Features
Tests