Few Modified changes for ABDM Facility #58
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request modifies the Changes
Suggested reviewers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Finishing Touches
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: 1
🔭 Outside diff range comments (3)
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (3)
Line range hint
445-455: Add null checks for name concatenation and date parsing.The name concatenation and date parsing operations could throw NullPointerException or ParseException. Consider adding proper validation:
-healthIDResp.setName( - healthIDResp.getFirstName() + " " + healthIDResp.getMiddleName() + " " + healthIDResp.getLastName()); +StringBuilder nameBuilder = new StringBuilder(); +if (healthIDResp.getFirstName() != null) { + nameBuilder.append(healthIDResp.getFirstName()); +} +if (healthIDResp.getMiddleName() != null) { + if (nameBuilder.length() > 0) nameBuilder.append(" "); + nameBuilder.append(healthIDResp.getMiddleName()); +} +if (healthIDResp.getLastName() != null) { + if (nameBuilder.length() > 0) nameBuilder.append(" "); + nameBuilder.append(healthIDResp.getLastName()); +} +healthIDResp.setName(nameBuilder.toString()); +try { SimpleDateFormat simpleDateFormat = new SimpleDateFormat("dd-MM-yyyy"); Date date = simpleDateFormat.parse(profile.get("dob").getAsString()); SimpleDateFormat year = new SimpleDateFormat("yyyy"); SimpleDateFormat month = new SimpleDateFormat("MM"); SimpleDateFormat day = new SimpleDateFormat("dd"); healthIDResp.setYearOfBirth(year.format(date)); healthIDResp.setMonthOfBirth(month.format(date)); healthIDResp.setDayOfBirth(day.format(date)); +} catch (ParseException e) { + logger.error("Error parsing date of birth: " + e.getMessage()); + throw e; +}
Line range hint
183-184: Remove sensitive data from logs.The code is logging sensitive information including Aadhaar-related data. This could lead to security vulnerabilities if logs are exposed.
-logger.info("ABDM request object for ABHA enrollment by AADHAAR: " + requestObj); +logger.info("ABDM request object for ABHA enrollment by AADHAAR: [REDACTED]"); -logger.info("ABDM request for enroll by Aadhaar: " + enrollByAadhar); +logger.info("ABDM request for enroll by Aadhaar: [REDACTED]"); -logger.info("ABDM request for enroll by biometric: " + enrollByAadhar); +logger.info("ABDM request for enroll by biometric: [REDACTED]");Also applies to: 293-294, 386-387
Line range hint
196-198: Implement proper error handling.Directly exposing exception messages to clients could reveal sensitive implementation details. Consider implementing proper error handling with custom error messages.
-throw new FHIRException(responseEntity.getBody()); +logger.error("Error in ABHA enrollment: " + responseEntity.getBody()); +throw new FHIRException("An error occurred during ABHA enrollment. Please try again later."); -throw new FHIRException(e.getMessage()); +logger.error("Unexpected error: " + e.getMessage(), e); +throw new FHIRException("An unexpected error occurred. Please try again later.");Also applies to: 408-410
🧹 Nitpick comments (9)
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
435-444: Improved handling of multiple PHR addresses.The new implementation properly handles multiple PHR addresses using JsonArray and StringBuilder, which is more robust than the previous string manipulation approach. However, consider adding null checks and empty array validation.
Consider adding validation:
JsonArray phrAddressArray = profile.getAsJsonArray("phrAddress"); +if (phrAddressArray == null || phrAddressArray.size() == 0) { + healthIDResp.setHealthId(""); + return; +} 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(", "); } }src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java (2)
89-90: Improve string comparison and null check.Consider using
StringUtils.isEmpty()orStringUtils.isBlank()for a more robust string validation.-if(requestObj.getAbdmFacilityId() == null || requestObj.getAbdmFacilityId() == "") { +if(StringUtils.isEmpty(requestObj.getAbdmFacilityId())) {
89-92: Enhance error logging for better debugging.Consider adding detailed logging for both success and failure scenarios to aid in troubleshooting.
if(StringUtils.isEmpty(requestObj.getAbdmFacilityId())) { requestObj.setAbdmFacilityId(abdmFacilityId); + log.debug("Using default ABDM facility ID: {}", abdmFacilityId); } Integer response = benHealthIDMappingRepo.updateFacilityIdForVisit(requestObj.getVisitCode(), requestObj.getAbdmFacilityId()); if(response > 0 ) { + log.info("Successfully updated ABDM facility ID for visit: {}", requestObj.getVisitCode()); res = "ABDM Facility ID updated successfully"; - } else + } else { + log.error("Failed to update ABDM facility ID for visit: {}", requestObj.getVisitCode()); res = "FHIR Error while updating ABDM Facility ID"; + }src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java (1)
8-12: Consider adding field validation annotations.Since this class handles ABDM facility IDs which likely follow a specific format, consider adding validation annotations (e.g.,
@NotNull,@Pattern) to ensure data integrity.@Data public class SaveFacilityIdForVisit { + @NotNull(message = "ABDM Facility ID cannot be null") + @Pattern(regexp = "^[A-Za-z0-9-]+$", message = "Invalid ABDM Facility ID format") String abdmFacilityId; + @NotNull(message = "Visit Code cannot be null") BigInteger visitCode; }src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (4)
164-165: Fix typo in logging message.There's a typo in the logging message: "dagetPatientListForResourceEligibleys".
- logger.info("No of records available to create FHIR in last 2 dagetPatientListForResourceEligibleys : " + logger.info("No of records available to create FHIR in last 2 days : "
338-344: Remove commented-out code.Remove the commented-out code block as it's no longer needed and can cause confusion.
-// } -// if (pcc != null && pcc.getIdentifier() != null) { -// ccList = pcc.getCareContextsList(); -// ccList.add(cc); -// pcc.setCareContextsList(ccList); -// resultSet = patientCareContextsMongoRepo.save(pcc); -//
405-413: Remove commented-out code and consider token expiry handling.Remove the commented-out NDHM_AUTH_TOKEN line. The expiry time calculation is good, but consider using it to implement token refresh logic.
- // NDHM_AUTH_TOKEN = "Bearer" + " " + jsnOBJ.get("accessToken").getAsString();Consider implementing a token refresh mechanism using the calculated expiry time.
610-611: Enhance environment mode validation.Consider using an enum for valid modes and throwing an exception for invalid values instead of silently defaulting to "sbx".
- if (abhaMode != null && !(abhaMode.equalsIgnoreCase("abdm") || abhaMode.equalsIgnoreCase("sbx"))) - abhaMode = "sbx"; + if (abhaMode == null) { + throw new IllegalStateException("abhaMode configuration is missing"); + } + String mode = abhaMode.toLowerCase(); + if (!mode.equals("abdm") && !mode.equals("sbx")) { + throw new IllegalStateException("Invalid abhaMode: " + abhaMode + ". Expected: abdm or sbx"); + }src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
242-251: Refactor response handling to reduce code duplication.The handling of "users" and "accounts" fields follows a similar pattern and could be refactored into a common method.
+private void extractResponseDetails(JsonObject jsonResponse, String arrayField, Map<String, String> responseMap) { + responseMap.put("abhaDetails", jsonResponse.get(arrayField).getAsJsonArray().get(0).getAsJsonObject().toString()); + responseMap.put("txnId", jsonResponse.get("txnId").getAsString()); + + if (arrayField.equals("users")) { + if (jsonResponse.has("tokens") && jsonResponse.get("tokens").isJsonObject()) { + JsonObject tokensObject = jsonResponse.get("tokens").getAsJsonObject(); + if (tokensObject.has("token") && !tokensObject.get("token").isJsonNull()) { + responseMap.put("xToken", tokensObject.get("token").getAsString()); + } + } + } else if (jsonResponse.has("token")) { + responseMap.put("xToken", jsonResponse.get("token").getAsString()); + } +}Then use it in the main method:
if (jsonResponse.has("accounts")) { - responseMap.put("abhaDetails", jsonResponse.get("accounts").getAsJsonArray().get(0).getAsJsonObject().toString()); - responseMap.put("txnId", jsonResponse.get("txnId").getAsString()); - if (jsonResponse.has("token")) { - responseMap.put("xToken", jsonResponse.get("token").getAsString()); - } + extractResponseDetails(jsonResponse, "accounts", responseMap); } 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); - } - } + extractResponseDetails(jsonResponse, "users", responseMap); }
📜 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(11 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/facility/FacilityServiceImpl.java (2)
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.
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.850Z
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`.
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (2)
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.294Z
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.
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.
🔇 Additional comments (8)
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
29-29: LGTM!The addition of the JsonArray import is appropriate for the new implementation of PHR address handling.
src/main/java/com/wipro/fhir/service/facility/FacilityServiceImpl.java (1)
89-92: Verify consistent renaming across the codebase.The change from
facilityIdtoabdmFacilityIdshould be consistently applied across all related files.✅ Verification successful
Renaming is consistent across ABDM integration code
The change from
facilityIdtoabdmFacilityIdis consistently implemented across all ABDM-related components. The other instances offacilityID/FacilityIDare unrelated as they belong to the E-Aushadhi integration system and use a different type (Integer vs String).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of facilityId that might need updating rg -i "facilityId" --type java # Search for any mixed usage of both terms rg -i "abdmFacilityId|facilityId" --type javaLength of output: 52406
src/main/java/com/wipro/fhir/data/mongo/care_context/SaveFacilityIdForVisit.java (1)
10-10: Verify MongoDB field mapping for renamed field.The field rename from
facilityIdtoabdmFacilityIdlooks good and aligns with the PR objectives. However, ensure that any MongoDB queries or field mappings are updated accordingly.Let's verify other occurrences of this field name change:
src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (4)
193-197: Improved error message formatting.The error messages are now more descriptive and provide better context for debugging.
288-288: Method signature improvement.Good change removing the unused return type. The method performs an action and doesn't need to return a value.
321-337: Well-implemented duplicate check.Good addition of duplicate check for visit codes to prevent duplicate care contexts.
97-97: Verify the configuration property in application.properties.Please ensure that the
abhaModeproperty is properly configured in the application.properties file with valid values (abdm/sbx).✅ Verification successful
Configuration property
abhaModeis properly configuredThe
abhaModeproperty is correctly set tosbxacross all environment configurations:
- common_test.properties
- common_dev.properties
- common_example.properties
- common_ci.properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if abhaMode property exists in application.properties rg -l "^abhaMode\s*="Length of output: 199
Script:
#!/bin/bash # Check the values of abhaMode in property files rg "^abhaMode\s*=" src/main/environment/common_*.propertiesLength of output: 424
src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
242-251: Verify the difference in token handling between users and accounts responses.The token extraction logic differs between "users" and "accounts" responses. For "users", it checks for a nested "tokens" object with additional null checks, while for "accounts" it directly checks for "token". Please verify if this difference is intentional and aligns with the ABDM API contract.
✅ Verification successful
Token handling implementation is correct and consistent
The different token extraction approaches are intentionally designed to match the API response structure:
- "accounts" responses: Direct
tokenfield access- "users" responses: Nested
tokensobject with null checksThis pattern is consistently implemented across multiple services (LoginAbhaV3, CreateAbhaV3), confirming it aligns with the ABDM API contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other token handling implementations to verify consistency ast-grep --pattern 'if ($response.has("tokens")) { $$$ $token = $$$ $$$ }'Length of output: 82
Script:
#!/bin/bash # Search for token-related patterns in JSON handling echo "=== Searching for token handling patterns ===" rg -U "tokens.*getAsJsonObject|token.*getAsString" -A 5 -B 5 echo -e "\n=== Searching for ABDM API related files ===" fd -t f -e java -e properties -e yml "abdm|abha" --exec grep -l "api" {} \; echo -e "\n=== Searching for similar response processing ===" rg -U "jsonResponse\.has\((\"users\"|\"accounts\")\)" -A 10Length of output: 11241
| String responseStrLogin = common_NDHMService.getBody(responseEntity); | ||
| logger.info("ABDM response for verify abha login: " + responseEntity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider logging only necessary response information.
Logging the entire responseEntity might expose sensitive data in logs. Consider logging only the status code and necessary non-sensitive information.
-logger.info("ABDM response for verify abha login: " + responseEntity);
+logger.info("ABDM response status for verify abha login: " + responseEntity.getStatusCode());📝 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("ABDM response for verify abha login: " + responseEntity); | |
| logger.info("ABDM response status for verify abha login: " + responseEntity.getStatusCode()); |
There was a problem hiding this comment.
@coderabbitai I don't alone want to log statusCode but want to log complete responseEntity
There was a problem hiding this comment.
@helenKaryamsetty Understood, I'll retract my suggestion. You can continue logging the complete responseEntity as needed for debugging purposes.
✏️ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#58
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:0-0
Timestamp: 2025-01-09T12:17:11.627Z
Learning: In the FHIR-API project's ABDM integration, complete responseEntity logging is preferred over just status codes for better debugging capabilities, particularly in the LoginAbhaV3ServiceImpl class when handling ABDM API responses.
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#57
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:242-251
Timestamp: 2025-01-07T12:49:16.294Z
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.
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.
|
📋 Description
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.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores