PII issues fixes with backward compatibility for booking service [MOSIP-44379]#1091
Draft
GOKULRAJ136 wants to merge 11 commits into
Draft
PII issues fixes with backward compatibility for booking service [MOSIP-44379]#1091GOKULRAJ136 wants to merge 11 commits into
GOKULRAJ136 wants to merge 11 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>
WalkthroughAdds canonical user-ID resolution to booking writes via UserDetailsService, wires ApplicationIdentityMigrationService into BookingService flows, bumps kernel and preregistration app dependencies, adds a PII backward-compatibility flag, and extends unit tests and config for these behaviors. ChangesBooking user-id resolution and kernel bump
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthContext
participant BookingService
participant BookingServiceUtil
participant UserDetailsService
participant AppIdentityMigration
Client->>AuthContext: obtain rawUserId
Client->>BookingService: create/cancel/delete booking (rawUserId, preregId)
BookingService->>BookingServiceUtil: bookingEntitySetter(authUserDetails().getUserId(), ...)
BookingServiceUtil->>UserDetailsService: getOrCreateInternalUserId(rawUserId)
alt resolved
UserDetailsService-->>BookingServiceUtil: effectiveUserId
BookingServiceUtil-->>BookingService: RegistrationBookingEntity.crBy=effectiveUserId
else lookup fails and piiBackwardCompatibility==true
UserDetailsService-->>BookingServiceUtil: (fallback) identifierOrRaw
BookingServiceUtil-->>BookingService: RegistrationBookingEntity.crBy=identifierOrRaw
else lookup fails and piiBackwardCompatibility==false
BookingServiceUtil-->>BookingService: throw AppointmentBookingFailedException
end
BookingService->>AppIdentityMigration: migrateRawUserToEffectiveUser(preregId, bookingEntity.getCrBy())
AppIdentityMigration-->>BookingService: migration result
BookingService-->>Client: respond
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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
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