Optimize ABHA Details Fetch Using Batch Query #101
Conversation
WalkthroughThis change refactors the retrieval of beneficiary health IDs by introducing batch querying for the "new ABHA" status, updates error handling and validation in the controller, and adds a new repository method for batch status retrieval. The controller and service logic are streamlined for efficiency and clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as CreateHealthIDWithMobileOTP
participant Service as HealthIDServiceImpl
participant Repo as BenHealthIDMappingRepo
Client->>Controller: POST /getBenhealthID with benRegID
Controller->>Service: getBenHealthID(benRegID)
Service->>Repo: findByBenRegID(benRegID)
Service->>Repo: getIsNewAbhaBatch(list of healthIdNumbers)
Repo-->>Service: List of (healthIdNumber, isNewAbha)
Service-->>Controller: JSON response with health IDs and newAbha flags
Controller-->>Client: Response
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java
Fixed
Show fixed
Hide fixed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (1)
139-166: Excellent batch processing optimization with minor null handling concern.The refactored implementation successfully reduces the N+1 query problem by batching all
isNewAbhalookups into a single database call. This should significantly improve performance as stated in the PR objectives.However, consider the null handling behavior on line 159:
healthDetails.setNewAbha(Boolean.TRUE.equals(isNew));This converts
nullvalues tofalse, which may differ from the original implementation's behavior. Verify that this is the intended behavior.Alternative approach for explicit null handling:
-healthDetails.setNewAbha(Boolean.TRUE.equals(isNew)); +healthDetails.setNewAbha(isNew != null ? isNew : false); // or handle nulls differently if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/HealthIDServiceImpl.java(2 hunks)
🔇 Additional comments (4)
src/main/java/com/wipro/fhir/service/healthID/HealthIDServiceImpl.java (2)
31-32: LGTM: Appropriate imports for stream processing.The new imports support the batch processing implementation effectively.
163-163: Improved response format.Good change to return the actual object list instead of calling
toString()on the map, which provides cleaner JSON serialization.src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java (2)
153-159: Improved error handling with specific error codes.Excellent improvement to provide specific error code 4001 for missing
beneficiaryRegID, making debugging easier compared to the generic validation in the original implementation.
160-163: Enhanced exception logging.Good improvement to log the full stack trace and prepend "Error: " to the message, providing better debugging information.
src/main/java/com/wipro/fhir/repo/healthID/BenHealthIDMappingRepo.java
Outdated
Show resolved
Hide resolved
| @Operation(summary = "Get Beneficiary ABHA details") | ||
| @PostMapping("/getBenhealthID") | ||
| public String getBenhealthID(@RequestBody String comingRequest) { | ||
| OutputResponse response = new OutputResponse(); | ||
|
|
||
| try { | ||
| logger.info("NDHM_FHIR Request to fetch ABHA details: {}", comingRequest); | ||
|
|
||
| JSONObject obj = new JSONObject(comingRequest); | ||
| if (obj.has("beneficiaryRegID")) { | ||
| Long benRegID = obj.getLong("beneficiaryRegID"); | ||
| String res = healthIDService.getBenHealthID(benRegID); | ||
| response.setResponse(res); | ||
| } else { | ||
| response.setError(4001, "Missing 'beneficiaryRegID' in request"); | ||
| } | ||
| } catch (Exception e) { | ||
| logger.error("NDHM_FHIR Error while getting beneficiary ABHA: ", e); | ||
| response.setError(5000, "Error: " + e.getMessage()); | ||
| } | ||
|
|
||
| logger.info("NDHM_FHIR Response:", response); | ||
| return response.toString(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good validation improvements but missing annotations and incomplete logging.
The refactored method provides better validation and error handling, specifically checking for the beneficiaryRegID key rather than just object length.
However, there are two issues to address:
-
Missing @crossorigin annotation: The original method had this annotation which may be needed for CORS handling.
-
Incomplete logging: Line 165 doesn't log the actual response content.
Apply these fixes:
+@CrossOrigin()
@Operation(summary = "Get Beneficiary ABHA details")
@PostMapping("/getBenhealthID")
public String getBenhealthID(@RequestBody String comingRequest) {
// ... existing code ...
- logger.info("NDHM_FHIR Response:", response);
+ logger.info("NDHM_FHIR Response: {}", response.toString());
return response.toString();
}📝 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.
| @Operation(summary = "Get Beneficiary ABHA details") | |
| @PostMapping("/getBenhealthID") | |
| public String getBenhealthID(@RequestBody String comingRequest) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| logger.info("NDHM_FHIR Request to fetch ABHA details: {}", comingRequest); | |
| JSONObject obj = new JSONObject(comingRequest); | |
| if (obj.has("beneficiaryRegID")) { | |
| Long benRegID = obj.getLong("beneficiaryRegID"); | |
| String res = healthIDService.getBenHealthID(benRegID); | |
| response.setResponse(res); | |
| } else { | |
| response.setError(4001, "Missing 'beneficiaryRegID' in request"); | |
| } | |
| } catch (Exception e) { | |
| logger.error("NDHM_FHIR Error while getting beneficiary ABHA: ", e); | |
| response.setError(5000, "Error: " + e.getMessage()); | |
| } | |
| logger.info("NDHM_FHIR Response:", response); | |
| return response.toString(); | |
| } | |
| @CrossOrigin() | |
| @Operation(summary = "Get Beneficiary ABHA details") | |
| @PostMapping("/getBenhealthID") | |
| public String getBenhealthID(@RequestBody String comingRequest) { | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| logger.info("NDHM_FHIR Request to fetch ABHA details: {}", comingRequest); | |
| JSONObject obj = new JSONObject(comingRequest); | |
| if (obj.has("beneficiaryRegID")) { | |
| Long benRegID = obj.getLong("beneficiaryRegID"); | |
| String res = healthIDService.getBenHealthID(benRegID); | |
| response.setResponse(res); | |
| } else { | |
| response.setError(4001, "Missing 'beneficiaryRegID' in request"); | |
| } | |
| } catch (Exception e) { | |
| logger.error("NDHM_FHIR Error while getting beneficiary ABHA: ", e); | |
| response.setError(5000, "Error: " + e.getMessage()); | |
| } | |
| logger.info("NDHM_FHIR Response: {}", response.toString()); | |
| return response.toString(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java
around lines 144 to 167, add the @CrossOrigin annotation above the
getBenhealthID method to enable CORS handling as in the original code. Also,
update the logger.info call on line 165 to include the actual response content
by passing response.toString() instead of just response, ensuring the response
details are properly logged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java (1)
143-165:⚠️ Potential issueResponse never logged due to missing
{}placeholder
logger.info("NDHM_FHIR Response:", response);passes theresponseas an extra argument but the format string contains no{}token, so SLF4J will discard it. Use parameterised logging or explicit concatenation:-logger.info("NDHM_FHIR Response:", response); +logger.info("NDHM_FHIR Response: {}", response);
🧹 Nitpick comments (4)
src/main/resources/application.properties (2)
49-50: Clinical atom-feed parser now always on
start-parsing-atom-feed-clinical-scheduler=trueenables the job in all environments. If this was previously disabled in prod/stage we may unintentionally ingest duplicate data. Consider guarding with a profile flag.
64-68: Log file path settings conflict
logging.pathandlogging.file.nameare mutually exclusive; Spring Boot ignoreslogging.pathwhenlogging.file.nameis present. If the intent is to keep the directory configurable, droplogging.path.src/main/java/com/wipro/fhir/controller/healthID/CreateHealthIDWithMobileOTP.java (2)
150-157: Consider early validation for empty payload
obj.has("beneficiaryRegID")guards the happy path, but an empty body triggers a JSON parsing exception that is mapped to a generic 5000 error. A quick null/blank check before parsing would return the clearer 4001 code and avoid stack-trace noise.
143-165: Opportunity to exploit new batch APIThe service now supports batch retrieval (
getIsNewAbhaBatch), yet the controller still handles only a singlebeneficiaryRegID. Accepting a list (e.g.,beneficiaryRegIDs: [1,2,3]) would surface the performance win all the way to the REST layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/resources/application.properties(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/wipro/fhir/repo/healthID/BenHealthIDMappingRepo.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/resources/application.properties (1)
34-35: 1-minute cron may overload downstream jobsChanging the FHIR bundle scheduler from every 5 min to every 1 min increases DB, network and CPU utilisation 5×. Confirm that the job is idempotent and that the infrastructure sizing and rate limits can cope.
| spring.session.store-type=redis | ||
| spring.redis.host=localhost | ||
| spring.redis.host=172.17.0.1 | ||
| spring.redis.password= |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coding Redis host reduces deploy portability
spring.redis.host=172.17.0.1 bakes an environment-specific IP into the JAR. Externalise via an environment variable or profile-specific properties so CI/CD pipelines and k8s pods don’t require code changes when the endpoint moves.
🤖 Prompt for AI Agents
In src/main/resources/application.properties at line 23, the Redis host is
hard-coded as 172.17.0.1, which reduces deployment flexibility. Replace this
static IP with a placeholder that references an environment variable or a
profile-specific property, such as using Spring's ${} syntax to externalize the
host configuration. This allows the Redis host to be set dynamically via
environment variables or different profiles without modifying the code or JAR.
|
📋 Description
JIRA ID:
AMM-1446
getIsNewAbhaBatch()instead of individual calls togetIsNewAbha().isNewAbhaflags in a single query.✅ 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