Fix: Added null safety check in NHM dashboard scheduler (#121)#367
Fix: Added null safety check in NHM dashboard scheduler (#121)#367srilasya1815 wants to merge 1 commit intoPSMRI:mainfrom
Conversation
…artup NullPointerException(PSMRI#121)
📝 WalkthroughWalkthroughThe pull request includes formatting adjustments to pom.xml and improves error handling and logging in the ScheduleJobForNHMDashboardData class by adding a null-check for nhmDashboardService and enhancing exception logging and reporting. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/config/quartz/ScheduleJobForNHMDashboardData.java (1)
50-60: Please add regression tests for the new control flow.Add tests for (1) null
nhmDashboardServiceskip path and (2) exception path frompull_NHM_Data_CTI()to lock in behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/config/quartz/ScheduleJobForNHMDashboardData.java` around lines 50 - 60, Add unit tests for ScheduleJobForNHMDashboardData to cover both new control paths: create a test that sets the nhmDashboardService field to null (or inject a null mock) and asserts that execute() returns/finishes without calling any service methods and that logger.error("NHM_DashboardService bean is not initialized. Skipping execution.") is invoked (use a logging/appenders test helper or verify no interactions on a mocked service); and create a test that injects a mock NHM_DashboardService where pull_NHM_Data_CTI() throws an exception and assert execute() catches it and calls logger.error("Error while pulling NHM dashboard data", e) (or verify exception handling behavior and that no exception escapes). Target the ScheduleJobForNHMDashboardData class and the nhmDashboardService field and pull_NHM_Data_CTI() method when wiring mocks and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/iemr/common/config/quartz/ScheduleJobForNHMDashboardData.java`:
- Around line 55-62: The catch block in ScheduleJobForNHMDashboardData currently
swallows exceptions from nhmDashboardService.pull_NHM_Data_CTI() and always logs
completion; change it to rethrow a Quartz JobExecutionException so Quartz sees
failures: move the "Completed job..." logger into the try block (after
nhmDashboardService.pull_NHM_Data_CTI() and logger.info(s)), and in the
catch(Exception e) throw new JobExecutionException("Error pulling NHM dashboard
data for " + arg0.getClass().getName(), e, false) (or true if you want retry) so
the failure is propagated to Quartz.
---
Nitpick comments:
In
`@src/main/java/com/iemr/common/config/quartz/ScheduleJobForNHMDashboardData.java`:
- Around line 50-60: Add unit tests for ScheduleJobForNHMDashboardData to cover
both new control paths: create a test that sets the nhmDashboardService field to
null (or inject a null mock) and asserts that execute() returns/finishes without
calling any service methods and that logger.error("NHM_DashboardService bean is
not initialized. Skipping execution.") is invoked (use a logging/appenders test
helper or verify no interactions on a mocked service); and create a test that
injects a mock NHM_DashboardService where pull_NHM_Data_CTI() throws an
exception and assert execute() catches it and calls logger.error("Error while
pulling NHM dashboard data", e) (or verify exception handling behavior and that
no exception escapes). Target the ScheduleJobForNHMDashboardData class and the
nhmDashboardService field and pull_NHM_Data_CTI() method when wiring mocks and
assertions.
| try { | ||
| String s = nhmDashboardService.pull_NHM_Data_CTI(); | ||
| logger.info(s); | ||
| } catch (Exception e) { | ||
| logger.error("Error while pulling NHM dashboard data", e); | ||
| } | ||
|
|
||
| logger.info("Completed job for NHM dashboard data pull from cti " + arg0.getClass().getName()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Quartz Scheduler Job execute method exception handling if exception caught and not thrown
💡 Result:
If your Quartz Job.execute(...) catches an exception and does not rethrow it, Quartz treats that run as successful: the trigger is considered “completed normally,” the scheduler won’t refire/unschedule based on that failure, and the job will simply run again at its next scheduled fire time. (Quartz only reacts when something escapes execute, especially a JobExecutionException.) [3], [4]
If you want Quartz to know the run failed and optionally take action, rethrow a JobExecutionException:
- Keep normal schedule (no immediate refire, no unschedule): throw
new JobExecutionException(cause)with no flags (it records the failure; next fire proceeds normally). [2], [5] - Refire immediately:
new JobExecutionException(cause, refireImmediately: true)(Quartz.NET example shown; same concept in Java). [1], [2] - Unschedule triggers / stop future runs: set
setUnscheduleAllTriggers(true)(Java example). [6], [2]
So: catch-and-swallow = Quartz won’t do anything special; catch-and-throw JobExecutionException = Quartz can apply retry/unschedule directives. [2], [6]
🏁 Script executed:
find . -name "ScheduleJobForNHMDashboardData.java" -type fRepository: PSMRI/Common-API
Length of output: 141
🏁 Script executed:
cat -n ./src/main/java/com/iemr/common/config/quartz/ScheduleJobForNHMDashboardData.javaRepository: PSMRI/Common-API
Length of output: 2757
Propagate job failures to Quartz instead of suppressing them.
Exceptions caught at lines 58–60 are logged but not re-thrown. Quartz treats this as a successful job execution (exceptions only matter to Quartz when JobExecutionException escapes execute()). Additionally, line 62 unconditionally logs completion even after a failure, obscuring actual failures in logs and monitoring.
Move the completion log into the try block and throw JobExecutionException on exception:
Proposed fix
public void execute(JobExecutionContext arg0) throws JobExecutionException {
logger.info("Started job for NHM dashboard data pull from cti " + arg0.getClass().getName());
if (nhmDashboardService == null) {
logger.error("NHM_DashboardService bean is not initialized. Skipping execution.");
return;
}
try {
String s = nhmDashboardService.pull_NHM_Data_CTI();
logger.info(s);
+ logger.info("Completed job for NHM dashboard data pull from cti " + arg0.getClass().getName());
} catch (Exception e) {
logger.error("Error while pulling NHM dashboard data", e);
- }
-
- logger.info("Completed job for NHM dashboard data pull from cti " + arg0.getClass().getName());
+ throw new JobExecutionException("Error while pulling NHM dashboard data", e, false);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/iemr/common/config/quartz/ScheduleJobForNHMDashboardData.java`
around lines 55 - 62, The catch block in ScheduleJobForNHMDashboardData
currently swallows exceptions from nhmDashboardService.pull_NHM_Data_CTI() and
always logs completion; change it to rethrow a Quartz JobExecutionException so
Quartz sees failures: move the "Completed job..." logger into the try block
(after nhmDashboardService.pull_NHM_Data_CTI() and logger.info(s)), and in the
catch(Exception e) throw new JobExecutionException("Error pulling NHM dashboard
data for " + arg0.getClass().getName(), e, false) (or true if you want retry) so
the failure is propagated to Quartz.
1 participant
Description
Added defensive null check for NHM_DashboardService in
ScheduleJobForNHMDashboardData to prevent potential
NullPointerException during scheduler execution after application startup.
Changes
This improves startup stability.
Closes #121
Summary by CodeRabbit