API to fetch beneficary linked with ABHA ID#68
API to fetch beneficary linked with ABHA ID#68ravishanigarapu merged 36 commits intoPSMRI:developfrom
Conversation
WalkthroughThis pull request introduces a new REST endpoint for retrieving beneficiary IDs using a health ID number. The Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/main/java/com/wipro/fhir/service/healthID/HealthIDService.java (1)
30-30: Consider adding exception handling to method signature.The newly added method doesn't declare that it throws FHIRException, unlike other methods in the interface. If database errors occur or the health ID doesn't exist, the implementation may need to handle exceptions.
-public String getMappedBenIdForHealthId(String healthIdNumber); +public String getMappedBenIdForHealthId(String healthIdNumber) throws FHIRException;src/main/java/com/wipro/fhir/repo/healthID/BenHealthIDMappingRepo.java (2)
48-49: Consider using consistent return types for entity IDs.The query returns
beneficiaryRegIDwhich might be a numeric type in the entity, but the method returnsString[]. This could cause type conversion issues.@Query("SELECT bvd.beneficiaryRegID from BenHealthIDMapping bvd WHERE bvd.healthIdNumber = :healthIdNumber") -public String[] getBenIdForHealthId(@Param("healthIdNumber") String healthIdNumber); +public Long[] getBenIdForHealthId(@Param("healthIdNumber") String healthIdNumber);
51-52: Use JPQL instead of native SQL for database portability.Native SQL queries are database-specific and may cause issues if the schema changes or if you migrate to a different database.
-@Query(value = "SELECT BeneficiaryID FROM db_identity.m_beneficiaryregidmapping where BenRegId in (:benIds)", nativeQuery = true) -public String[] getBeneficiaryIds(@Param("benIds") String[] benIds); +@Query("SELECT m.beneficiaryID FROM BeneficiaryRegIDMapping m WHERE m.benRegId IN (:benIds)") +public String[] getBeneficiaryIds(@Param("benIds") String[] benIds);Note: This assumes there's an entity class
BeneficiaryRegIDMappingthat maps to the tabledb_identity.m_beneficiaryregidmapping. Adjust the entity and property names accordingly.src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java (1)
143-167: Consider using more specific exception handling.The current implementation catches all exceptions which may hide specific issues. Consider using more specific exception types.
try { // existing code -} catch (Exception e) { +} catch (JSONException e) { + response.setError(4000, "Invalid JSON format: " + e.getMessage()); + logger.error("NDHM_FHIR Error parsing JSON request:", e); +} catch (RuntimeException e) { response.setError(5000, e.getMessage()); logger.error("NDHM_FHIR Error while getting beneficiary Ids for HealthID:", e); +} catch (Exception e) { + response.setError(5000, "Unexpected error occurred"); + logger.error("NDHM_FHIR Unexpected error while processing request:", e); }🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 150-150: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java(1 hunks)src/main/java/com/wipro/fhir/repo/healthID/BenHealthIDMappingRepo.java(1 hunks)src/main/java/com/wipro/fhir/service/healthID/HealthIDService.java(1 hunks)src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java (3)
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java (1)
CrossOrigin(19-79)src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithBio.java (1)
CrossOrigin(20-97)src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithUID.java (1)
CrossOrigin(41-169)
🪛 GitHub Check: SonarCloud
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java
[notice] 150-150: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
| public String getMappedBenIdForHealthId(String healthIdNumber) { | ||
| String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber); | ||
|
|
||
| if(beneficiaryIdsList.length > 0) { | ||
| String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList); | ||
| return String.join(",", benIds); | ||
| } | ||
| return "No Beneficiary Found"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check and improve response format.
The implementation has potential issues:
- Doesn't handle null values from repository calls
- Returns a hard-coded message instead of a consistent response format
- Missing exception handling
@Override
public String getMappedBenIdForHealthId(String healthIdNumber) {
- String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber);
-
- if(beneficiaryIdsList.length > 0) {
- String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList);
- return String.join(",", benIds);
- }
- return "No Beneficiary Found";
+ try {
+ String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber);
+
+ if(beneficiaryIdsList != null && beneficiaryIdsList.length > 0) {
+ String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList);
+ return String.join(",", benIds);
+ }
+ return ""; // Return empty string for no results
+ } catch (Exception e) {
+ logger.error("Error retrieving beneficiary IDs for health ID: " + healthIdNumber, e);
+ throw new RuntimeException("Failed to retrieve beneficiary information", e);
+ }
}📝 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.
| public String getMappedBenIdForHealthId(String healthIdNumber) { | |
| String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber); | |
| if(beneficiaryIdsList.length > 0) { | |
| String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList); | |
| return String.join(",", benIds); | |
| } | |
| return "No Beneficiary Found"; | |
| } | |
| @Override | |
| public String getMappedBenIdForHealthId(String healthIdNumber) { | |
| try { | |
| String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber); | |
| if (beneficiaryIdsList != null && beneficiaryIdsList.length > 0) { | |
| String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList); | |
| return String.join(",", benIds); | |
| } | |
| return ""; // Return empty string for no results | |
| } catch (Exception e) { | |
| logger.error("Error retrieving beneficiary IDs for health ID: " + healthIdNumber, e); | |
| throw new RuntimeException("Failed to retrieve beneficiary information", e); | |
| } | |
| } |
There was a problem hiding this comment.
@coderabbitai if i am not getting any beneficiary ID in response its not an error. so, i am just returning as no beneficiary found
There was a problem hiding this comment.
@helenKaryamsetty I understand your design choice. Returning "No Beneficiary Found" as a clear message is perfectly valid when no beneficiary IDs are found. I still recommend adding a null check and some basic exception handling for robustness:
@Override
public String getMappedBenIdForHealthId(String healthIdNumber) {
try {
String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber);
if(beneficiaryIdsList != null && beneficiaryIdsList.length > 0) {
String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList);
return String.join(",", benIds);
}
return "No Beneficiary Found";
} catch (Exception e) {
logger.error("Error retrieving beneficiary IDs for health ID: " + healthIdNumber, e);
throw new RuntimeException("Failed to retrieve beneficiary information", e);
}
}This approach keeps your intended message while protecting against potential null values and unexpected exceptions from the repository layer.
✏️ Learnings added
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.289Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| response.setError(5000, e.getMessage()); | ||
| logger.error("NDHM_FHIR Error while getting beneficiary Ids for HealthID:" + e); | ||
| } | ||
| logger.info("NDHM_FHIR get beneficiary Ids for HealthID response:" + response.toString()); |
There was a problem hiding this comment.
Avoid logging sensitive data in response.
Similar to the request logging, avoid logging the full response which might contain sensitive information.
-logger.info("NDHM_FHIR get beneficiary Ids for HealthID response:" + response.toString());
+logger.info("NDHM_FHIR get beneficiary Ids for HealthID completed successfully");📝 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.
| logger.info("NDHM_FHIR get beneficiary Ids for HealthID response:" + response.toString()); | |
| logger.info("NDHM_FHIR get beneficiary Ids for HealthID completed successfully"); |
There was a problem hiding this comment.
@coderabbitai i want those IDs to be logged for future debugging purpose. and those IDs are not any threat to secure.
There was a problem hiding this comment.
@helenKaryamsetty I understand your reasoning - logging the IDs serves an important debugging purpose, and you've confirmed they don't present a security concern. Thank you for providing this context. I'll withdraw my suggestion and respect your implementation decision.
✏️ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#68
File: src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java:165-165
Timestamp: 2025-04-08T10:36:01.980Z
Learning: In the FHIR-API project, logging beneficiary IDs in response logs (e.g., "NDHM_FHIR get beneficiary Ids for HealthID response:") is acceptable and useful for debugging purposes as these IDs do not pose a security threat according to the domain expert.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
| if (request != null) { | ||
| String healthId = obj.getString("healthIdNumber"); | ||
| String res = healthIDService.getMappedBenIdForHealthId(healthId); | ||
| response.setResponse(res); | ||
| } else { | ||
| logger.info("NDHM_FHIR Invalid Request Data."); | ||
| response.setError(5000, "NDHM_FHIR Invalid request"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve request validation and fix conditional logic.
The current implementation has issues:
- The null check on
requestis redundant - No validation on health ID format
- Missing or empty healthIdNumber handling
-JSONObject obj = new JSONObject(request);
-if (request != null) {
- String healthId = obj.getString("healthIdNumber");
- String res = healthIDService.getMappedBenIdForHealthId(healthId);
- response.setResponse(res);
-} else {
- logger.info("NDHM_FHIR Invalid Request Data.");
- response.setError(5000, "NDHM_FHIR Invalid request");
-}
+JSONObject obj = new JSONObject(request);
+if (obj.has("healthIdNumber") && !obj.getString("healthIdNumber").trim().isEmpty()) {
+ String healthId = obj.getString("healthIdNumber");
+ String res = healthIDService.getMappedBenIdForHealthId(healthId);
+ if (res == null || res.isEmpty()) {
+ response.setResponse("No beneficiaries found for the provided Health ID");
+ } else {
+ response.setResponse(res);
+ }
+} else {
+ logger.info("NDHM_FHIR Invalid Request Data: Missing or empty healthIdNumber");
+ response.setError(5000, "NDHM_FHIR Invalid request: healthIdNumber is required");
+}📝 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.
| if (request != null) { | |
| String healthId = obj.getString("healthIdNumber"); | |
| String res = healthIDService.getMappedBenIdForHealthId(healthId); | |
| response.setResponse(res); | |
| } else { | |
| logger.info("NDHM_FHIR Invalid Request Data."); | |
| response.setError(5000, "NDHM_FHIR Invalid request"); | |
| } | |
| JSONObject obj = new JSONObject(request); | |
| if (obj.has("healthIdNumber") && !obj.getString("healthIdNumber").trim().isEmpty()) { | |
| String healthId = obj.getString("healthIdNumber"); | |
| String res = healthIDService.getMappedBenIdForHealthId(healthId); | |
| if (res == null || res.isEmpty()) { | |
| response.setResponse("No beneficiaries found for the provided Health ID"); | |
| } else { | |
| response.setResponse(res); | |
| } | |
| } else { | |
| logger.info("NDHM_FHIR Invalid Request Data: Missing or empty healthIdNumber"); | |
| response.setError(5000, "NDHM_FHIR Invalid request: healthIdNumber is required"); | |
| } |
There was a problem hiding this comment.
@coderabbitai if no response is coming then i handled in service layer and i want to response to be returned what ever i am returning from servicelayer.
There was a problem hiding this comment.
@helenKaryamsetty I understand your approach now. You're right - if you've already handled empty response cases in the service layer with the "No Beneficiary Found" message, then adding controller-level handling would be redundant and override your intended design pattern. The controller should simply pass through whatever the service layer returns. Thanks for the clarification!
🧠 Learnings used
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#68
File: src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java:165-165
Timestamp: 2025-04-08T10:36:02.237Z
Learning: In the FHIR-API project, logging beneficiary IDs in response logs (e.g., "NDHM_FHIR get beneficiary Ids for HealthID response:") is acceptable and useful for debugging purposes as these IDs do not pose a security threat according to the domain expert.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (1)
181-190:⚠️ Potential issueAdd null check and improve error handling.
The current implementation has several issues that could lead to runtime failures:
- No null check before accessing
beneficiaryIdsList.length- Missing exception handling
- Inconsistent return format (array string vs plain text message)
These issues were previously identified but not addressed in this implementation.
Apply this improvement:
@Override public String getMappedBenIdForHealthId(String healthIdNumber) { - String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber); - - if(beneficiaryIdsList.length > 0) { - String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList); - return Arrays.toString(benIds); - } - return "No Beneficiary Found"; + try { + String[] beneficiaryIdsList = benHealthIDMappingRepo.getBenIdForHealthId(healthIdNumber); + + if(beneficiaryIdsList != null && beneficiaryIdsList.length > 0) { + String[] benIds = benHealthIDMappingRepo.getBeneficiaryIds(beneficiaryIdsList); + return Arrays.toString(benIds); + } + return "No Beneficiary Found"; + } catch (Exception e) { + logger.error("Error retrieving beneficiary IDs for health ID: " + healthIdNumber, e); + throw new RuntimeException("Failed to retrieve beneficiary information", e); + } }
🧹 Nitpick comments (1)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (1)
187-187: Consider using a more consistent response format.Using
Arrays.toString()returns a string with brackets and spaces like "[id1, id2, id3]" which may require additional parsing by clients. Consider usingString.join(",", benIds)for a cleaner comma-separated format like "id1,id2,id3".- return Arrays.toString(benIds); + return String.join(",", benIds);
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (1)
26-26: Appropriate import addition.The import of
java.util.Arraysis correctly added to support theArrays.toString()method used in the new implementation.
|
3 participants
📋 Description
JIRA ID: AMM-1299
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ 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