feat(health,version): add health and version endpoints without auth - #120
Merged
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:
📝 WalkthroughWalkthroughAdds two REST endpoints (/health, /version), a HealthService performing MySQL and Redis checks, inserts the git-commit-id-maven-plugin twice in Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Filter as JwtFilter
participant Controller as HealthController
participant Service as HealthService
participant DB as DataSource
participant Redis as Redis
Client->>Filter: HTTP GET /health
Filter->>Controller: forward (skip validation for /health)
Controller->>Service: checkHealth()
par MySQL check
Service->>DB: execute quick health query (3s timeout)
DB-->>Service: result / error
and Redis check
Service->>Redis: PING (3s timeout)
Redis-->>Service: PONG / error
end
Service-->>Controller: aggregated health map (components + overall status)
Controller-->>Client: HTTP 200 (UP/DEGRADED) or 503 (DOWN) with payload
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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/main/java/com/iemr/ecd/service/health/HealthService.java (2)
91-94: Unauthenticated requests still expose host, port, and database name ifincludeDetailsistrue.Looking at the flow:
HealthController.isUserAuthenticated()determinesincludeDetails. ThecheckHealth()no-arg overload (line 83-85) defaults toincludeDetails=true. Ensure this no-arg overload isn't called from any unauthenticated code path, as it would leak infrastructure details (host, port, database name) to anonymous users.Currently only
HealthControllercallscheckHealth(isAuthenticated), so this is safe today, but the no-arg overload is public and could be misused.Consider making the no-arg overload package-private or removing it
- public Map<String, Object> checkHealth() { + // Consider restricting visibility or removing if not needed externally + Map<String, Object> checkHealth() { return checkHealth(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 91 - 94, The no-arg public overload HealthService.checkHealth() currently defaults includeDetails=true and can leak host/port/database; change this public no-arg method to either be removed or made package-private (or protected) so it cannot be called from unauthenticated callers, and update any callers to explicitly call checkHealth(boolean includeDetails) (e.g., HealthController should continue calling checkHealth(isUserAuthenticated())); ensure the sensitive-detail population logic in checkHealth(boolean) (extractHost, extractPort, extractDatabaseName) remains gated by the includeDetails flag.
243-297: JDBC URL parsing is fragile for non-standard URL formats.The
extractHost/extractPort/extractDatabaseNamehelpers assume a simplejdbc:mysql://host:port/db?paramsformat. They won't handle failover URLs (jdbc:mysql://host1:3306,host2:3306/db), URLs with embedded credentials, or other JDBC URL variants. The defensivetry/catchwithUNKNOWN_VALUEfallback makes this safe but potentially misleading.This is acceptable for a health display, but consider documenting the limitation or using
java.net.URIfor more robust parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 243 - 297, The helper methods extractHost, extractPort and extractDatabaseName are fragile for non-standard JDBC strings (failover lists, embedded credentials, etc.); update them to robustly parse JDBC URLs by first stripping the "jdbc:mysql://" prefix, then if the authority portion contains credentials (user@host...) remove the credentials, split on commas to handle failover lists and take the first host:port entry, and then parse host and optional port (fall back to 3306) and database (strip query params) from that entry; alternatively use java.net.URI by replacing the "jdbc:" prefix and handling missing scheme/authority to extract host, port and path safely. Apply these changes in the methods extractHost, extractPort and extractDatabaseName and add a short comment documenting remaining limitations if any.src/main/java/com/iemr/ecd/controller/version/VersionController.java (1)
49-67: Consider loadinggit.propertiesonce at startup instead of per-request.
loadGitProperties()reads from the classpath on every invocation of/version. Since this file is static after build, loading it once (e.g., in the constructor or a@PostConstructmethod) avoids repeated I/O and simplifies the method.♻️ Cache properties at startup
`@RestController` public class VersionController { private final Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); - private static final String UNKNOWN_VALUE = "unknown"; + private final Map<String, String> versionInfo; + + public VersionController() { + this.versionInfo = loadVersionInfo(); + } `@Operation`(summary = "Get version information") `@GetMapping`(value = "/version", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity<Map<String, String>> versionInformation() { - Map<String, String> response = new LinkedHashMap<>(); - try { - logger.info("version Controller Start"); - Properties gitProperties = loadGitProperties(); - response.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); - response.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); - response.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); - response.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); - } catch (Exception e) { - logger.error("Failed to load version information", e); - response.put("buildTimestamp", UNKNOWN_VALUE); - response.put("version", UNKNOWN_VALUE); - response.put("branch", UNKNOWN_VALUE); - response.put("commitHash", UNKNOWN_VALUE); - } - logger.info("version Controller End"); + logger.info("version Controller Start"); + Map<String, String> response = new LinkedHashMap<>(versionInfo); + logger.info("version Controller End"); return ResponseEntity.ok(response); } + private Map<String, String> loadVersionInfo() { + Map<String, String> info = new LinkedHashMap<>(); + try { + Properties gitProperties = loadGitProperties(); + info.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); + info.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); + info.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); + info.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); + } catch (Exception e) { + logger.error("Failed to load version information", e); + info.put("buildTimestamp", UNKNOWN_VALUE); + info.put("version", UNKNOWN_VALUE); + info.put("branch", UNKNOWN_VALUE); + info.put("commitHash", UNKNOWN_VALUE); + } + return info; + } + private Properties loadGitProperties() throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/controller/version/VersionController.java` around lines 49 - 67, The versionInformation() method calls loadGitProperties() on every request; change this by loading and caching the Properties once at startup (e.g., in the class constructor or a `@PostConstruct` method) into a private field like cachedGitProperties, and have versionInformation() read from cachedGitProperties (falling back to UNKNOWN_VALUE if cache is null or missing keys). Ensure loadGitProperties() is invoked only during startup, preserve the existing exception handling (log errors when initializing the cache), and update references in versionInformation() to use cachedGitProperties instead of calling loadGitProperties() per-request.src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java (1)
111-113:startsWithmay unintentionally bypass auth for future sub-paths.Using
path.startsWith(contextPath + "/health")also matches paths like/healthcheck,/health-admin, etc. Since these are leaf endpoints, consider usingpath.equals(...)or matching with a trailing slash as well (path.startsWith(contextPath + "/health/")) to avoid accidentally opening future endpoints.This is consistent with the existing approach (e.g.,
/public,/swagger-ui), but those are intentionally prefix-based, whereas/healthand/versionare single endpoints.Tighter path matching
- || path.startsWith(contextPath + "/health") - || path.startsWith(contextPath + "/version")) { + || path.equals(contextPath + "/health") + || path.equals(contextPath + "/version")) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java` around lines 111 - 113, The current path checks in JwtUserIdValidationFilter use path.startsWith(contextPath + "/health") and path.startsWith(contextPath + "/version"), which may unintentionally match longer endpoints (e.g., /healthcheck); update the conditional to only allow the exact leaf endpoints by replacing those two checks with either path.equals(contextPath + "/health") || path.equals(contextPath + "/health/") (and same for "/version") or use startsWith(contextPath + "/health/") plus an exact equals fallback, so the logic only bypasses auth for the intended /health and /version endpoints while still leaving prefix-based checks (e.g., /public) unchanged.
🤖 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 427-456: Remove the four unused git properties from the plugin's
includeOnlyProperties to shrink the generated git.properties: delete entries for
git.commit.message.short, git.commit.user.name, git.dirty, and
git.total.commit.count in the git-commit-id-maven-plugin configuration (the
includeOnlyProperties block). Keep only the properties referenced by
VersionController (git.build.time, git.build.version, git.branch,
git.commit.id.abbrev) so the generated file contains just the values consumed by
VersionController.
In `@src/main/java/com/iemr/ecd/controller/health/HealthController.java`:
- Around line 71-89: isUserAuthenticated currently only reads the
"Authorization" header so requests that supply the JWT via the application's
"Jwttoken" header (Constants.JWT_TOKEN) or cookies will be treated as
unauthenticated; update isUserAuthenticated to first check for a token in
request.getHeader(Constants.JWT_TOKEN) (and if absent fall back to Authorization
handling including "Bearer " prefix) and optionally fall back to the cookie
lookup logic used by JwtUserIdValidationFilter, then pass the resolved token to
jwtAuthenticationUtil.validateUserIdAndJwtToken(token) inside the existing
try/catch (keep the logger.debug on exception).
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Line 36: The static ExecutorService executorService in HealthService is an
unbounded cached thread pool that is never shut down; replace it with a bounded,
Spring-managed executor and ensure lifecycle cleanup: make the field non-static
(remove private static final ExecutorService executorService), inject a
TaskExecutor/Executor (e.g., via a constructor parameter annotated
`@Qualifier`("taskExecutor") or `@Autowired`) or create a fixed-size ExecutorService
with a clear maxThreads, and add a `@PreDestroy` method to call
shutdown/shutdownNow for the ExecutorService if you instantiate it locally;
update any usages in HealthService to use the injected executor field.
- Around line 46-48: The HealthService constructor is using outdated Redis
property names; update the `@Value` annotations for the redis parameters in the
HealthService class to use Spring Boot 3.2 keys: change
`@Value`("${spring.redis.host:localhost}") String redisHost to
`@Value`("${spring.data.redis.host:localhost}") String redisHost and change
`@Value`("${spring.redis.port:6379}") int redisPort to
`@Value`("${spring.data.redis.port:6379}") int redisPort so the health check
reflects configured values; keep dbUrl unchanged.
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/controller/version/VersionController.java`:
- Around line 49-67: The versionInformation() method calls loadGitProperties()
on every request; change this by loading and caching the Properties once at
startup (e.g., in the class constructor or a `@PostConstruct` method) into a
private field like cachedGitProperties, and have versionInformation() read from
cachedGitProperties (falling back to UNKNOWN_VALUE if cache is null or missing
keys). Ensure loadGitProperties() is invoked only during startup, preserve the
existing exception handling (log errors when initializing the cache), and update
references in versionInformation() to use cachedGitProperties instead of calling
loadGitProperties() per-request.
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 91-94: The no-arg public overload HealthService.checkHealth()
currently defaults includeDetails=true and can leak host/port/database; change
this public no-arg method to either be removed or made package-private (or
protected) so it cannot be called from unauthenticated callers, and update any
callers to explicitly call checkHealth(boolean includeDetails) (e.g.,
HealthController should continue calling checkHealth(isUserAuthenticated()));
ensure the sensitive-detail population logic in checkHealth(boolean)
(extractHost, extractPort, extractDatabaseName) remains gated by the
includeDetails flag.
- Around line 243-297: The helper methods extractHost, extractPort and
extractDatabaseName are fragile for non-standard JDBC strings (failover lists,
embedded credentials, etc.); update them to robustly parse JDBC URLs by first
stripping the "jdbc:mysql://" prefix, then if the authority portion contains
credentials (user@host...) remove the credentials, split on commas to handle
failover lists and take the first host:port entry, and then parse host and
optional port (fall back to 3306) and database (strip query params) from that
entry; alternatively use java.net.URI by replacing the "jdbc:" prefix and
handling missing scheme/authority to extract host, port and path safely. Apply
these changes in the methods extractHost, extractPort and extractDatabaseName
and add a short comment documenting remaining limitations if any.
In `@src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java`:
- Around line 111-113: The current path checks in JwtUserIdValidationFilter use
path.startsWith(contextPath + "/health") and path.startsWith(contextPath +
"/version"), which may unintentionally match longer endpoints (e.g.,
/healthcheck); update the conditional to only allow the exact leaf endpoints by
replacing those two checks with either path.equals(contextPath + "/health") ||
path.equals(contextPath + "/health/") (and same for "/version") or use
startsWith(contextPath + "/health/") plus an exact equals fallback, so the logic
only bypasses auth for the intended /health and /version endpoints while still
leaving prefix-based checks (e.g., /public) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/iemr/ecd/service/health/HealthService.java (3)
299-309: Consider using a Javarecordsince the project targets Java 17.
HealthCheckResultis a simple immutable data carrier — a perfect fit for a record.Suggested refactor
- private static class HealthCheckResult { - final boolean isHealthy; - final String version; - final String error; - - HealthCheckResult(boolean isHealthy, String version, String error) { - this.isHealthy = isHealthy; - this.version = version; - this.error = error; - } - } + private record HealthCheckResult(boolean isHealthy, String version, String error) {}Note: record accessor methods are
isHealthy(),version(),error()— update field access inperformHealthCheckaccordingly (e.g.,result.isHealthy→result.isHealthy()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 299 - 309, Replace the inner immutable class HealthCheckResult with a Java record (e.g., record HealthCheckResult(boolean isHealthy, String version, String error)) to simplify the data carrier; then update all usages in performHealthCheck (and elsewhere) to use record accessor methods (result.isHealthy() / result.version() / result.error()) instead of field access (result.isHealthy, result.version, result.error). Ensure the constructor semantics remain the same and remove the old final fields and explicit constructor.
243-277:extractHostandextractPortduplicate the same URL-parsing preamble.Both methods repeat the
replaceFirst/indexOf('/')/indexOf(':')sequence. Consider extracting a shared helper that returns thehost:portpair once.Suggested refactor
+ private String[] extractHostAndPort(String jdbcUrl) { + if (jdbcUrl == null || UNKNOWN_VALUE.equals(jdbcUrl)) { + return new String[]{UNKNOWN_VALUE, UNKNOWN_VALUE}; + } + try { + String withoutPrefix = jdbcUrl.replaceFirst("jdbc:mysql://", ""); + int slashIndex = withoutPrefix.indexOf('/'); + String hostPort = slashIndex > 0 + ? withoutPrefix.substring(0, slashIndex) + : withoutPrefix; + int colonIndex = hostPort.indexOf(':'); + String host = colonIndex > 0 ? hostPort.substring(0, colonIndex) : hostPort; + String port = colonIndex > 0 ? hostPort.substring(colonIndex + 1) : "3306"; + return new String[]{host, port}; + } catch (Exception e) { + logger.debug("Could not parse host/port from URL", e); + return new String[]{UNKNOWN_VALUE, "3306"}; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 243 - 277, extractHost and extractPort duplicate the JDBC URL parsing; extract the common preamble into a private helper (e.g., getHostPortFromJdbcUrl(String jdbcUrl)) that returns the "host:port" string or null/UNKNOWN_VALUE on error, reuse that helper inside extractHost and extractPort to split on ':' for host or port, preserve the same defaults ("3306") and UNKNOWN_VALUE behavior and keep the existing logger.debug exception handling by catching exceptions inside the helper and logging with logger.debug("Could not extract host/port from URL", e).
78-78: Health checkINFOlog on every invocation may be noisy.Load balancers and monitoring systems typically poll
/healthevery few seconds. Logging atINFOlevel for each call will flood the logs. ConsiderDEBUGlevel instead.Suggested change
- 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/ecd/service/health/HealthService.java` at line 78, The health check currently logs every invocation at INFO in HealthService (the logger.info call that prints "Health check completed - Overall status: {}"), which is noisy; change this to logger.debug (or guard it behind logger.isDebugEnabled()) so routine /health polls don't flood logs—update the logging statement in HealthService to use debug-level logging while preserving the same message and status expression (STATUS_UP/STATUS_DOWN).
🤖 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/ecd/service/health/HealthService.java`:
- Around line 286-291: The substring extraction in HealthService (method
handling jdbcUrl using variables jdbcUrl, lastSlash, afterSlash, queryStart)
incorrectly uses the condition "queryStart > 0" which lets a leading '?' (index
0) fall through; change the condition to "queryStart >= 0" so that when
afterSlash starts with '?' (e.g., "/?useSSL...") you correctly return an empty
database name (afterSlash.substring(0, queryStart)) instead of the raw query
string.
---
Duplicate comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Line 36: HealthService currently holds a static ExecutorService
(executorService) that is never shut down; add a clean shutdown to the class by
either converting executorService to a Spring-managed bean (injecting a
TaskExecutor/ThreadPoolTaskExecutor) or keep the field and add a `@PreDestroy`
method (e.g., shutdownExecutor or destroy) that calls
executorService.shutdownNow()/shutdown() and awaits termination with a timeout,
handling InterruptedException and logging appropriately to ensure the thread
pool is terminated on context close/redeploy.
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 299-309: Replace the inner immutable class HealthCheckResult with
a Java record (e.g., record HealthCheckResult(boolean isHealthy, String version,
String error)) to simplify the data carrier; then update all usages in
performHealthCheck (and elsewhere) to use record accessor methods
(result.isHealthy() / result.version() / result.error()) instead of field access
(result.isHealthy, result.version, result.error). Ensure the constructor
semantics remain the same and remove the old final fields and explicit
constructor.
- Around line 243-277: extractHost and extractPort duplicate the JDBC URL
parsing; extract the common preamble into a private helper (e.g.,
getHostPortFromJdbcUrl(String jdbcUrl)) that returns the "host:port" string or
null/UNKNOWN_VALUE on error, reuse that helper inside extractHost and
extractPort to split on ':' for host or port, preserve the same defaults
("3306") and UNKNOWN_VALUE behavior and keep the existing logger.debug exception
handling by catching exceptions inside the helper and logging with
logger.debug("Could not extract host/port from URL", e).
- Line 78: The health check currently logs every invocation at INFO in
HealthService (the logger.info call that prints "Health check completed -
Overall status: {}"), which is noisy; change this to logger.debug (or guard it
behind logger.isDebugEnabled()) so routine /health polls don't flood logs—update
the logging statement in HealthService to use debug-level logging while
preserving the same message and status expression (STATUS_UP/STATUS_DOWN).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/service/health/HealthService.java (1)
128-135: Consider surfacing the failure reason in the health response.
safeErroris only written to the log; the JSON response for aDOWNcomponent contains only{"status":"DOWN","responseTimeMs":…}. Operators polling the/healthendpoint have no direct indication of why a component is down without consulting logs.💡 Optional improvement
if (result.isHealthy) { logger.debug("{} health check: UP ({}ms)", componentName, responseTime); status.put(STATUS_KEY, STATUS_UP); } else { String safeError = result.error != null ? result.error : "Health check failed"; logger.warn("{} health check failed: {}", componentName, safeError); status.put(STATUS_KEY, STATUS_DOWN); + status.put("error", safeError); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 128 - 135, The health response omits the failure reason; update HealthService to include the error text from result.error (or the safeError fallback) in the health JSON for DOWN components: when you populate the status map (STATUS_KEY -> STATUS_DOWN) also add a key like "error" or "failureReason" with safeError and keep responseTimeMs; modify the block around result.isHealthy (references: result, safeError, STATUS_KEY, STATUS_DOWN, status) so consumers of /health can read the failure reason without checking logs.
🤖 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/ecd/service/health/HealthService.java`:
- Line 34: HealthService declares a long-lived ExecutorService instance
(executorService) but never shuts it down; add a lifecycle teardown method
annotated with `@PreDestroy` in the HealthService class that calls
executorService.shutdown(), waits briefly for termination (e.g.,
awaitTermination with a timeout) and calls executorService.shutdownNow() if it
does not terminate; also add the appropriate `@PreDestroy` import
(javax.annotation.PreDestroy or jakarta.annotation.PreDestroy consistent with
the project) to ensure threads are cleanly stopped when the Spring context
closes.
- Around line 95-114: The CompletableFuture returned by
CompletableFuture.supplyAsync is not stored and thus not cancelled on timeout;
update the Redis health check inside performHealthCheck to assign the
CompletableFuture to a local variable (e.g., future), call
future.get(REDIS_TIMEOUT_SECONDS, TimeUnit.SECONDS), and in the TimeoutException
handler call future.cancel(true) to stop the underlying task; ensure
InterruptedException handling still re-interrupts the thread and return
appropriate HealthCheckResult values as before, keeping references to
executorService and REDIS_TIMEOUT_SECONDS unchanged.
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 128-135: The health response omits the failure reason; update
HealthService to include the error text from result.error (or the safeError
fallback) in the health JSON for DOWN components: when you populate the status
map (STATUS_KEY -> STATUS_DOWN) also add a key like "error" or "failureReason"
with safeError and keep responseTimeMs; modify the block around result.isHealthy
(references: result, safeError, STATUS_KEY, STATUS_DOWN, status) so consumers of
/health can read the failure reason without checking logs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/iemr/ecd/service/health/HealthService.java (2)
77-77: MySQL query timeout uses a magic literal3instead of a named constant.
REDIS_TIMEOUT_SECONDSalready captures the 3-second value for Redis; the MySQL path should reuse it (or a sharedDB_TIMEOUT_SECONDS) for consistency and to avoid silent divergence when the timeout is changed.♻️ Proposed refactor
- private static final int REDIS_TIMEOUT_SECONDS = 3; + private static final int REDIS_TIMEOUT_SECONDS = 3; + private static final int DB_TIMEOUT_SECONDS = 3;- stmt.setQueryTimeout(3); + stmt.setQueryTimeout(DB_TIMEOUT_SECONDS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` at line 77, Replace the magic literal timeout in HealthService where stmt.setQueryTimeout(3) is used with the existing named constant REDIS_TIMEOUT_SECONDS (or better, introduce/reuse a shared DB_TIMEOUT_SECONDS constant) so the MySQL query timeout is consistent with other timeouts; update the reference in the HealthService class to use that constant (or create DB_TIMEOUT_SECONDS near other timeout constants and use it in stmt.setQueryTimeout(DB_TIMEOUT_SECONDS)) so future changes remain synchronized.
65-65:INFO-level log on every/healthinvocation will flood logs in production.Kubernetes liveness/readiness probes typically poll
/healthevery few seconds. Logging atINFOgenerates continuous noise. Consider downgrading toDEBUG(or logging only on state transitions).- 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/ecd/service/health/HealthService.java` at line 65, The health-check logging in HealthService currently calls logger.info("Health check completed - Overall status: {}", overallHealth ? STATUS_UP : STATUS_DOWN) on every /health request; change this to logger.debug to avoid noisy INFO logs in production (or implement conditional logging to emit an INFO only when overallHealth changes—store previous state in a field like lastHealthStatus and log on transitions), using the existing overallHealth boolean and STATUS_UP/STATUS_DOWN symbols to build the message.
🤖 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/ecd/service/health/HealthService.java`:
- Around line 129-148: The DOWN branches currently log error details but do not
include them in the response map; update the else branch that computes safeError
(from result.error) to add status.put("error", safeError) and update the
catch(Exception e) branch to compute a sanitized message (e.getMessage() != null
? e.getMessage() : "Health check failed") and add status.put("error",
sanitizedMessage) before returning; this ensures any code reading the map (using
STATUS_KEY, STATUS_UP, STATUS_DOWN, status.put(...)) receives a human-readable
error without exposing full stack traces while keeping existing logger calls
(logger.warn/logger.error).
---
Duplicate comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Line 34: The ExecutorService instance executorService in HealthService is
created with Executors.newFixedThreadPool(2) but never shut down; add a
lifecycle teardown method annotated with `@PreDestroy` inside HealthService that
calls executorService.shutdown(), waits for termination with a timeout (e.g.,
awaitTermination), and falls back to executorService.shutdownNow() if
interrupted or timeout elapses, ensuring proper cleanup of worker threads when
the Spring context closes.
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Line 77: Replace the magic literal timeout in HealthService where
stmt.setQueryTimeout(3) is used with the existing named constant
REDIS_TIMEOUT_SECONDS (or better, introduce/reuse a shared DB_TIMEOUT_SECONDS
constant) so the MySQL query timeout is consistent with other timeouts; update
the reference in the HealthService class to use that constant (or create
DB_TIMEOUT_SECONDS near other timeout constants and use it in
stmt.setQueryTimeout(DB_TIMEOUT_SECONDS)) so future changes remain synchronized.
- Line 65: The health-check logging in HealthService currently calls
logger.info("Health check completed - Overall status: {}", overallHealth ?
STATUS_UP : STATUS_DOWN) on every /health request; change this to logger.debug
to avoid noisy INFO logs in production (or implement conditional logging to emit
an INFO only when overallHealth changes—store previous state in a field like
lastHealthStatus and log on transitions), using the existing overallHealth
boolean and STATUS_UP/STATUS_DOWN symbols to build the message.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/service/health/HealthService.java (1)
92-107: RedundantexecutorService != nullguard in@PreDestroy.
executorServiceisfinaland unconditionally assigned in the constructor, so the!= nullcheck at line 94 is alwaystrue. The@PreDestroyshutdown logic itself is correct and complete.🧹 Proposed cleanup
- if (executorService != null && !executorService.isShutdown()) { - try { + if (!executorService.isShutdown()) { + try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 92 - 107, The null check on executorService inside the `@PreDestroy` shutdown() method is redundant because executorService is final and always set in the constructor; remove the "executorService != null &&" portion and keep the remaining logic (check !executorService.isShutdown(), call executorService.shutdown(), awaitTermination, shutdownNow(), and the InterruptedException handling) so shutdown() now only guards on isShutdown and preserves the existing try/catch and logging behavior referencing executorService and shutdown().
🤖 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/ecd/service/health/HealthService.java`:
- Around line 397-426: The hasDeadlocks method currently reads "LATEST DETECTED
DEADLOCK" from SHOW ENGINE INNODB STATUS which can reflect historical events;
change it to detect recent deadlocks by either (A) querying
information_schema.innodb_metrics for the lock_deadlocks counter (metric name
"lock_deadlocks") and comparing the current value to a stored
baseline/timestamped value to detect a delta, or (B) augmenting the current
approach to parse the timestamp of the last deadlock from INNODB STATUS and only
return true if that timestamp is within a configurable recent window; update
hasDeadlocks to use one of these approaches, add storage for the
baseline/timestamp (e.g., a field like lastDeadlockCount or lastDeadlockTime),
and make the recency window configurable via a new property.
- Line 57: The ADVANCED_CHECKS_TIMEOUT_MS constant is unused and the
per-sub-check timeouts (stmt.setQueryTimeout(2)) allow advanced MySQL checks to
run much longer than intended, blocking executor threads; fix by enforcing the
500ms budget: either (A) reduce each sub-check's JDBC timeout in
performAdvancedMySQLChecks and in hasDeadlocks/hasSlowQueries to 1 (or 0.5s
equivalent) so every executeQuery respects the 500ms cap, or (B) wrap
performAdvancedMySQLChecks in a time-boxed CompletableFuture that uses
ADVANCED_CHECKS_TIMEOUT_MS to cancel/timeout the task (and then handle
mysqlFuture.cancel(true) and exceptions) so the outer checkHealth/mysqFuture
honors the 500ms budget—pick one approach and apply consistently to
performAdvancedMySQLChecks, hasDeadlocks, hasSlowQueries, and the code that
manages mysqlFuture and checkHealth.
- Around line 113-142: The issue is that mysqlStatus and redisStatus are
LinkedHashMap instances mutated by background tasks (performHealthCheck via
checkMySQLHealthSync/checkRedisHealthSync), leading to race conditions and
possible ConcurrentModificationException; replace those LinkedHashMap creations
with thread-safe maps (e.g., new ConcurrentHashMap<>() for mysqlStatus and
redisStatus), keep the method signatures using Map<String,Object> so
performHealthCheck and ensurePopulated can operate unchanged, and when reading
the results (before passing into computeOverallStatus) consider using a snapshot
copy or Collections.unmodifiableMap(new HashMap<>(statusMap)) to avoid observing
partial concurrent writes.
- Around line 187-204: checkRedisHealthSync lacks a per-operation timeout so
redisTemplate.execute(connection -> connection.ping()) can block on Lettuce for
up to its default 60s and tie up the health-check thread pool; update
HealthService to enforce REDIS_TIMEOUT_SECONDS for the Redis path (same approach
used for MySQL): run the ping call in a cancellable task (like the existing
pattern around checkHealth/redisFuture.cancel(true)) and wait with a scoped
timeout, or alternatively set spring.data.redis.timeout to REDIS_TIMEOUT_SECONDS
in configuration; ensure you apply the timeout when invoking
redisTemplate.execute(...) inside checkRedisHealthSync so threads are not left
blocked after redisFuture.cancel(true).
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 92-107: The null check on executorService inside the `@PreDestroy`
shutdown() method is redundant because executorService is final and always set
in the constructor; remove the "executorService != null &&" portion and keep the
remaining logic (check !executorService.isShutdown(), call
executorService.shutdown(), awaitTermination, shutdownNow(), and the
InterruptedException handling) so shutdown() now only guards on isShutdown and
preserves the existing try/catch and logging behavior referencing
executorService and shutdown().
|
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 ECD-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Behavior Change
Chores