Restricting one ben with one healthId #109
Merged
Conversation
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
Contributor
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthIDServiceImpl
participant Repository
Client->>HealthIDServiceImpl: mapHealthIDToBeneficiary(request)
HealthIDServiceImpl->>Repository: find beneficiary IDs linked to health ID
alt Health ID already linked
HealthIDServiceImpl-->>Client: Return "already linked" message
else No existing link
HealthIDServiceImpl->>Repository: Save BenHealthIDMapping
HealthIDServiceImpl->>Repository: Check if health ID exists
alt Health ID missing
HealthIDServiceImpl->>Repository: Save HealthIDResponse profile
end
HealthIDServiceImpl-->>Client: Return saved mapping as JSON
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (2)
76-78: Hard-coded string response & potential information leak.Returning the literal message
"HealthId is already linked to other beneficiary ID":
- Couples business logic with presentation – use an enum / constant or a proper response DTO.
- Discloses existence of the healthId to unauthorised callers (privacy concern). Consider returning a generic conflict/error code (HTTP 409) instead.
108-111: Possible NPE when middle name is absent.
abhaProfileJson.get("middleName").getAsString()will throw ifmiddleNameis missing ornull.
Use safe extraction:String middle = abhaProfileJson.has("middleName") && !abhaProfileJson.get("middleName").isJsonNull() ? abhaProfileJson.get("middleName").getAsString() : ""; healthID.setName( abhaProfileJson.get("firstName").getAsString() + " " + middle + " " + abhaProfileJson.get("lastName").getAsString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#68
File: src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java:0-0
Timestamp: 2025-04-08T10:35:30.482Z
Learning: When no beneficiary IDs are found in the getMappedBenIdForHealthId method, returning "No Beneficiary Found" as a string response is the expected behavior rather than treating it as an error condition.
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#68
File: src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java:0-0
Timestamp: 2025-04-08T10:35:30.482Z
Learning: When no beneficiary IDs are found in the getMappedBenIdForHealthId method, returning "No Beneficiary Found" as a string response is the expected behavior rather than treating it as an error condition.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: style-check / checkstyle
- GitHub Check: Analyze (java)
- GitHub Check: Build
- GitHub Check: Package-test
Comment on lines
+73
to
+78
| if (health.getHealthIdNumber() != null) { | ||
| beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(health.getHealthIdNumber()); | ||
|
|
||
| for (int i = 0; i < phrAddressArray.size(); i++) { | ||
| abhaAddressBuilder.append(phrAddressArray.get(i).getAsString()); | ||
| if (i < phrAddressArray.size() - 1) { | ||
| abhaAddressBuilder.append(", "); | ||
| } | ||
| } | ||
| healthID.setHealthId(abhaAddressBuilder.toString()); | ||
| healthID.setName( | ||
| abhaProfileJson.get("firstName").getAsString() + " " + abhaProfileJson.get("middleName").getAsString() + " " + abhaProfileJson.get("lastName").getAsString()); | ||
| SimpleDateFormat simpleDateFormat = new SimpleDateFormat("dd-MM-yyyy"); | ||
| Date date = simpleDateFormat.parse(abhaProfileJson.get("dob").getAsString()); | ||
| SimpleDateFormat year = new SimpleDateFormat("yyyy"); | ||
| SimpleDateFormat month = new SimpleDateFormat("MM"); | ||
| SimpleDateFormat day = new SimpleDateFormat("dd"); | ||
| healthID.setYearOfBirth(year.format(date)); | ||
| healthID.setMonthOfBirth(month.format(date)); | ||
| healthID.setDayOfBirth(day.format(date)); | ||
| healthID.setCreatedBy(jsonRequest.get("createdBy").getAsString()); | ||
| healthID.setProviderServiceMapID(jsonRequest.get("providerServiceMapId").getAsInt()); | ||
| healthID.setIsNewAbha(jsonRequest.get("isNew").getAsBoolean()); | ||
| healthIDRepo.save(healthID); | ||
| if (beneficiaryIdsList != null && beneficiaryIdsList.length > 0) { | ||
| return "HealthId is already linked to other beneficiary ID"; | ||
| } else { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race condition: “check-then-save” is not atomic.
Between fetching the existing beneficiary IDs (line 74) and persisting the new mapping (later), another thread could insert a row, bypassing this safeguard and again linking the same healthId to a different beneficiary.
Use a database-level unique constraint on health_id_number (preferred) or wrap the block in a @Transactional method with ISOLATION.SERIALIZABLE / PESSIMISTIC_WRITE locking to guarantee consistency.
🤖 Prompt for AI Agents
In src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java around
lines 73 to 78, the current check-then-save logic for verifying if a
healthIdNumber is already linked is not atomic, causing a race condition. To fix
this, add a unique constraint on the health_id_number column at the database
level to prevent duplicates. Alternatively, annotate the method with
@Transactional and use an isolation level of SERIALIZABLE or apply
PESSIMISTIC_WRITE locking to ensure the check and save operations are atomic and
consistent.
Comment on lines
82
to
87
| if (health.getBeneficiaryID() != null) { | ||
| Long check1 = benHealthIDMappingRepo.getBenRegID(health.getBeneficiaryID()); | ||
| health.setBeneficiaryRegID(check1); | ||
| health = benHealthIDMappingRepo.save(health); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safety & naming.
check1 may be null if getBenRegID finds no record, leading to NULL being persisted in beneficiaryRegID.
Also, the variable name check1 is non-descriptive.
-Long check1 = benHealthIDMappingRepo.getBenRegID(health.getBeneficiaryID());
-health.setBeneficiaryRegID(check1);
+Long beneficiaryRegId = benHealthIDMappingRepo.getBenRegID(health.getBeneficiaryID());
+if (beneficiaryRegId == null) {
+ throw new FHIRException("Invalid beneficiaryID – no matching registration ID found");
+}
+health.setBeneficiaryRegID(beneficiaryRegId);📝 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
| if (health.getBeneficiaryID() != null) { | |
| Long check1 = benHealthIDMappingRepo.getBenRegID(health.getBeneficiaryID()); | |
| health.setBeneficiaryRegID(check1); | |
| health = benHealthIDMappingRepo.save(health); | |
| } | |
| } | |
| if (health.getBeneficiaryID() != null) { | |
| - Long check1 = benHealthIDMappingRepo.getBenRegID(health.getBeneficiaryID()); | |
| - health.setBeneficiaryRegID(check1); | |
| + Long beneficiaryRegId = benHealthIDMappingRepo.getBenRegID(health.getBeneficiaryID()); | |
| + if (beneficiaryRegId == null) { | |
| + throw new FHIRException("Invalid beneficiaryID – no matching registration ID found"); | |
| + } | |
| + health.setBeneficiaryRegID(beneficiaryRegId); | |
| health = benHealthIDMappingRepo.save(health); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java around
lines 82 to 87, the variable 'check1' is non-descriptive and may be null if
getBenRegID returns no record, causing null to be saved in beneficiaryRegID.
Rename 'check1' to a meaningful name like 'beneficiaryRegID' and add a null
check before setting beneficiaryRegID and saving the entity to avoid persisting
null values.
|
helenKaryamsetty
changed the title
ravishanigarapu
approved these changes
Jun 18, 2025
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
📋 Description
JIRA ID: AMM-1610
Added check to verify if that healthId number has any beneficiaries linked and adding only if not present
✅ 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
Bug Fixes
Other