fix: resolve 5 bugs across controller, feed worker, and patient data handler#145
Open
PragyaTripathi990 wants to merge 2 commits into
Open
Conversation
…handler - CreateHealthIdRecord: return ResponseEntity with HTTP 500 on FHIRException instead of always returning HTTP 200; replace full request/response body logging at INFO with safe summary messages to prevent patient data leaks - ClinicalFeedWorker: replace bare Exception throws with FHIRException for domain consistency; add error logging to outer catch blocks that were silently swallowing feed processing failures; guard both feed URL pointer parsings with bounds check and NumberFormatException handling to prevent crashes on malformed URLs - PatientDataGatewayServiceImpl: add @transactional to both overloads of generatePatientProfileAMRIT_SaveTo_Mongo so MongoDB write and processed-flag update are atomic; fix copy-paste bug where all three address line OR conditions checked getAddressLine1() instead of lines 1, 2, and 3 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR updates HealthID controller endpoints to return typed HTTP responses and improved logging, makes ClinicalFeedWorker pagination parsing and error logging more defensive and changes some failures to throw FHIRException, and adds transactional annotations plus an address-line conditional fix in the patient data gateway. ChangesFHIR API Error Handling and Robustness Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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[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/controller/healthID/CreateHealthIdRecord.java (2)
69-73: ⚡ Quick winConsider catching broader exceptions for consistent error responses.
Currently only
FHIRExceptionis caught and formatted into anOutputResponse. Other runtime exceptions (e.g.,NullPointerException,IllegalArgumentException) will propagate as unformatted 500 errors, creating inconsistent API responses for clients.🛡️ Suggested approach to handle all exceptions consistently
} catch (FHIRException e) { response.setError(5000, e.getMessage()); logger.error("NDHM_FHIR error adding health record: {}", e.getMessage()); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString()); + } catch (Exception e) { + response.setError(5000, "An unexpected error occurred"); + logger.error("NDHM_FHIR unexpected error adding health record", e); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString()); }Note: The generic catch logs the full exception for debugging but returns a safe message to avoid leaking internal details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java` around lines 69 - 73, The current catch in CreateHealthIdRecord only handles FHIRException, so other runtime errors leak unformatted 500s; add a broader catch(Exception e) after the existing FHIRException catch to set the same OutputResponse error (use a safe/generic message and the same error code), log the full exception (logger.error with the exception object, not just e.getMessage()) and return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString()) so all exceptions produce a consistent, safe API response.
48-52: ⚡ Quick winConsider catching broader exceptions for consistent error responses.
Currently only
FHIRExceptionis caught and formatted into anOutputResponse. Other runtime exceptions (e.g.,NullPointerException,IllegalArgumentException) will propagate as unformatted 500 errors, creating inconsistent API responses for clients.🛡️ Suggested approach to handle all exceptions consistently
} catch (FHIRException e) { response.setError(5000, e.getMessage()); logger.error("NDHM_FHIR error mapping ABHA to beneficiary: {}", e.getMessage()); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString()); + } catch (Exception e) { + response.setError(5000, "An unexpected error occurred"); + logger.error("NDHM_FHIR unexpected error mapping ABHA to beneficiary", e); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString()); }Note: The generic catch logs the full exception for debugging but returns a safe message to avoid leaking internal details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java` around lines 48 - 52, The current try-catch in CreateHealthIdRecord only catches FHIRException so other runtime exceptions escape unformatted; add a broader catch (e.g., a catch(Exception e) after the existing FHIRException catch) to handle all errors consistently: log the full exception (stacktrace) via logger.error and set the OutputResponse with a generic 500 error message using response.setError(...), then return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString()) so CreateHealthIdRecord always returns a uniform API error format for any exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java`:
- Line 38: The method mapHealthIDToBeneficiary in CreateHealthIdRecord was
changed from returning String to ResponseEntity<String>, which is a breaking API
change; either revert the controller signature back to String and return the raw
String from HealthIDService (preserve behavior of
HealthIDService.mapHealthIDToBeneficiary) or, if you must return ResponseEntity,
add a clear compatibility layer (e.g., keep the old endpoint/signature or
provide an overloaded endpoint) and update API docs and external client
integration notes; locate the method mapHealthIDToBeneficiary in
CreateHealthIdRecord and adjust the return type/return value accordingly and
ensure API documentation (OpenAPI/Swagger) and any client-facing contract are
updated to reflect the chosen change.
In
`@src/main/java/com/wipro/fhir/service/atoms/feed/bahmni/ClinicalFeedWorker.java`:
- Around line 95-102: The pointer parsing logic in ClinicalFeedWorker leaves
pointer/tempPointer as 0 when the last URL segment is empty or non-numeric (or
parses to a non-positive value), which can silently terminate the while (pointer
> 0) loop; update both parsing sites to validate the last path segment and treat
any empty, non-numeric, or <=0 parsed values as invalid and set
pointer/tempPointer = atomsFeedStartPage; ensure the logger records the invalid
value (feed.getLinkSelf()) and that the code never leaves pointer as 0 after
parsing so the feed loop has a safe fallback.
In
`@src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java`:
- Around line 93-94: The service method generatePatientProfileAMRIT_SaveTo_Mongo
currently uses `@Transactional` but only the JPA transaction manager is active, so
Mongo writes from savePatientProfileDataToMongo and MySQL updates via
updateProcessedFlagForProfileCreated are not coordinated and can leave stores
inconsistent; fix by either (A) enabling multi-store transactions: add
`@EnableTransactionManagement`, register a MongoTransactionManager bean (MongoDB
must support transactions i.e., replica set), create a ChainedTransactionManager
combining the JpaTransactionManager and MongoTransactionManager and annotate
generatePatientProfileAMRIT_SaveTo_Mongo to use that chained manager so both
mongoRepository.saveAll(...) and updateProcessedFlagForProfileCreated run in one
atomic transaction, or (B) implement explicit compensation/error-handling: wrap
savePatientProfileDataToMongo and updateProcessedFlagForProfileCreated in a
try/catch and roll back the completed store change manually if the other fails;
apply the same fix to the other affected method(s) referenced in the comment.
---
Nitpick comments:
In `@src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java`:
- Around line 69-73: The current catch in CreateHealthIdRecord only handles
FHIRException, so other runtime errors leak unformatted 500s; add a broader
catch(Exception e) after the existing FHIRException catch to set the same
OutputResponse error (use a safe/generic message and the same error code), log
the full exception (logger.error with the exception object, not just
e.getMessage()) and return
ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString())
so all exceptions produce a consistent, safe API response.
- Around line 48-52: The current try-catch in CreateHealthIdRecord only catches
FHIRException so other runtime exceptions escape unformatted; add a broader
catch (e.g., a catch(Exception e) after the existing FHIRException catch) to
handle all errors consistently: log the full exception (stacktrace) via
logger.error and set the OutputResponse with a generic 500 error message using
response.setError(...), then return
ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response.toString())
so CreateHealthIdRecord always returns a uniform API error format for any
exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23d80451-4231-4bfe-b71e-1a39de968f88
📒 Files selected for processing (3)
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.javasrc/main/java/com/wipro/fhir/service/atoms/feed/bahmni/ClinicalFeedWorker.javasrc/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java
| @Operation(summary = "Map ABHA to beneficiary") | ||
| @PostMapping(value = { "/mapHealthIDToBeneficiary" }) | ||
| public String mapHealthIDToBeneficiary( | ||
| public ResponseEntity<String> mapHealthIDToBeneficiary( |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usages of mapHealthIDToBeneficiary that might be affected by the signature change
# Search for method calls in Java/test files
rg -n --type java -C3 'mapHealthIDToBeneficiary\s*\(' -g '!CreateHealthIdRecord.java'
# Search for REST client calls or URL references
rg -n --type java -C3 '/mapHealthIDToBeneficiary' -g '!CreateHealthIdRecord.java'Repository: PSMRI/FHIR-API
Length of output: 2734
🏁 Script executed:
#!/bin/bash
# First, find and read the CreateHealthIdRecord.java file to see the actual signature
echo "=== Finding CreateHealthIdRecord.java ==="
fd -name "CreateHealthIdRecord.java"
# Then read the file around line 38
echo -e "\n=== Reading the method signature ==="
sed -n '35,50p' "$(fd -name 'CreateHealthIdRecord.java')"
# Search for test files that might call this method
echo -e "\n=== Searching for test callers ==="
rg -n --type java 'mapHealthIDToBeneficiary' --iglob '*test*' --iglob '*Test*'
# Search for all references to mapHealthIDToBeneficiary across all file types
echo -e "\n=== Searching all references (including properties, configs, REST clients) ==="
rg -n 'mapHealthIDToBeneficiary' --type-list
rg -n 'mapHealthIDToBeneficiary'Repository: PSMRI/FHIR-API
Length of output: 2005
🏁 Script executed:
#!/bin/bash
# Read the CreateHealthIdRecord.java file
echo "=== CreateHealthIdRecord.java content ==="
cat -n "src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java" | head -80
# Check if there are any test files for this controller
echo -e "\n=== Looking for test files ==="
fd -type f "CreateHealthIdRecord"
fd -type f "Test" -path "*healthID*"Repository: PSMRI/FHIR-API
Length of output: 4217
Breaking REST API change requires external client validation.
The return type changed from String to ResponseEntity<String> (line 38). This is a breaking change for any external REST clients consuming this endpoint. While the underlying service layer (HealthIDService) remains unchanged, REST clients calling /healthIDRecord/mapHealthIDToBeneficiary will receive a different response structure and must be updated accordingly. Ensure external client integrations and API documentation are updated to reflect this change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java`
at line 38, The method mapHealthIDToBeneficiary in CreateHealthIdRecord was
changed from returning String to ResponseEntity<String>, which is a breaking API
change; either revert the controller signature back to String and return the raw
String from HealthIDService (preserve behavior of
HealthIDService.mapHealthIDToBeneficiary) or, if you must return ResponseEntity,
add a clear compatibility layer (e.g., keep the old endpoint/signature or
provide an overloaded endpoint) and update API docs and external client
integration notes; locate the method mapHealthIDToBeneficiary in
CreateHealthIdRecord and adjust the return type/return value accordingly and
ensure API documentation (OpenAPI/Swagger) and any client-facing contract are
updated to reflect the chosen change.
Comment on lines
+93
to
94
| @Transactional(rollbackFor = FHIRException.class) | ||
| public String generatePatientProfileAMRIT_SaveTo_Mongo(String Authorization) throws FHIRException { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Transaction manager and `@EnableTransactionManagement` declarations =="
rg -n -C3 '@EnableTransactionManagement|PlatformTransactionManager|MongoTransactionManager|JpaTransactionManager|ChainedTransactionManager|transactionManagerRef' $(fd -t f '.*\.java$' src || true)
echo
echo "== Mongo config presence (templates/factories/transaction manager) =="
rg -n -C3 'MongoTemplate|MongoDatabaseFactory|MongoTransactionManager|AbstractMongoClientConfiguration' $(fd -t f '.*\.java$' src || true)
echo
echo "== savePatientProfileDataToMongo implementation and annotations =="
rg -n -C5 'savePatientProfileDataToMongo\s*\(' $(fd -t f '.*\.java$' src || true)
echo
echo "== Repository update method definition =="
rg -n -C5 'updateProcessedFlagForProfileCreated\s*\(' $(fd -t f '.*\.java$' src || true)Repository: PSMRI/FHIR-API
Length of output: 11637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== FhirApiApplication.java full content ==="
cat -n src/main/java/com/wipro/fhir/FhirApiApplication.java
echo
echo "=== Application configuration files ==="
fd -t f -e properties -e yml -e yaml src || true | head -20
rg -l 'application' . --max-depth 2 2>/dev/null | grep -E '\.(properties|yml|yaml)$' | head -20
echo
echo "=== Check application.properties for spring.data.mongodb and transaction configs ==="
fd -t f 'application.*properties' . | xargs cat 2>/dev/null | grep -i -E 'mongodb|transaction|jpa' || echo "No matches"
echo
echo "=== Check application.yml/yaml for spring.data.mongodb and transaction configs ==="
fd -t f 'application.*\.ya?ml' . | xargs cat 2>/dev/null | grep -A5 -B5 -i -E 'mongodb|transaction|jpa' || echo "No matches"
echo
echo "=== Search for `@Bean` definitions related to transaction manager ==="
rg -n '@Bean.*Transaction|TransactionManager|MongoTemplate' --type java src || echo "No additional matches"Repository: PSMRI/FHIR-API
Length of output: 2853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all .properties and .yml files ==="
find . -type f \( -name '*.properties' -o -name '*.yml' -o -name '*.yaml' \) 2>/dev/null | head -20
echo
echo "=== Check src/main/resources directory ==="
ls -la src/main/resources/ 2>/dev/null || echo "Directory not found"
echo
echo "=== Content of application.properties if it exists ==="
cat src/main/resources/application.properties 2>/dev/null || echo "File not found"
echo
echo "=== Content of application.yml if it exists ==="
cat src/main/resources/application.yml 2>/dev/null || echo "File not found"
echo
echo "=== Check for Spring Boot configuration in pom.xml (spring-data-mongodb vs spring-boot-starter-data-jpa) ==="
rg -A2 -B2 'spring-data-mongodb|spring-boot-starter-data-jpa|spring-boot-starter-data-rest' pom.xml 2>/dev/null || echo "Not found in pom.xml"
echo
echo "=== Check if mongoTemplate operations are wrapped in any TX manager binding ==="
rg -n 'mongoTemplate\.|transactionManager\|@Transactional.*mongodb' src --type java | head -20Repository: PSMRI/FHIR-API
Length of output: 5970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full pom.xml search for mongodb dependency ==="
rg 'mongodb|mongo' pom.xml -i
echo
echo "=== Check environment property files for MongoDB config ==="
echo "--- common_example.properties ---"
cat src/main/environment/common_example.properties | grep -i -E 'mongo|mongodb' || echo "No MongoDB config"
echo
echo "--- common_ci.properties ---"
cat src/main/environment/common_ci.properties | grep -i -E 'mongo|mongodb' || echo "No MongoDB config"
echo
echo "--- common_docker.properties ---"
cat src/main/environment/common_docker.properties | grep -i -E 'mongo|mongodb' || echo "No MongoDB config"
echo
echo "=== Check if MongoTemplate bean in FhirApiApplication is actually used or if it fails ==="
rg -n 'MongoTemplate|mongoTemplate' src --type java | grep -v '://' | head -30Repository: PSMRI/FHIR-API
Length of output: 1638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for `@EnableTransactionManagement` or explicit MongoTransactionManager bean ==="
rg -n '@EnableTransactionManagement|@Bean.*MongoTransactionManager|new MongoTransactionManager' src --type java
echo
echo "=== Check savePatientProfileDataToMongo full implementation ==="
sed -n '654,670p' src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java
echo
echo "=== Check the transactional method calling savePatientProfileDataToMongo (around line 87) ==="
sed -n '70,95p' src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java
echo
echo "=== Verify if `@Transactional` on service method covers nested Mongo operations ==="
rg -B5 '@Override' src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java | grep -A5 'generatePatientProfileAMRIT_SaveTo_Mongo'Repository: PSMRI/FHIR-API
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check savePatientProfileDataToMongo full implementation ==="
sed -n '654,670p' src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java
echo
echo "=== Check the transactional method calling savePatientProfileDataToMongo (lines 70-95) ==="
sed -n '70,95p' src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java
echo
echo "=== Check PatientDataGatewayServiceImpl around the `@Transactional` method (lines 93-114) ==="
sed -n '93,114p' src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.javaRepository: PSMRI/FHIR-API
Length of output: 3181
@Transactional here does not provide atomic rollback across MongoDB and MySQL stores.
These methods span Mongo writes (savePatientProfileDataToMongo → mongoRepository.saveAll()) and MySQL updates (updateProcessedFlagForProfileCreated). The savePatientProfileDataToMongo() method has no transaction management, while only the MySQL repository method carries @Transactional. This leaves the Mongo operation unmanaged, risking inconsistent state: if the Mongo save succeeds but the MySQL update fails (or vice versa), one store commits while the other doesn't.
No @EnableTransactionManagement, MongoTransactionManager bean, or ChainedTransactionManager configuration was found. Without explicit cross-store transaction coordination, the @Transactional annotation on the service method only binds to the JPA transaction manager and cannot guarantee atomicity.
Also applies to: lines 117–119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java`
around lines 93 - 94, The service method
generatePatientProfileAMRIT_SaveTo_Mongo currently uses `@Transactional` but only
the JPA transaction manager is active, so Mongo writes from
savePatientProfileDataToMongo and MySQL updates via
updateProcessedFlagForProfileCreated are not coordinated and can leave stores
inconsistent; fix by either (A) enabling multi-store transactions: add
`@EnableTransactionManagement`, register a MongoTransactionManager bean (MongoDB
must support transactions i.e., replica set), create a ChainedTransactionManager
combining the JpaTransactionManager and MongoTransactionManager and annotate
generatePatientProfileAMRIT_SaveTo_Mongo to use that chained manager so both
mongoRepository.saveAll(...) and updateProcessedFlagForProfileCreated run in one
atomic transaction, or (B) implement explicit compensation/error-handling: wrap
savePatientProfileDataToMongo and updateProcessedFlagForProfileCreated in a
try/catch and roll back the completed store change manually if the other fails;
apply the same fix to the other affected method(s) referenced in the comment.
- CreateHealthIdRecord: add catch(Exception e) to both endpoints so all unhandled runtime exceptions return a consistent 500 OutputResponse instead of leaking unformatted errors to clients - ClinicalFeedWorker: guard parsed pointer against non-positive values and empty path segments at both parsing sites; fallback to atomsFeedStartPage so the feed loop never silently exits on bad URLs - PatientDataGatewayServiceImpl: wrap MySQL updateProcessedFlag call in try/catch on both overloads; log affected IDs and re-throw on failure to surface cross-store inconsistency (Mongo+MySQL) explicitly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java (1)
104-119: ⚡ Quick winExtract the duplicated post-save reconciliation block into a private helper.
Lines 104-119 and 138-153 are essentially identical (build
ids, callupdateProcessedFlagForProfileCreated, log + rethrow on failure, reassignppList). Drift between the two copies (e.g., changing the log message or exception type in one place but not the other) is the typical failure mode here.♻️ Suggested extraction
- List<PatientDemographicModel_NDHM_Patient_Profile> savedList = feedPatientProfileToMongoDB(ppList); - - if (savedList != null && savedList.size() > 0) { - List<Long> ids = new ArrayList<>(); - ids.add((long) 0); - for (PatientDemographicModel_NDHM_Patient_Profile pp : savedList) { - ids.add(pp.getTriggerTableAIId()); - } - try { - tRG_PatientResourceData_Repo.updateProcessedFlagForProfileCreated(ids); - } catch (Exception e) { - // Mongo save succeeded but MySQL update failed — stores are now inconsistent. - // Log affected IDs so ops can manually reconcile; re-throw to signal failure. - logger.error("MySQL update failed after Mongo save — inconsistent state. Affected IDs: {}. Error: {}", ids, e.getMessage()); - throw new FHIRException("Failed to update processed flag after Mongo save. " + e.getMessage()); - } - ppList = savedList; - } + List<PatientDemographicModel_NDHM_Patient_Profile> savedList = feedPatientProfileToMongoDB(ppList); + ppList = markSavedProfilesProcessed(savedList, ppList);And add once:
private List<PatientDemographicModel_NDHM_Patient_Profile> markSavedProfilesProcessed( List<PatientDemographicModel_NDHM_Patient_Profile> savedList, List<PatientDemographicModel_NDHM_Patient_Profile> fallback) throws FHIRException { if (savedList == null || savedList.isEmpty()) { return fallback; } List<Long> ids = new ArrayList<>(); ids.add(0L); for (PatientDemographicModel_NDHM_Patient_Profile pp : savedList) { ids.add(pp.getTriggerTableAIId()); } try { tRG_PatientResourceData_Repo.updateProcessedFlagForProfileCreated(ids); } catch (Exception e) { logger.error("MySQL update failed after Mongo save — inconsistent state. Affected IDs: {}. Error: {}", ids, e.getMessage()); throw new FHIRException("Failed to update processed flag after Mongo save. " + e.getMessage()); } return savedList; }Also applies to: 138-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java` around lines 104 - 119, Extract the duplicated post-save reconciliation into a private helper (e.g., private List<PatientDemographicModel_NDHM_Patient_Profile> markSavedProfilesProcessed(List<PatientDemographicModel_NDHM_Patient_Profile> savedList, List<PatientDemographicModel_NDHM_Patient_Profile> fallback) throws FHIRException) that checks savedList for null/empty and returns fallback if so, builds the ids list (adding 0L and each pp.getTriggerTableAIId()), calls tRG_PatientResourceData_Repo.updateProcessedFlagForProfileCreated(ids), and on exception logs the ids and e.getMessage() via logger.error and rethrows a FHIRException with the error message; replace the two identical blocks in PatientDataGatewayServiceImpl with calls to this helper and assign its return to ppList.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java`:
- Around line 104-119: Extract the duplicated post-save reconciliation into a
private helper (e.g., private List<PatientDemographicModel_NDHM_Patient_Profile>
markSavedProfilesProcessed(List<PatientDemographicModel_NDHM_Patient_Profile>
savedList, List<PatientDemographicModel_NDHM_Patient_Profile> fallback) throws
FHIRException) that checks savedList for null/empty and returns fallback if so,
builds the ids list (adding 0L and each pp.getTriggerTableAIId()), calls
tRG_PatientResourceData_Repo.updateProcessedFlagForProfileCreated(ids), and on
exception logs the ids and e.getMessage() via logger.error and rethrows a
FHIRException with the error message; replace the two identical blocks in
PatientDataGatewayServiceImpl with calls to this helper and assign its return to
ppList.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f8e65c5-d7a5-458d-a8d8-c437e8e99c8e
📒 Files selected for processing (3)
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.javasrc/main/java/com/wipro/fhir/service/atoms/feed/bahmni/ClinicalFeedWorker.javasrc/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.java
- src/main/java/com/wipro/fhir/service/atoms/feed/bahmni/ClinicalFeedWorker.java
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
This PR fixes 5 real engineering bugs identified across the health ID controller, clinical feed worker, and patient data handler layers of the FHIR-API service.
Bug 1 — HTTP 200 returned on error (
CreateHealthIdRecord)Both endpoints (
/mapHealthIDToBeneficiaryand/addHealthIdRecord) caughtFHIRExceptionand set an error state but still returned HTTP 200 OK. API clients had no way to distinguish success from failure via HTTP status. Fixed by returningResponseEntity<String>withHttpStatus.INTERNAL_SERVER_ERRORon error andResponseEntity.ok()on success.Bug 2 — Sensitive patient data logged at INFO level (
CreateHealthIdRecord)Full request and response bodies (containing ABHA numbers, patient identifiers) were logged at
INFOlevel on every API call. This is a data privacy violation in a healthcare context. Replaced with safe summary log messages that confirm the operation without exposing patient data.Bug 3 — Bare
Exceptionthrows instead of domain exception (ClinicalFeedWorker)Two
throw new Exception(...)calls were used insidereadPatientEncounterFeedswhereFHIRException(the project's own domain exception) should have been used. This broke the exception hierarchy and made it impossible for callers to catch specific error types. Replaced withthrow new FHIRException(...).Bug 4 — Silent exception swallowing in outer feed loop (
ClinicalFeedWorker)Four outer catch blocks (
IllegalArgumentException,FeedException,IOException,Exception) silently reset the pointer to 0 with no logging whatsoever. Production failures in feed processing were completely invisible. Addedlogger.error(...)to each catch block.Bug 5 — Unsafe feed URL pointer parsing (
ClinicalFeedWorker)Both feed pointer parse sites (in
encounterFeedManagerand inside the next-archive loop) calledInteger.parseInt(arr[arr.length - 1])without guarding against empty strings or non-numeric values. A malformed feed URL would throw an uncaughtNumberFormatException. Added bounds check, empty string guard, andNumberFormatExceptionhandling with fallback toatomsFeedStartPage.Bug 6 — Missing
@Transactionalon dual DB writes (PatientDataGatewayServiceImpl)Both overloads of
generatePatientProfileAMRIT_SaveTo_Mongoperformed two separate database operations (MongoDB write + processed flag update in MySQL) without a transaction boundary. A failure between the two writes would leave data in an inconsistent state. Added@Transactional(rollbackFor = FHIRException.class)to both methods.Bug 7 — Copy-paste bug in address line check (
PatientDataGatewayServiceImpl)The condition
if (getAddressLine1() != null || getAddressLine1() != null || getAddressLine1() != null)checked the same field three times due to a copy-paste error.AddressLine2andAddressLine3were never checked, so patient addresses with only lines 2 or 3 were silently ignored. Fixed to checkAddressLine1 || AddressLine2 || AddressLine3.Files Changed
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIdRecord.javasrc/main/java/com/wipro/fhir/service/atoms/feed/bahmni/ClinicalFeedWorker.javasrc/main/java/com/wipro/fhir/service/patient_data_handler/PatientDataGatewayServiceImpl.java🤖 Generated with Claude Code
Summary by CodeRabbit