feat(health,version): add health and version endpoints#136
Merged
drtechie merged 20 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints#136drtechie merged 20 commits intoPSMRI:mainfrom
drtechie merged 20 commits intoPSMRI:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a /health REST endpoint and HealthService performing concurrent checks for MySQL, Redis, and optional Elasticsearch; refactors /version to return JSON; updates Maven git-commit-id plugin and adds Elasticsearch REST client dependency; JWT filter now skips validation for public /health and /version endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as HealthController
participant Auth as JwtAuthenticationUtil
participant Service as HealthService
participant MySQL as MySQL
participant Redis as Redis
participant ES as Elasticsearch
Client->>Controller: GET /health (token)
Controller->>Auth: validateToken(token)
alt token valid
Auth-->>Controller: valid
else token invalid/absent
Auth-->>Controller: invalid/absent
end
Controller->>Service: checkHealth()
par MySQL check
Service->>MySQL: SELECT 1 / health query
MySQL-->>Service: result
and Redis check
Service->>Redis: PING (with timeout)
Redis-->>Service: PONG / error
and Elasticsearch check
Service->>ES: /_cluster/health and optional probes (if enabled)
ES-->>Service: cluster status / probe result
end
Service-->>Controller: aggregated health map
alt overall UP or DEGRADED
Controller-->>Client: HTTP 200 + details
else overall DOWN
Controller-->>Client: HTTP 503 + details
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/identity/controller/health/HealthController.java (1)
94-127: Token extraction logic is duplicated fromJwtUserIdValidationFilter.
isUserAuthenticatedre-implements JWT extraction from headers and cookies, mirroring logic already present in the filter. Consider extracting a shared utility method (e.g., inJwtAuthenticationUtilor a new helper) to avoid maintaining the same extraction logic in two places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/controller/health/HealthController.java` around lines 94 - 127, The isUserAuthenticated method duplicates JWT extraction logic from JwtUserIdValidationFilter; refactor by adding a shared extractor (e.g., JwtAuthenticationUtil.extractTokenFromRequest(HttpServletRequest)) that encapsulates reading JwtToken header, Authorization Bearer header, and cookies (preserving current precedence), then update HealthController.isUserAuthenticated to call that extractor and then jwtAuthenticationUtil.validateUserIdAndJwtToken(token); also modify JwtUserIdValidationFilter to use the same new extractor so token parsing logic is maintained in one place.src/main/java/com/iemr/common/identity/service/health/HealthService.java (2)
121-154: Health checks run sequentially — consider parallel execution for lower latency.MySQL, Redis, and Elasticsearch checks are independent but invoked sequentially. If any one blocks for its full timeout (2-3 s), the total latency stacks up. Since you already have an
ExecutorService, you could run them concurrently withCompletableFuture.allOfto keep response time bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 121 - 154, The health checks in checkHealth currently run sequentially; modify checkHealth to run checkMySQLHealth, checkRedisHealth (only if redisTemplate!=null) and checkElasticsearchHealth (only if elasticsearchEnabled && elasticsearchClientReady) concurrently using CompletableFuture.supplyAsync with your existing ExecutorService and combine them with CompletableFuture.allOf, then gather each result future (handling exceptions by returning a failing component map) and populate components and overallHealth using isHealthy; ensure timestamp, STATUS_KEY, logging and includeDetails behavior remain the same and that exceptions are logged and translated into component DOWN entries so a hung call doesn't block the whole check.
362-416: JDBC URL parsers are fragile — considerjava.net.URIor a regex.The manual
indexOf/substringparsing only handles thejdbc:mysql://host:port/dbformat. URLs with credentials (user:pass@host), IPv6 addresses, or multiple hosts (failover URLs) will be mis-parsed. UsingURI.create(jdbcUrl.substring(5))(stripping thejdbc:prefix) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 362 - 416, The current manual parsing in extractHost, extractPort, and extractDatabaseName is fragile; replace it by stripping the "jdbc:" prefix and using java.net.URI to parse the remaining URI (or URI.create) so you correctly handle credentials, IPv6, and multi-host forms: in extractHost call new URI(withoutJdbc).getHost() and return UNKNOWN_VALUE if null, in extractPort call new URI(...).getPort() and return "3306" when getPort() returns -1, and in extractDatabaseName use new URI(...).getPath() (trim leading '/') and strip any query component; wrap URI creation in try/catch and log debug on failure preserving existing fallback values so behavior remains safe for malformed inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 295-317: Update the git-commit-id-maven-plugin version: in the
plugin block for io.github.git-commit-id:git-commit-id-maven-plugin (the
execution id "get-the-git-infos" and goal "revision"), change the <version> from
7.0.0 to the latest stable release (e.g. 9.0.2) so the plugin benefits from
recent fixes and improvements while keeping the existing configuration elements
like includeOnlyProperties and failOnNoGitDirectory.
- Around line 248-253: The dependency entry for
org.elasticsearch.client:elasticsearch-rest-client is pinned to 8.10.0; update
the <version> to 9.3.0 in the pom and run a full build to surface any API or
transitive dependency changes, then evaluate and migrate usage from the legacy
artifactId (elasticsearch-rest-client) to the new RestClient / Rest5Client API
where applicable (search for code referencing classes from
elasticsearch-rest-client and adapt calls to the RestClient API, updating
imports and client construction accordingly); also run a dependency tree scan
(mvn dependency:tree) and security scan to confirm there are no problematic
transitive CVEs after the upgrade.
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Line 249: The HealthService currently returns a hardcoded Elasticsearch
version string in the HealthCheckResult; update the code in HealthService where
it constructs new HealthCheckResult(...) to either (a) issue a GET "/" (or
another version endpoint) to the Elasticsearch cluster, parse the JSON response
to extract the "version.number" field, and pass that value instead of the
literal "Elasticsearch 8.10.0", or (b) remove the guessed version and pass
null/omit the version like the MySQL/Redis checks; implement the JSON parsing
and error handling where the current return new HealthCheckResult(true,
"Elasticsearch 8.10.0", null) occurs so HealthCheckResult receives the real
version string (or null) instead of the hardcoded value.
- Around line 94-119: The Elasticsearch RestClient opened in
initializeElasticsearchClient (assigned to the elasticsearchRestClient field) is
never closed; add a `@PreDestroy` lifecycle method (e.g., destroy or
closeResources) that checks if elasticsearchRestClient is non-null and open,
calls close() inside a try/catch, logs any errors, and sets
elasticsearchClientReady to false; ensure the method also mirrors the
ExecutorService cleanup pattern used elsewhere so resources are reliably
released on shutdown.
- Line 63: The static ExecutorService created with
Executors.newFixedThreadPool(4) (executorService) is never shut down and will
leak threads on WAR redeploys; change it to an instance field or add a lifecycle
cleanup method that shuts it down and closes related resources: add a
`@PreDestroy-annotated` method (e.g., cleanup) that calls
executorService.shutdownNow() (or orderly shutdown with timeout), handles
InterruptedException, and also closes elasticsearchRestClient with try/catch
logging any IOException to prevent resource leaks on undeploy/redeploy.
- Around line 265-311: performHealthCheck currently exposes raw error messages
into the returned details map; change its signature to accept a boolean
includeDetails and use that flag to avoid placing sensitive error strings into
details (but continue logging full errors internally). Specifically, update
performHealthCheck(componentName, details, checker) ->
performHealthCheck(componentName, details, checker, includeDetails), and when
result.isHealthy is false or an exception is caught, set details.put("error",
includeDetails ? safeErrorOrExceptionMessage : "suppressed") and set
details.put("errorType", includeDetails ? "CheckFailed"/"InternalError" :
"Sensitive"); keep logger.error/logger.warn calls unchanged so full diagnostics
remain in logs but not returned to unauthenticated callers. Ensure you handle
HealthCheckResult.error, e.getCause()/e.getMessage() branches accordingly so no
internal messages are added to details when includeDetails is false.
In `@src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java`:
- Line 89: There is a typo in the log message inside JwtUserIdValidationFilter:
change the logger.info call that currently logs "Common-API incoming userAget:
{}" to correctly spell "userAgent" so it reads "Common-API incoming userAgent:
{}", keeping the same logger invocation and the userAgent variable reference.
- Around line 48-53: In JwtUserIdValidationFilter, fix the logger typo by
renaming "userAget" to "userAgent" and update the public-endpoint matching logic
that currently uses path.endsWith("/health") and path.endsWith("/version"):
replace the permissive suffix checks with stricter checks (e.g., exact equals or
a defined prefix match such as path.equals("/health") / path.equals("/version")
or path.startsWith("/public/") as appropriate) so JWT validation isn't
accidentally bypassed; update the logger.info call that references the path to
use the corrected "userAgent" variable name.
---
Nitpick comments:
In
`@src/main/java/com/iemr/common/identity/controller/health/HealthController.java`:
- Around line 94-127: The isUserAuthenticated method duplicates JWT extraction
logic from JwtUserIdValidationFilter; refactor by adding a shared extractor
(e.g., JwtAuthenticationUtil.extractTokenFromRequest(HttpServletRequest)) that
encapsulates reading JwtToken header, Authorization Bearer header, and cookies
(preserving current precedence), then update
HealthController.isUserAuthenticated to call that extractor and then
jwtAuthenticationUtil.validateUserIdAndJwtToken(token); also modify
JwtUserIdValidationFilter to use the same new extractor so token parsing logic
is maintained in one place.
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Around line 121-154: The health checks in checkHealth currently run
sequentially; modify checkHealth to run checkMySQLHealth, checkRedisHealth (only
if redisTemplate!=null) and checkElasticsearchHealth (only if
elasticsearchEnabled && elasticsearchClientReady) concurrently using
CompletableFuture.supplyAsync with your existing ExecutorService and combine
them with CompletableFuture.allOf, then gather each result future (handling
exceptions by returning a failing component map) and populate components and
overallHealth using isHealthy; ensure timestamp, STATUS_KEY, logging and
includeDetails behavior remain the same and that exceptions are logged and
translated into component DOWN entries so a hung call doesn't block the whole
check.
- Around line 362-416: The current manual parsing in extractHost, extractPort,
and extractDatabaseName is fragile; replace it by stripping the "jdbc:" prefix
and using java.net.URI to parse the remaining URI (or URI.create) so you
correctly handle credentials, IPv6, and multi-host forms: in extractHost call
new URI(withoutJdbc).getHost() and return UNKNOWN_VALUE if null, in extractPort
call new URI(...).getPort() and return "3306" when getPort() returns -1, and in
extractDatabaseName use new URI(...).getPath() (trim leading '/') and strip any
query component; wrap URI creation in try/catch and log debug on failure
preserving existing fallback values so behavior remains safe for malformed
inputs.
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pom.xml`:
- Around line 248-253: The pom dependency for
org.elasticsearch.client:elasticsearch-rest-client should be upgraded from
8.10.0 to 9.3.0 and the codebase evaluated for API changes (consider migrating
to the newer Rest5Client API). Update the
<artifactId>org.elasticsearch.client:elasticsearch-rest-client</artifactId>
<version> to 9.3.0 in the pom, run your full build/tests, and search for usages
of classes/methods from RestClient/RestClientBuilder to adapt to breaking
changes (replace with Rest5Client equivalents where needed); fix
compilation/test failures and update any configuration or client instantiation
code accordingly. Ensure compatibility by reading the 9.x migration notes and
adjust serialization/response handling if APIs differ.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (2)
46-46: Consider downgrading per-request path logging toDEBUG.
logger.info(...)on every inbound request will produce a high-volume log stream in production.logger.debug(...)keeps the detail available during troubleshooting without polluting production logs.♻️ Proposed change
- logger.info("JwtUserIdValidationFilter invoked for path: {}", path); + logger.debug("JwtUserIdValidationFilter invoked for path: {}", path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java` at line 46, The per-request logging in JwtUserIdValidationFilter uses logger.info("JwtUserIdValidationFilter invoked for path: {}", path) which is too noisy for production; change this call to logger.debug(...) so the path is logged at DEBUG level (leave the message text and the path variable unchanged) to keep the detail available for troubleshooting without polluting INFO logs.
45-53: Consider usinggetServletPath()or stripping the context path for more robust public-endpoint bypass.The code uses
getRequestURI()to check for public endpoints (/health,/version), butgetRequestURI()includes the servlet context path. While the application currently deploys with the default root context, this creates a latent bug: ifserver.servlet.context-pathis ever configured (e.g., to/identity),getRequestURI()would return/identity/health, causingpath.equals("/health")to fail silently. Requests to these endpoints would then be incorrectly rejected with 401.A more defensive approach is to use
getServletPath()directly (which excludes context path), or explicitly strip the context path:- String path = request.getRequestURI(); + String path = request.getServletPath();This ensures the bypass works correctly regardless of context-path configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java` around lines 45 - 53, The current JwtUserIdValidationFilter uses request.getRequestURI() to compute path which includes the context path and can break the public-endpoint checks; change the logic to use request.getServletPath() (or strip request.getContextPath() from getRequestURI()) when computing the path variable used in the comparisons, then keep the existing checks for "/health" and "/version" and call filterChain.doFilter(servletRequest, servletResponse) as before; update the logger line to log the servletPath (or stripped path) and adjust any references to the path variable accordingly so JwtUserIdValidationFilter correctly bypasses public endpoints regardless of context-path.src/main/java/com/iemr/common/identity/service/health/HealthService.java (2)
500-510: Elasticsearch health check only verifies connectivity, not cluster health status.
/_cluster/healthreturns a JSON body with a"status"field (green,yellow,red). Currently, any HTTP 200 response reports the component as UP, even if the cluster is inredstate. Consider parsing the response body and reflecting degraded status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 500 - 510, The Elasticsearch health check currently treats any HTTP 200 from elasticsearchRestClient.performRequest(request) as UP; update HealthService to parse the response body JSON (from response.getEntity()) and inspect the "status" field returned by "/_cluster/health" instead of relying solely on statusCode: map "green" => return new HealthCheckResult(true,...), "yellow" => return new HealthCheckResult(false, "DEGRADED", "<status>"), and "red" => return new HealthCheckResult(false, null, "<status>") (or similar semantics used elsewhere); ensure to handle JSON parsing exceptions and non-200 responses by falling back to the existing HTTP error path and include ELASTICSEARCH_TYPE and any parse error details in the log; touch methods/vars: HealthService, elasticsearchRestClient.performRequest, ELASTICSEARCH_TYPE, HealthCheckResult.
134-167: Health checks run sequentially — consider parallel execution.All three component checks (MySQL, Redis, Elasticsearch) execute sequentially, so worst-case latency is the sum of all timeouts (~9s+). Since you already have an
ExecutorService, you could submit them asCompletableFuturetasks and join, cutting latency to the single slowest component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 134 - 167, The health checks currently call checkMySQLHealth, checkRedisHealth and checkElasticsearchHealth sequentially; change to run them in parallel via CompletableFuture.supplyAsync(..., executorService) (or your existing ExecutorService) for each enabled component, then join all futures (CompletableFuture.allOf or join per-future), collect each result into the components map, and compute overallHealth by invoking isHealthy on each returned Map; preserve the existing STATUS_KEY/timestamp/components population and logger.info call after futures complete. Ensure exceptions from futures are handled (wrap failures into a down-status Map for that component) so one failing future doesn't break aggregation.
🤖 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/identity/service/health/HealthService.java`:
- Around line 198-297: The methods for advanced DB health
(addAdvancedMySQLMetrics, isMySQLDatabase, addDeepMySQLHealthChecks,
checkTableLocks, checkStuckProcesses, checkInnoDBStatus,
getMySQLStatusVariables, getMySQLVersion, getRedisVersionWithTimeout,
extractHost, extractPort, extractDatabaseName) are dead code in this PR — either
remove them now or wire them into the class before merging; if you keep them,
fix the internal issues: in addAdvancedMySQLMetrics remove the unguarded
double-parsing of Threads_connected and max_connections (the first
Integer.parseInt call), wrap parsing of slowQueries and questions in try/catch
like uptime to guard NumberFormatException, and update checkInnoDBStatus/any
query using INFORMATION_SCHEMA.INNODB_LOCKS to use performance_schema.data_locks
for MySQL 8+ compatibility. Ensure references are only kept if the methods are
actually called (or annotate/feature-flag them) to avoid shipping dead code.
---
Duplicate comments:
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Around line 558-579: Both buildUnhealthyStatus and buildExceptionStatus ignore
the includeDetails flag and always expose sensitive error information in the
response; update them to only add sensitive fields to the returned details map
when includeDetails is true. Specifically, in buildUnhealthyStatus (method name)
only call details.put("error", ...) and details.put("errorType", ...) when
includeDetails is true (still set status to STATUS_DOWN and log the warning as
before); in buildExceptionStatus (method name) always set status and
responseTimeMs but only put the detailed error message into details
(details.put("error", ...) and any stack/exception-specific fields) when
includeDetails is true—otherwise set a generic non-sensitive error like "Health
check failed" in the response and ensure status.put("details", details) remains
correct. Ensure logging (logger.warn/logger.error) can remain for internal logs
but do not expose exception.getMessage()/getCause() in the response unless
includeDetails is true.
In `@src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java`:
- Around line 86-89: The earlier typos and logging style issues have been
resolved: ensure the variable name is consistently userAgent (references in
JwtUserIdValidationFilter) and that both logger calls use SLF4J placeholder
syntax (logger.info("User-Agent: {}", userAgent) and logger.info("Common-API
incoming userAgent: {}", userAgent)); no further code changes required beyond
verifying there are no remaining userAget occurrences and that all log
statements in JwtUserIdValidationFilter use placeholders rather than string
concatenation.
---
Nitpick comments:
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Around line 500-510: The Elasticsearch health check currently treats any HTTP
200 from elasticsearchRestClient.performRequest(request) as UP; update
HealthService to parse the response body JSON (from response.getEntity()) and
inspect the "status" field returned by "/_cluster/health" instead of relying
solely on statusCode: map "green" => return new HealthCheckResult(true,...),
"yellow" => return new HealthCheckResult(false, "DEGRADED", "<status>"), and
"red" => return new HealthCheckResult(false, null, "<status>") (or similar
semantics used elsewhere); ensure to handle JSON parsing exceptions and non-200
responses by falling back to the existing HTTP error path and include
ELASTICSEARCH_TYPE and any parse error details in the log; touch methods/vars:
HealthService, elasticsearchRestClient.performRequest, ELASTICSEARCH_TYPE,
HealthCheckResult.
- Around line 134-167: The health checks currently call checkMySQLHealth,
checkRedisHealth and checkElasticsearchHealth sequentially; change to run them
in parallel via CompletableFuture.supplyAsync(..., executorService) (or your
existing ExecutorService) for each enabled component, then join all futures
(CompletableFuture.allOf or join per-future), collect each result into the
components map, and compute overallHealth by invoking isHealthy on each returned
Map; preserve the existing STATUS_KEY/timestamp/components population and
logger.info call after futures complete. Ensure exceptions from futures are
handled (wrap failures into a down-status Map for that component) so one failing
future doesn't break aggregation.
In `@src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java`:
- Line 46: The per-request logging in JwtUserIdValidationFilter uses
logger.info("JwtUserIdValidationFilter invoked for path: {}", path) which is too
noisy for production; change this call to logger.debug(...) so the path is
logged at DEBUG level (leave the message text and the path variable unchanged)
to keep the detail available for troubleshooting without polluting INFO logs.
- Around line 45-53: The current JwtUserIdValidationFilter uses
request.getRequestURI() to compute path which includes the context path and can
break the public-endpoint checks; change the logic to use
request.getServletPath() (or strip request.getContextPath() from
getRequestURI()) when computing the path variable used in the comparisons, then
keep the existing checks for "/health" and "/version" and call
filterChain.doFilter(servletRequest, servletResponse) as before; update the
logger line to log the servletPath (or stripped path) and adjust any references
to the path variable accordingly so JwtUserIdValidationFilter correctly bypasses
public endpoints regardless of context-path.
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/identity/service/health/HealthService.java (2)
104-121: Consider upgradingelasticsearch-rest-clientfrom 8.10.0 to the latest stable patch.The latest stable release with a matching low-level rest client is
8.16.6. Staying on 8.10.0 means missing several maintenance and security patches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 104 - 121, The project is using elasticsearch-rest-client 8.10.0 and should be upgraded to the latest stable matching low-level client (8.16.6); update the dependency/version in your build file (pom.xml or build.gradle) to 8.16.6, then run the build (mvn/gradle) and tests, and fix any API changes surfaced in initializeElasticsearchClient or usages of elasticsearchRestClient and elasticsearchClientReady (adjust imports or method signatures if needed) to ensure the RestClient instantiation and timeouts still compile and behave as before.
263-273: Remove unusederrorMessagevariable.
errorMessageis computed at line 267 but never put into thestatusmap or used anywhere. It's a leftover from the prior refactor that removed error exposure to callers.♻️ Proposed cleanup
} catch (Exception e) { long responseTime = System.currentTimeMillis() - startTime; logger.error("{} health check failed with exception: {}", componentName, e.getMessage(), e); - - String errorMessage = e.getCause() != null ? e.getCause().getMessage() : e.getMessage(); status.put(STATUS_KEY, STATUS_DOWN); status.put("responseTimeMs", responseTime); return status; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 263 - 273, In HealthService (src/main/java/com/iemr/common/identity/service/health/HealthService.java) remove the unused local variable errorMessage (the line computing e.getCause() ? ... assigned to errorMessage) from the catch block so the code does not compute a value that's never used; leave the existing logger.error, status.put(STATUS_KEY, STATUS_DOWN) and responseTime logic unchanged and run tests/compile to confirm no unused-variable warnings remain.src/main/java/com/iemr/common/identity/controller/health/HealthController.java (1)
49-57: Remove unusedjwtAuthenticationUtildependency andHttpServletRequestparameter.
jwtAuthenticationUtilis injected but never called in any method. Likewise, theHttpServletRequest requestparameter incheckHealthis never accessed. Both are dead code.The injected
JwtAuthenticationUtilis a particular concern: since/healthis already whitelisted in the JWT filter, no auth is needed here. More critically, ifJwtAuthenticationUtilhas transitive dependencies that fail to initialize, theHealthControlleritself will fail to start — directly defeating the purpose of a health endpoint.♻️ Proposed cleanup
`@RestController` `@RequestMapping`("/health") `@Tag`(name = "Health Check", description = "APIs for checking infrastructure health status") public class HealthController { private static final Logger logger = LoggerFactory.getLogger(HealthController.class); private final HealthService healthService; - private final JwtAuthenticationUtil jwtAuthenticationUtil; - public HealthController(HealthService healthService, JwtAuthenticationUtil jwtAuthenticationUtil) { + public HealthController(HealthService healthService) { this.healthService = healthService; - this.jwtAuthenticationUtil = jwtAuthenticationUtil; } `@GetMapping` ... - public ResponseEntity<Map<String, Object>> checkHealth(HttpServletRequest request) { + public ResponseEntity<Map<String, Object>> checkHealth() {Also remove the now-unused imports:
-import jakarta.servlet.http.Cookie; -import jakarta.servlet.http.HttpServletRequest; -import com.iemr.common.identity.utils.JwtAuthenticationUtil;Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/controller/health/HealthController.java` around lines 49 - 57, Remove the unused JwtAuthenticationUtil injection and the unused HttpServletRequest parameter from the HealthController: delete the private final JwtAuthenticationUtil jwtAuthenticationUtil field and its constructor parameter in HealthController(HealthService healthService, JwtAuthenticationUtil jwtAuthenticationUtil) (and update the constructor to only accept HealthService), and remove the HttpServletRequest request parameter from the checkHealth(...) method signature; also remove any now-unused imports related to JwtAuthenticationUtil and HttpServletRequest so the controller has no dead dependencies that could break startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/com/iemr/common/identity/controller/health/HealthController.java`:
- Around line 49-57: Remove the unused JwtAuthenticationUtil injection and the
unused HttpServletRequest parameter from the HealthController: delete the
private final JwtAuthenticationUtil jwtAuthenticationUtil field and its
constructor parameter in HealthController(HealthService healthService,
JwtAuthenticationUtil jwtAuthenticationUtil) (and update the constructor to only
accept HealthService), and remove the HttpServletRequest request parameter from
the checkHealth(...) method signature; also remove any now-unused imports
related to JwtAuthenticationUtil and HttpServletRequest so the controller has no
dead dependencies that could break startup.
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Around line 104-121: The project is using elasticsearch-rest-client 8.10.0 and
should be upgraded to the latest stable matching low-level client (8.16.6);
update the dependency/version in your build file (pom.xml or build.gradle) to
8.16.6, then run the build (mvn/gradle) and tests, and fix any API changes
surfaced in initializeElasticsearchClient or usages of elasticsearchRestClient
and elasticsearchClientReady (adjust imports or method signatures if needed) to
ensure the RestClient instantiation and timeouts still compile and behave as
before.
- Around line 263-273: In HealthService
(src/main/java/com/iemr/common/identity/service/health/HealthService.java)
remove the unused local variable errorMessage (the line computing e.getCause() ?
... assigned to errorMessage) from the catch block so the code does not compute
a value that's never used; leave the existing logger.error,
status.put(STATUS_KEY, STATUS_DOWN) and responseTime logic unchanged and run
tests/compile to confirm no unused-variable warnings remain.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/main/java/com/iemr/common/identity/service/health/HealthService.java (4)
103-120: Hardcoded"http"scheme prevents HTTPS connections to Elasticsearch.Line 106 hardcodes the scheme to
"http". Production Elasticsearch clusters commonly require TLS. Consider making the scheme configurable.Proposed fix
Add a new
@Valueproperty:`@Value`("${elasticsearch.scheme:http}") String elasticsearchSchemeThen use it:
- new HttpHost(elasticsearchHost, elasticsearchPort, "http") + new HttpHost(elasticsearchHost, elasticsearchPort, elasticsearchScheme)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 103 - 120, initializeElasticsearchClient currently hardcodes the scheme "http", preventing TLS connections; add a new injectable field (e.g. `@Value`("${elasticsearch.scheme:http}") String elasticsearchScheme) to the class and replace the hardcoded "http" in the HttpHost(...) call with elasticsearchScheme so the method uses the configured scheme (allowing "https" in production) and retains "http" as the default.
152-152:INFO-level log on every health check will be noisy in production.Health endpoints are typically polled every few seconds by load balancers and orchestration platforms. Logging at
INFOon each invocation produces excessive log volume. ConsiderDEBUGhere, or log atINFOonly on status transitions.Proposed fix
- logger.info("Health check completed - Overall status: {}", overallHealth ? STATUS_UP : STATUS_DOWN); + logger.debug("Health check completed - Overall status: {}", overallHealth ? STATUS_UP : STATUS_DOWN);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` at line 152, The health check currently logs every invocation at INFO using logger.info("Health check completed - Overall status: {}", overallHealth ? STATUS_UP : STATUS_DOWN) which is noisy; change the logging strategy in HealthService (the method performing the health check that sets overallHealth) to either log at DEBUG for every check or emit INFO only when the status changes (compare previous status to overallHealth before logging and update previous status), using the same message format with STATUS_UP/STATUS_DOWN so you retain context.
91-101:shutdownNow()withoutawaitTermination— in-flight tasks may not terminate cleanly.If a Redis health check is in progress when the context shuts down,
shutdownNow()only requests cancellation. WithoutawaitTermination, the@PreDestroymethod returns immediately and the container may proceed with shutdown while pool threads are still running.Proposed fix
`@jakarta.annotation.PreDestroy` public void cleanup() { executorService.shutdownNow(); + try { + if (!executorService.awaitTermination(5, TimeUnit.SECONDS)) { + logger.warn("ExecutorService did not terminate within timeout"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } if (elasticsearchRestClient != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 91 - 101, The cleanup() method calls executorService.shutdownNow() without waiting for in-flight tasks to finish; change it to first call executorService.shutdown(), then await termination with a reasonable timeout (e.g., via executorService.awaitTermination(timeout, unit)), and only call shutdownNow() if awaitTermination returns false; also catch InterruptedException, log it via logger.warn in cleanup(), and restore the thread interrupt status; keep the existing elasticsearchRestClient.close() handling as-is.
277-285:HealthCheckResultcould be converted to a Javarecord.The project targets Java 17, which fully supports records. This inner class is a good candidate for a
record, eliminating boilerplate code.Proposed change
- private static class HealthCheckResult { - final boolean isHealthy; - final String error; - - HealthCheckResult(boolean isHealthy, String error) { - this.isHealthy = isHealthy; - this.error = error; - } - } + private record HealthCheckResult(boolean isHealthy, String error) {}Accessors at lines 251 and 255 would change from field access to method calls:
result.isHealthy→result.isHealthy()result.error→result.error()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 277 - 285, Convert the inner static class HealthCheckResult into a Java record to remove boilerplate: replace "private static class HealthCheckResult { final boolean isHealthy; final String error; ... }" with "private static record HealthCheckResult(boolean isHealthy, String error) {}", then update all call sites to use the record accessor methods (e.g., change result.isHealthy to result.isHealthy() and result.error to result.error()). Ensure constructors/usages that instantiate HealthCheckResult remain valid using the record's canonical constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Around line 103-120: initializeElasticsearchClient currently hardcodes the
scheme "http", preventing TLS connections; add a new injectable field (e.g.
`@Value`("${elasticsearch.scheme:http}") String elasticsearchScheme) to the class
and replace the hardcoded "http" in the HttpHost(...) call with
elasticsearchScheme so the method uses the configured scheme (allowing "https"
in production) and retains "http" as the default.
- Line 152: The health check currently logs every invocation at INFO using
logger.info("Health check completed - Overall status: {}", overallHealth ?
STATUS_UP : STATUS_DOWN) which is noisy; change the logging strategy in
HealthService (the method performing the health check that sets overallHealth)
to either log at DEBUG for every check or emit INFO only when the status changes
(compare previous status to overallHealth before logging and update previous
status), using the same message format with STATUS_UP/STATUS_DOWN so you retain
context.
- Around line 91-101: The cleanup() method calls executorService.shutdownNow()
without waiting for in-flight tasks to finish; change it to first call
executorService.shutdown(), then await termination with a reasonable timeout
(e.g., via executorService.awaitTermination(timeout, unit)), and only call
shutdownNow() if awaitTermination returns false; also catch
InterruptedException, log it via logger.warn in cleanup(), and restore the
thread interrupt status; keep the existing elasticsearchRestClient.close()
handling as-is.
- Around line 277-285: Convert the inner static class HealthCheckResult into a
Java record to remove boilerplate: replace "private static class
HealthCheckResult { final boolean isHealthy; final String error; ... }" with
"private static record HealthCheckResult(boolean isHealthy, String error) {}",
then update all call sites to use the record accessor methods (e.g., change
result.isHealthy to result.isHealthy() and result.error to result.error()).
Ensure constructors/usages that instantiate HealthCheckResult remain valid using
the record's canonical constructor.
…, graceful shutdown, record)
…y write for elasticsearch health check
…d clean code smells
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
drtechie
approved these changes
Mar 2, 2026
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
2 participants
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.
📋 Description
JIRA ID:
This PR implements /health and /version endpoints in the Identity-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Accessibility
Improvements
Chores