Bug/fix full immunization 0 1#101
Conversation
WalkthroughThree methods in ChildCareServiceImpl were modified: incentive processing in getChildVaccinationDetails shifted to operate on the full list; saveOrsDistributionDetails changed user identification from JWT storage to DTO source; checkAndAddOrdDistributionIncentive updated username derivation to use database lookup instead of JWT storage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java (2)
335-351: Based on my verification, all concerns in the review comment are confirmed and accurate:
O(N²) Performance Issue: ✓ Confirmed
checkAndAddIncentives(vaccinationDetails)is called at line 348 inside the forEach loop- For N items in
vaccinationDetails, it executes N times- Each execution iterates over the entire list again (line 834)
- Each iteration performs multiple DB queries and calls
recordRepo.save()Side-Effect Concern: ✓ Confirmed
getChildVaccinationDetailsis a read API but performs writes viacreateIncentiveRecord()→recordRepo.save()- This violates the principle that read operations shouldn't have side effects
Correct Pattern Exists: ✓ Confirmed
saveChildVaccinationDetails(line 380) correctly callscheckAndAddIncentives(vaccinationList)once after the loop ends- This is the pattern that should be used in
getChildVaccinationDetailsThe suggested diff is correct and matches the proper implementation pattern already used elsewhere in the file.
Avoid calling
checkAndAddIncentivesinside the per-item loop
checkAndAddIncentives(vaccinationDetails)is invoked once per element invaccinationDetails, but each call re-iterates over the entire list and performs database queries and writes multiple times. This is O(N²) work and repeats side-effectful incentive logic on the same data.If the intention is to process incentives once per request, call it once after you finish building
result:vaccinationDetails.forEach(childVaccination -> { ChildVaccinationDTO vaccinationDTO = mapper.convertValue(childVaccination, ChildVaccinationDTO.class); BigInteger benId = beneficiaryRepo.getBenIdFromRegID(childVaccination.getBeneficiaryRegId()); vaccinationDTO.setBeneficiaryId(benId.longValue()); result.add(vaccinationDTO); - checkAndAddIncentives(vaccinationDetails); }); + checkAndAddIncentives(vaccinationDetails); return result;Also, this
get*method now performs writes (creates incentive records). Please confirm this side effect is acceptable for a read API and won't surprise callers.
568-581: Based on my investigation, I have verified the security concerns raised in the review comment. Here's what I found:Critical Findings:
The endpoint is unauthenticated: The
JwtUserIdValidationFilterhas a hardcodedif (true)statement (line 84-87) that bypasses all JWT validation for every endpoint, including/ors/saveAll.Inconsistent user attribution pattern: Other methods in the codebase (lines 534, 460-463, 690, 717, 723) correctly use
jwtUtil.getUserNameFromStorage()to derive the userId from the authenticated user. ThesaveOrsDistributionDetailsmethod diverges from this pattern.No validation of payload userName: The controller simply passes the
orsDistributionDTOSdirectly without validating thatorsDistributionDTO.getUserName()is legitimate.Incentive creation risk: The method calls
checkAndAddOrdDistributionIncentive(), which means a malicious client could attribute ORS distributions and associated incentives to arbitrary users.
Be explicit about trusting
orsDistributionDTO.getUserName()as the user identityThe endpoint POST
/ors/saveAllis currently unauthenticated and uses the username from the request payload without validation:orsDistribution.setUserId(userRepo.getUserIdByName(orsDistributionDTO.getUserName()));This deviates from the established pattern in the codebase (e.g.,
BeneficiaryServiceImpl, otherChildCareServiceImplmethods) which correctly usejwtUtil.getUserNameFromStorage()to derive the authenticated user.Without validation, a malicious or buggy client can attribute ORS distributions and associated incentives to arbitrary users, enabling privilege escalation or fraud.
Recommendation: Either authenticate the endpoint and use
jwtUtil.getUserNameFromStorage()to establish the user identity, or add explicit validation thatorsDistributionDTO.getUserName()is non-null, known, and belongs to an authorized user.
🧹 Nitpick comments (1)
src/main/java/com/iemr/flw/service/impl/ChildCareServiceImpl.java (1)
731-751: Avoid duplicate username lookups and handle missinguserIdin ORS incentive creationIn
checkAndAddOrdDistributionIncentive, you now derivecreatedByfromuserRepo.getUserNamedByUserId(orsDistribution.getUserId())for both AM and CH incentives:createIncentiveRecordforOrsDistribution( orsDistribution, orsDistribution.getBeneficiaryId(), orsPacketActivityAM, userRepo.getUserNamedByUserId(orsDistribution.getUserId()), false ); ... createIncentiveRecordforOrsDistribution( orsDistribution, orsDistribution.getBeneficiaryId(), orsPacketActivityCH, userRepo.getUserNamedByUserId(orsDistribution.getUserId()), true );Two points:
userRepo.getUserNamedByUserIdis called twice per record with the sameuserId. Cache it locally:orsDistributionList.forEach(orsDistribution -> { + String createdBy = userRepo.getUserNamedByUserId(orsDistribution.getUserId()); + if (createdBy == null) { + logger.warn("No username found for userId {}", orsDistribution.getUserId()); + return; + } IncentiveActivity orsPacketActivityAM = incentivesRepo.findIncentiveMasterByNameAndGroup("ORS_DISTRIBUTION", GroupName.CHILD_HEALTH.getDisplayName()); IncentiveActivity orsPacketActivityCH = incentivesRepo.findIncentiveMasterByNameAndGroup("ORS_DISTRIBUTION", GroupName.ACTIVITY.getDisplayName()); if (orsPacketActivityAM != null) { if (orsDistribution.getNumOrsPackets() != null) { - createIncentiveRecordforOrsDistribution(orsDistribution, orsDistribution.getBeneficiaryId(), orsPacketActivityAM, userRepo.getUserNamedByUserId(orsDistribution.getUserId()), false); + createIncentiveRecordforOrsDistribution(orsDistribution, orsDistribution.getBeneficiaryId(), orsPacketActivityAM, createdBy, false); } } if (orsPacketActivityCH != null) { if (orsDistribution.getNumOrsPackets() != null) { - createIncentiveRecordforOrsDistribution(orsDistribution, orsDistribution.getBeneficiaryId(), orsPacketActivityCH, userRepo.getUserNamedByUserId(orsDistribution.getUserId()), true); + createIncentiveRecordforOrsDistribution(orsDistribution, orsDistribution.getBeneficiaryId(), orsPacketActivityCH, createdBy, true); } } });
- If
orsDistribution.getUserId()is ever null or invalid (e.g., because the DTO’s username didn’t resolve),createdBybecomes null andcreateIncentiveRecordforOrsDistributionwill attemptbeneficiaryRepo.getUserIDByUserName(createdBy), which can fail. The guard above (early return with a warning) avoids that.Please verify that
userIdis always set before this method is called, or add similar defensive checks.
1 participant
📋 Description
JIRA ID:
fix full immunization for 1 year and 2 year
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.