abdm search modifications #57
Conversation
WalkthroughThis pull request involves several modifications across different Java classes in a FHIR-related project. The changes primarily focus on renaming a facility ID field from Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
![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
🔭 Outside diff range comments (4)
src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (2)
Line range hint
111-113: Improve exception handling.The catch block swallows exceptions with only basic logging. This could hide critical issues and make debugging difficult.
} catch (Exception e) { - logger.error(e.getMessage()); + logger.error("Failed to process beneficiary data - benRegId: " + resourceRequestHandler.getBeneficiaryRegID(), e); + // Consider adding metrics/monitoring for failed operations }
Line range hint
1-614: Consider refactoring for better maintainability.The class has several maintainability concerns:
- Multiple responsibilities (violates SRP)
- Deprecated methods still in use
- Magic numbers and strings should be constants
Consider splitting this class into focused services:
- ResourceProcessingService
- NotificationService
- AuthenticationService
Also, extract configuration values to constants:
+ private static final int MAX_RETRY_ATTEMPTS = 5; + private static final int RETRY_DELAY_MS = 5000; + private static final int HTTP_STATUS_ACCEPTED = 202;src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
Sensitive data exposure in logs confirmed - Security Risk
The codebase has multiple instances of logging sensitive information including:
- Full request/response objects containing Aadhaar numbers and biometric data
- Mobile numbers and OTP-related information
- Authentication tokens and encrypted data
Immediate actions needed:
- Implement data masking for sensitive fields before logging
- Create a secure logging utility class with sanitization methods
- Remove or mask PII (Personally Identifiable Information) from all logs
- Review log retention policies and access controls
🔗 Analysis chain
Line range hint
1-456: Consider adding comprehensive error handling for ABDM API responses.The service makes multiple HTTP calls to ABDM APIs but only checks for specific status codes (200, 202). Consider implementing:
- Comprehensive error handling for all possible HTTP status codes
- Retry mechanism for transient failures
- Circuit breaker pattern for API calls
Verify proper logging of sensitive information.
The service logs request and response objects that might contain sensitive information (Aadhaar, mobile numbers, etc.). Ensure that sensitive data is properly masked in logs.
Consider implementing request validation.
Add input validation for all request objects to ensure data integrity and security:
- Validate mobile number format
- Validate Aadhaar number format
- Check for required fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential sensitive data logging rg -i "logger.info.*(?:aadhaar|mobile|otp|password|token)" src/Length of output: 9860
src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java (1)
Line range hint
98-100: Enhance error handlingThe catch block should:
- Log the error for debugging
- Avoid exposing internal error messages
- Provide more context about the operation that failed
catch (Exception e) { - throw new FHIRException(e.getMessage()); + log.error("Failed to update ABDM facility ID for visit code: " + requestObj.getVisitCode(), e); + throw new FHIRException("Failed to update ABDM facility ID. Please try again later."); }
🧹 Nitpick comments (6)
src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (2)
215-215: Enhance logging message clarity.The logging statement should be more descriptive about what the values represent (OP consult, diagnostic report, and prescription bundle processing results).
- logger.info("The value of i: " +i + " The value of j: " + j + " The value of k: " + k ); + logger.info("Bundle processing results - OP Consult: " + i + ", Diagnostic Report: " + j + ", Prescription: " + k);
Line range hint
12-24: Enhance security measures for sensitive healthcare data.The code handles sensitive healthcare data and ABDM operations. Consider implementing:
- Input validation for phone numbers and other user inputs
- Rate limiting for SMS notifications
- Audit logging for sensitive operations
- Data masking in logs
Also applies to: 590-614
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
456-456: Remove unnecessary blank line.The blank line at the end of the file is not needed.
src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java (3)
89-89: Use String.isEmpty() for empty string checkReplace
== ""withisEmpty()orStringUtils.isEmpty()for better string comparison.-if(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId() == "") { +if(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId().isEmpty()) {
92-92: Consider adding transaction boundarySince this is updating a database record, consider adding
@Transactionalto ensure data consistency.+@Transactional public String saveAbdmFacilityId(String reqObj) throws FHIRException {
Line range hint
94-97: Improve response handlingThe success/error messages should be constants and potentially include more context (like the facility ID that was updated).
+private static final String SUCCESS_MESSAGE = "ABDM Facility ID %s updated successfully for visit %s"; +private static final String ERROR_MESSAGE = "Failed to update ABDM Facility ID for visit %s"; if(response > 0 ) { - res = "ABDM Facility ID updated successfully"; + res = String.format(SUCCESS_MESSAGE, requestObj.getAbdmFacilityId(), requestObj.getVisitCode()); } else - res = "FHIR Error while updating ABDM Facility ID"; + res = String.format(ERROR_MESSAGE, requestObj.getVisitCode());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java(1 hunks)src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java(1 hunks)src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java(3 hunks)src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java(2 hunks)
🧰 Additional context used
📓 Learnings (3)
src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#46
File: src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java:7-8
Timestamp: 2024-11-20T07:41:40.038Z
Learning: In the `SaveFacilityIdForVisit` class (`src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java`), the handling of the MongoDB ID field and indexing is provided in another module and is not required in this class.
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:84-91
Timestamp: 2024-12-09T16:01:39.809Z
Learning: In the `requestOtpForAbhaLogin` and `verifyAbhaLogin` methods of `LoginAbhaV3ServiceImpl.java`, `loginId` is mandatory, but even if it is null, the process can proceed without input validation.
src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#46
File: src/main/java/com/wipro/fhir/service/facility/FacilityService.java:9-9
Timestamp: 2024-11-20T07:43:15.272Z
Learning: In `FacilityService.java`, for the `saveAbdmFacilityId` method, when the response is fixed, it's acceptable to use `String` as the return type and accept `String` as a parameter, allowing for a generic declaration.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
29-29: LGTM!The addition of JsonArray import is necessary for the new implementation of phrAddress handling.
src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java (1)
10-10: LGTM! Clean field renaming.The renaming from
facilityIdtoabdmFacilityIdis consistent and maintains the same data type. The class structure remains clean with appropriate Lombok usage.src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
Line range hint
1-267: Enhance security and error handling.Based on the context and previous discussions:
- Input validation for
loginIdis still missing- Error messages could be more descriptive
- Security events should be audit logged
Consider these improvements:
- Add input validation:
private void validateLoginId(String loginId) { if (loginId == null || loginId.trim().isEmpty()) { throw new FHIRException("Login ID is required"); } // Add additional validation rules as needed }
- Implement audit logging for security events:
private void auditLog(String event, String userId, boolean success) { logger.info("Security Event - Event: {}, User: {}, Success: {}, Timestamp: {}", event, userId, success, new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").format(new Date())); }Let's verify the usage of audit logging in similar ABDM implementations:
| JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress"); | ||
| StringBuilder abhaAddressBuilder = new StringBuilder(); | ||
|
|
||
| for (int i = 0; i < phrAddressArray.size(); i++) { | ||
| abhaAddressBuilder.append(phrAddressArray.get(i).getAsString()); | ||
| if (i < phrAddressArray.size() - 1) { | ||
| abhaAddressBuilder.append(", "); | ||
| } | ||
| } | ||
| healthIDResp.setHealthId(abhaAddressBuilder.toString()); |
There was a problem hiding this comment.
Consider handling empty or null phrAddress array.
The new implementation properly handles multiple PHR addresses, but it should include null checks and empty array validation to prevent potential NullPointerException.
Apply this diff to add proper validation:
- JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress");
- StringBuilder abhaAddressBuilder = new StringBuilder();
-
- for (int i = 0; i < phrAddressArray.size(); i++) {
- abhaAddressBuilder.append(phrAddressArray.get(i).getAsString());
- if (i < phrAddressArray.size() - 1) {
- abhaAddressBuilder.append(", ");
- }
- }
- healthIDResp.setHealthId(abhaAddressBuilder.toString());
+ StringBuilder abhaAddressBuilder = new StringBuilder();
+ if (profile.has("phrAddress") && !profile.get("phrAddress").isJsonNull()) {
+ JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress");
+ if (phrAddressArray != null && phrAddressArray.size() > 0) {
+ for (int i = 0; i < phrAddressArray.size(); i++) {
+ if (!phrAddressArray.get(i).isJsonNull()) {
+ abhaAddressBuilder.append(phrAddressArray.get(i).getAsString());
+ if (i < phrAddressArray.size() - 1) {
+ abhaAddressBuilder.append(", ");
+ }
+ }
+ }
+ }
+ }
+ healthIDResp.setHealthId(abhaAddressBuilder.toString().trim());📝 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.
| JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress"); | |
| StringBuilder abhaAddressBuilder = new StringBuilder(); | |
| for (int i = 0; i < phrAddressArray.size(); i++) { | |
| abhaAddressBuilder.append(phrAddressArray.get(i).getAsString()); | |
| if (i < phrAddressArray.size() - 1) { | |
| abhaAddressBuilder.append(", "); | |
| } | |
| } | |
| healthIDResp.setHealthId(abhaAddressBuilder.toString()); | |
| StringBuilder abhaAddressBuilder = new StringBuilder(); | |
| if (profile.has("phrAddress") && !profile.get("phrAddress").isJsonNull()) { | |
| JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress"); | |
| if (phrAddressArray != null && phrAddressArray.size() > 0) { | |
| for (int i = 0; i < phrAddressArray.size(); i++) { | |
| if (!phrAddressArray.get(i).isJsonNull()) { | |
| abhaAddressBuilder.append(phrAddressArray.get(i).getAsString()); | |
| if (i < phrAddressArray.size() - 1) { | |
| abhaAddressBuilder.append(", "); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| healthIDResp.setHealthId(abhaAddressBuilder.toString().trim()); |
| if(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId() == "") { | ||
| requestObj.setAbdmFacilityId(abdmFacilityId); | ||
| } | ||
| Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getFacilityId()); | ||
| Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getAbdmFacilityId()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for visitCode
The method should validate that visitCode is not null before proceeding with the update.
if(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId() == "") {
requestObj.setAbdmFacilityId(abdmFacilityId);
}
+if(requestObj.getVisitCode() == null) {
+ throw new FHIRException("Visit code cannot be null");
+}
Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getAbdmFacilityId());📝 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(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId() == "") { | |
| requestObj.setAbdmFacilityId(abdmFacilityId); | |
| } | |
| Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getFacilityId()); | |
| Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getAbdmFacilityId()); | |
| if(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId() == "") { | |
| requestObj.setAbdmFacilityId(abdmFacilityId); | |
| } | |
| if(requestObj.getVisitCode() == null) { | |
| throw new FHIRException("Visit code cannot be null"); | |
| } | |
| Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getAbdmFacilityId()); |
There was a problem hiding this comment.
@coderabbitai it is handled from UI end that visitcode is not null. Hence not checking the null condition again
There was a problem hiding this comment.
@helenKaryamsetty Understood, thanks for clarifying that the visitCode validation is handled at the UI layer. I'll withdraw the suggestion.
✏️ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#57
File: src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java:89-92
Timestamp: 2025-01-07T11:20:36.697Z
Learning: In the FHIR-API project, visitCode validation (null checks) is handled at the UI layer, and therefore doesn't need to be duplicated in the service layer methods like `saveAbdmFacilityId`.
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.
| } else if(jsonResponse.has("users")) { | ||
| responseMap.put("abhaDetails", jsonResponse.get("users").getAsJsonArray().get(0).getAsJsonObject().toString()); | ||
| responseMap.put("txnId", jsonResponse.get("txnId").getAsString()); | ||
| if (jsonResponse.has("tokens") && jsonResponse.get("tokens").isJsonObject()) { | ||
| JsonObject tokensObject = jsonResponse.get("tokens").getAsJsonObject(); | ||
| if (tokensObject.has("token") && !tokensObject.get("token").isJsonNull()) { | ||
| String token = tokensObject.get("token").getAsString(); | ||
| responseMap.put("xToken", token); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and improve error handling.
The current implementation has several potential issues:
- Missing null checks could lead to NullPointerException
- No handling for empty arrays
- Similar logic to "accounts" block suggests potential for refactoring
Consider this safer implementation:
} else if(jsonResponse.has("users")) {
- responseMap.put("abhaDetails", jsonResponse.get("users").getAsJsonArray().get(0).getAsJsonObject().toString());
- responseMap.put("txnId", jsonResponse.get("txnId").getAsString());
- if (jsonResponse.has("tokens") && jsonResponse.get("tokens").isJsonObject()) {
- JsonObject tokensObject = jsonResponse.get("tokens").getAsJsonObject();
- if (tokensObject.has("token") && !tokensObject.get("token").isJsonNull()) {
- String token = tokensObject.get("token").getAsString();
- responseMap.put("xToken", token);
- }
- }
+ JsonArray users = jsonResponse.get("users").getAsJsonArray();
+ if (users == null || users.size() == 0) {
+ throw new FHIRException("No user details found in response");
+ }
+ responseMap.put("abhaDetails", users.get(0).getAsJsonObject().toString());
+ responseMap.put("txnId", jsonResponse.has("txnId") ? jsonResponse.get("txnId").getAsString() : "");
+
+ Optional.ofNullable(jsonResponse.get("tokens"))
+ .filter(JsonElement::isJsonObject)
+ .map(JsonElement::getAsJsonObject)
+ .map(obj -> obj.get("token"))
+ .filter(token -> !token.isJsonNull())
+ .map(JsonElement::getAsString)
+ .ifPresent(token -> responseMap.put("xToken", token));Consider extracting common logic for handling "accounts" and "users" responses into a separate method to improve maintainability:
private void processUserResponse(JsonObject jsonResponse, Map<String, String> responseMap, String arrayKey) {
JsonArray array = jsonResponse.get(arrayKey).getAsJsonArray();
if (array == null || array.size() == 0) {
throw new FHIRException("No " + arrayKey + " details found in response");
}
responseMap.put("abhaDetails", array.get(0).getAsJsonObject().toString());
responseMap.put("txnId", jsonResponse.has("txnId") ? jsonResponse.get("txnId").getAsString() : "");
}There was a problem hiding this comment.
@CodeRabbit it is not neccessary to specifically maintain a separate method for it. Can be integrated inside itself. And no need to check array it is fixed response from abdm side
There was a problem hiding this comment.
@helenKaryamsetty Understood, thank you for clarifying about the ABDM API response structure. I'll retract the suggestion.
✏️ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#57
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:242-251
Timestamp: 2025-01-07T12:49:16.160Z
Learning: The ABDM API response for user verification has a fixed structure where "accounts" and "users" arrays are guaranteed to contain data when present, making null/empty checks unnecessary. This is part of the API contract with ABDM.
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java
Show resolved
Hide resolved
📋 Description : Facility and searchAPI updations
JIRA ID:
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.