feat(health,version): add health and version endpoints without auth#61
Merged
drtechie merged 8 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints without auth#61drtechie merged 8 commits intoPSMRI:mainfrom
drtechie merged 8 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:
📝 WalkthroughWalkthroughAdds a HealthController and HealthService for concurrent MySQL and optional Redis health checks exposed at /health; converts /version to a JSON ResponseEntity using git.properties; updates JWT filter to skip /health and /version; adds git-commit-id-maven-plugin to produce git.properties. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthController
participant HealthService
participant DataSource
participant RedisTemplate
Client->>HealthController: GET /health
HealthController->>HealthService: checkHealth()
HealthService->>DataSource: lightweight query (SELECT 1) / metadata
DataSource-->>HealthService: result / version
alt Redis configured
HealthService->>RedisTemplate: PING / INFO
RedisTemplate-->>HealthService: PONG / info
end
HealthService-->>HealthController: health map (components, status, timestamp)
alt status != "DOWN"
HealthController-->>Client: HTTP 200 + health map
else
HealthController-->>Client: HTTP 503 + health map (status DOWN)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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: 1
🧹 Nitpick comments (7)
src/main/java/com/iemr/common/bengen/controller/version/VersionController.java (1)
69-78: Consider logging whengit.propertiesis not found on the classpath.If
inputisnull, the method silently returns emptyProperties. A debug/warn log here would help diagnose missing build-time configuration.🔍 Proposed improvement
try (InputStream input = getClass().getClassLoader() .getResourceAsStream("git.properties")) { if (input != null) { properties.load(input); + } else { + logger.warn("git.properties not found on classpath"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/controller/version/VersionController.java` around lines 69 - 78, The loadGitProperties method silently returns empty Properties when getResourceAsStream("git.properties") returns null; update loadGitProperties to log a debug/warn message in that branch so missing build-time git.properties is visible (e.g., use the class logger to log that "git.properties" was not found on the classpath before returning properties). Keep the existing behavior of returning empty Properties but add the log statement in the input == null branch and reference the getResourceAsStream call and git.properties to make the intent clear.src/main/java/com/iemr/common/bengen/service/health/HealthService.java (2)
84-88: Unauthenticated callers still see component type ("MySQL","Redis") andresponseTimeMs.When
includeDetailsisfalse, thetypefield is still populated (Line 82, 114) before theif (includeDetails)guard. This leaks which backing stores are in use. Consider whether this is acceptable for your threat model — it's a minor info leak but common in health endpoints.🛡️ Option: Move type into the details guard
private Map<String, Object> checkMySQLHealth(boolean includeDetails) { Map<String, Object> details = new LinkedHashMap<>(); - details.put("type", "MySQL"); if (includeDetails) { + details.put("type", "MySQL"); details.put("host", extractHost(dbUrl)); details.put("port", extractPort(dbUrl)); details.put("database", extractDatabaseName(dbUrl)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` around lines 84 - 88, The health endpoint currently sets the component "type" and "responseTimeMs" outside the includeDetails guard in HealthService, leaking backend types to unauthenticated callers; update the logic so that assignments populating the details map (including the "type" value currently set near Line 82/114 and any "responseTimeMs" entry) are only added when includeDetails is true (i.e., move the code that puts "type" and responseTimeMs into the same if (includeDetails) block that already uses extractHost/extractPort/extractDatabaseName and the details map), or alternatively omit/mask those fields when includeDetails is false.
207-261: JDBC URL parsing is fragile — consider usingURIor a library.The manual
replaceFirst/indexOfparsing assumes the standardjdbc:mysql://host:port/db?paramsformat. It will produce incorrect results for:
- URLs with credentials:
jdbc:mysql://user:pass@host:3306/db- URLs with '/' in query parameters (rare but possible)
- Non-MySQL JDBC URLs if the datasource driver ever changes
A more robust approach:
♻️ Suggested: Use java.net.URI for parsing
private String extractHost(String jdbcUrl) { if (jdbcUrl == null || UNKNOWN_VALUE.equals(jdbcUrl)) { return 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(':'); - return colonIndex > 0 ? hostPort.substring(0, colonIndex) : hostPort; + java.net.URI uri = new java.net.URI(jdbcUrl.replaceFirst("^jdbc:", "")); + return uri.getHost() != null ? uri.getHost() : UNKNOWN_VALUE; } catch (Exception e) { logger.debug("Could not extract host from URL", e); } return UNKNOWN_VALUE; }The same
URIapproach works forextractPortandextractDatabaseName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` around lines 207 - 261, The current manual parsing in extractHost, extractPort, and extractDatabaseName is fragile (fails for credentials, non‑MySQL forms, etc.); replace the string-splitting logic in these methods to use java.net.URI: strip the "jdbc:" prefix (or the "jdbc:mysql://" scheme), create a URI from the remainder, then use URI.getHost(), getPort() (with default 3306 when -1), and URI.getPath() (trim leading '/' and drop any query portion) to determine host, port, and DB name respectively; preserve existing null/UNKNOWN_VALUE checks and logger debug fallbacks so any URI parsing exception returns UNKNOWN_VALUE (or "3306" for port) as before.src/main/java/com/iemr/common/bengen/controller/health/HealthController.java (2)
42-55: Reduce log level for health check invocations.Line 43 logs at
infolevel on every health check call. If this endpoint is polled frequently (e.g., Kubernetes liveness/readiness probes every 10–30 seconds), the logs will be very noisy. Demote todebug.♻️ Proposed fix
- logger.info("Health check endpoint called"); + logger.debug("Health check endpoint called");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/controller/health/HealthController.java` around lines 42 - 55, The health check method checkHealth currently logs every invocation at INFO level causing noise; change the logger.info call in HealthController.checkHealth to logger.debug so routine probes (which call isUserAuthenticated and healthService.checkHealth) are only recorded at debug level while keeping the existing debug log for the status; no other behavior changes required.
57-68: Error response usesMap.of()— good for immutability, but note it excludescomponents.The error response shape (
status,error,timestamp) differs from the normal response shape (status,timestamp,components). Clients consuming this endpoint should handle both shapes. This is fine, but document it in the API contract if not already.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/controller/health/HealthController.java` around lines 57 - 68, The error response in HealthController's exception block uses Map.of(...) and omits the "components" field present in normal responses; change the fallback to return the same response shape by constructing a mutable map (e.g., new LinkedHashMap<>() or HashMap) and populate "status" = "DOWN", "timestamp" = Instant.now().toString(), "components" = Collections.emptyMap(), and optionally include an "error" message; replace the Map.of(...) with this map construction in the catch block so clients always receive a consistent response shape.src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java (2)
3-11: Duplicate import oforg.springframework.stereotype.Component.
Componentis imported on both Line 4 and Line 11. Remove one to keep imports clean.♻️ Proposed fix
import org.springframework.context.annotation.Profile; -import org.springframework.stereotype.Component; import java.io.IOException; import java.util.Arrays;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java` around lines 3 - 11, Remove the duplicate import of org.springframework.stereotype.Component in JwtUserIdValidationFilter: keep a single import statement for Component (remove the redundant one) and run an import cleanup/organize step so there are no duplicate or unused imports; locate the import lines near the top of the JwtUserIdValidationFilter class to make the change.
116-123: Reconsider the use ofstartsWith()for/publicendpoint matching.The filter uses
path.startsWith(contextPath + "/public")(line 118) while usingpath.equals()for/healthand/version(line 119). This asymmetry is worth addressing:startsWith()would match any path beginning with/public, including hypothetical paths like/publicInternal/admin.However, the codebase currently has no endpoints defined under
/public— only/health,/version, and/generateBeneficiaryControllerexist. If/publicendpoints are intended for future use, clarify whether they should all skip JWT validation. Otherwise, align the logic by either:
- Changing
/publictoequals()and defining it as a specific endpoint, or- Documenting that all paths under
/publicare intentionally unauthenticated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java` around lines 116 - 123, JwtUserIdValidationFilter currently uses path.startsWith(contextPath + "/public") while other checks use equals(), which can unintentionally skip JWT validation for any subpaths; either change the check to path.equals(contextPath + "/public") to only exempt the exact /public endpoint, or if every subpath under /public should be unauthenticated, add a clear comment and unit test documenting that intent and leave path.startsWith(...) in place; update the condition in the filter (the block that calls filterChain.doFilter(servletRequest, servletResponse)) and adjust any related tests or documentation accordingly.
🤖 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/bengen/service/health/HealthService.java`:
- Around line 80-110: checkMySQLHealth currently calls
dataSource.getConnection() with no bounded wait, risking indefinite blocking;
change it to obtain connections with a bounded timeout by either (A) creating
and using a dedicated small HikariDataSource (e.g., healthDataSource) for the
health check with a short pool size and connectionTimeout set via
HikariConfig.setConnectionTimeout, then use that
healthDataSource.getConnection() inside checkMySQLHealth/performHealthCheck, or
(B) if you must use the existing dataSource, cast it to HikariDataSource and
ensure HikariConfig.connectionTimeout is configured to a short value before
acquiring the connection; also keep / document the combined timeout budget for
isValid(2) and the query timeout(3) when returning HealthCheckResult.
---
Nitpick comments:
In
`@src/main/java/com/iemr/common/bengen/controller/health/HealthController.java`:
- Around line 42-55: The health check method checkHealth currently logs every
invocation at INFO level causing noise; change the logger.info call in
HealthController.checkHealth to logger.debug so routine probes (which call
isUserAuthenticated and healthService.checkHealth) are only recorded at debug
level while keeping the existing debug log for the status; no other behavior
changes required.
- Around line 57-68: The error response in HealthController's exception block
uses Map.of(...) and omits the "components" field present in normal responses;
change the fallback to return the same response shape by constructing a mutable
map (e.g., new LinkedHashMap<>() or HashMap) and populate "status" = "DOWN",
"timestamp" = Instant.now().toString(), "components" = Collections.emptyMap(),
and optionally include an "error" message; replace the Map.of(...) with this map
construction in the catch block so clients always receive a consistent response
shape.
In
`@src/main/java/com/iemr/common/bengen/controller/version/VersionController.java`:
- Around line 69-78: The loadGitProperties method silently returns empty
Properties when getResourceAsStream("git.properties") returns null; update
loadGitProperties to log a debug/warn message in that branch so missing
build-time git.properties is visible (e.g., use the class logger to log that
"git.properties" was not found on the classpath before returning properties).
Keep the existing behavior of returning empty Properties but add the log
statement in the input == null branch and reference the getResourceAsStream call
and git.properties to make the intent clear.
In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java`:
- Around line 84-88: The health endpoint currently sets the component "type" and
"responseTimeMs" outside the includeDetails guard in HealthService, leaking
backend types to unauthenticated callers; update the logic so that assignments
populating the details map (including the "type" value currently set near Line
82/114 and any "responseTimeMs" entry) are only added when includeDetails is
true (i.e., move the code that puts "type" and responseTimeMs into the same if
(includeDetails) block that already uses
extractHost/extractPort/extractDatabaseName and the details map), or
alternatively omit/mask those fields when includeDetails is false.
- Around line 207-261: The current manual parsing in extractHost, extractPort,
and extractDatabaseName is fragile (fails for credentials, non‑MySQL forms,
etc.); replace the string-splitting logic in these methods to use java.net.URI:
strip the "jdbc:" prefix (or the "jdbc:mysql://" scheme), create a URI from the
remainder, then use URI.getHost(), getPort() (with default 3306 when -1), and
URI.getPath() (trim leading '/' and drop any query portion) to determine host,
port, and DB name respectively; preserve existing null/UNKNOWN_VALUE checks and
logger debug fallbacks so any URI parsing exception returns UNKNOWN_VALUE (or
"3306" for port) as before.
In `@src/main/java/com/iemr/common/bengen/utils/JwtUserIdValidationFilter.java`:
- Around line 3-11: Remove the duplicate import of
org.springframework.stereotype.Component in JwtUserIdValidationFilter: keep a
single import statement for Component (remove the redundant one) and run an
import cleanup/organize step so there are no duplicate or unused imports; locate
the import lines near the top of the JwtUserIdValidationFilter class to make the
change.
- Around line 116-123: JwtUserIdValidationFilter currently uses
path.startsWith(contextPath + "/public") while other checks use equals(), which
can unintentionally skip JWT validation for any subpaths; either change the
check to path.equals(contextPath + "/public") to only exempt the exact /public
endpoint, or if every subpath under /public should be unauthenticated, add a
clear comment and unit test documenting that intent and leave
path.startsWith(...) in place; update the condition in the filter (the block
that calls filterChain.doFilter(servletRequest, servletResponse)) and adjust any
related tests or documentation accordingly.
src/main/java/com/iemr/common/bengen/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/bengen/service/health/HealthService.java (2)
269-303: Duplicated JDBC URL parsing logic betweenextractHostandextractPort.Both methods perform identical prefix removal and
hostPortextraction. Extract a shared helper to reduce duplication.Proposed refactor
+ private String extractHostPort(String jdbcUrl) { + String withoutPrefix = jdbcUrl.replaceFirst("jdbc:mysql://", ""); + int slashIndex = withoutPrefix.indexOf('/'); + return slashIndex > 0 ? withoutPrefix.substring(0, slashIndex) : withoutPrefix; + } + private String extractHost(String jdbcUrl) { if (jdbcUrl == null || UNKNOWN_VALUE.equals(jdbcUrl)) { return UNKNOWN_VALUE; } try { - String withoutPrefix = jdbcUrl.replaceFirst("jdbc:mysql://", ""); - int slashIndex = withoutPrefix.indexOf('/'); - String hostPort = slashIndex > 0 - ? withoutPrefix.substring(0, slashIndex) - : withoutPrefix; + String hostPort = extractHostPort(jdbcUrl); int colonIndex = hostPort.indexOf(':'); return colonIndex > 0 ? hostPort.substring(0, colonIndex) : hostPort; } catch (Exception e) {Apply similarly to
extractPort.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` around lines 269 - 303, extractHost and extractPort duplicate the same JDBC parsing logic; create a single private helper (e.g., getHostPortFromJdbcUrl or parseHostPort) that takes jdbcUrl and returns the hostPort string (or Optional<String>) after removing the "jdbc:mysql://" prefix and trimming at the first '/' and reuse it from extractHost and extractPort; keep the existing behavior for null/UNKNOWN_VALUE and exception handling using UNKNOWN_VALUE, default port "3306", and logger.debug, and have extractHost split hostPort on ':' to return host and extractPort return the port (or default "3306" when no colon).
38-42: Single-thread executor can cause head-of-line blocking for non-Hikari datasources.If the
dataSource.getConnection()call on line 159 blocks beyond the 2-second timeout, theFuturetimes out but the underlying task keeps running on the sole executor thread. All subsequent health checks will queue behind it until that task completes (or never, if it's truly stuck).For robustness, consider using a cached/bounded thread pool, or creating a fresh virtual thread per check (Java 21), so a stuck task doesn't block future checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` around lines 38 - 42, The single-thread executor (executorService created with Executors.newSingleThreadExecutor) can cause head-of-line blocking if a task (e.g., the health check that calls dataSource.getConnection()) stalls; replace it with a non-blocking executor such as a cached/bounded thread pool (Executors.newCachedThreadPool or a fixed pool with a sane max) or, if on Java 21+, use Executors.newVirtualThreadPerTaskExecutor() so each health check runs in its own thread and a stuck dataSource.getConnection() cannot block subsequent checks; update any shutdown logic that references executorService accordingly.
🤖 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/bengen/service/health/HealthService.java`:
- Around line 183-192: The Redis health check in performHealthCheck that calls
redisTemplate.execute(...) (returning pong and using getRedisVersion and
HealthCheckResult) needs a bounded timeout like the MySQL path; wrap the Redis
ping logic in a CompletableFuture (or executor) and block with a timeout (e.g.,
2–3 seconds) so a hung TCP/command doesn't block the health endpoint, and on
timeout return a failing HealthCheckResult with an explanatory message;
alternatively ensure the Redis client/connection uses a configured
command/connection timeout (spring.redis.timeout) and fall back to the
CompletableFuture approach if the client-level timeout is not present.
- Around line 243-253: The getMySQLVersion method uses DB_VERSION_QUERY without
setting a query timeout, so a hanging version query can block health checks;
update getMySQLVersion to call stmt.setQueryTimeout(3) (or reuse the same
timeout constant used by the health check) immediately after preparing the
statement in getMySQLVersion, and handle any SQLTimeoutException similarly to
the existing catch path so the method returns null on timeout; reference
getMySQLVersion, DB_VERSION_QUERY, and stmt.setQueryTimeout to locate and modify
the code.
- Around line 148-152: The current catch in HealthService around
hikariDs.getConnection() indiscriminately wraps all SQLExceptions as
TimeoutException; change it to detect SQLTransientConnectionException (thrown by
Hikari on pool exhaustion) and only convert that to TimeoutException, while
letting other SQLExceptions propagate (or wrap them in a more accurate
exception) so callers (the code that logs "connection pool may be exhausted")
receive correct diagnostics; update the catch logic around
hikariDs.getConnection() to specifically catch SQLTransientConnectionException
-> throw TimeoutException, and catch/throw or rethrow other SQLException types
unchanged (or wrap in a generic/credential failure exception) so issues like
authentication failures are not misreported.
---
Nitpick comments:
In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java`:
- Around line 269-303: extractHost and extractPort duplicate the same JDBC
parsing logic; create a single private helper (e.g., getHostPortFromJdbcUrl or
parseHostPort) that takes jdbcUrl and returns the hostPort string (or
Optional<String>) after removing the "jdbc:mysql://" prefix and trimming at the
first '/' and reuse it from extractHost and extractPort; keep the existing
behavior for null/UNKNOWN_VALUE and exception handling using UNKNOWN_VALUE,
default port "3306", and logger.debug, and have extractHost split hostPort on
':' to return host and extractPort return the port (or default "3306" when no
colon).
- Around line 38-42: The single-thread executor (executorService created with
Executors.newSingleThreadExecutor) can cause head-of-line blocking if a task
(e.g., the health check that calls dataSource.getConnection()) stalls; replace
it with a non-blocking executor such as a cached/bounded thread pool
(Executors.newCachedThreadPool or a fixed pool with a sane max) or, if on Java
21+, use Executors.newVirtualThreadPerTaskExecutor() so each health check runs
in its own thread and a stuck dataSource.getConnection() cannot block subsequent
checks; update any shutdown logic that references executorService accordingly.
src/main/java/com/iemr/common/bengen/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/bengen/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/bengen/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/bengen/controller/health/HealthController.java (2)
39-47:logger.infoat the entry point will flood logs when polled by load balancers or monitoring.Line 39 uses
INFOlevel while the corresponding completion log on line 47 correctly usesDEBUG. Downgrades entry toDEBUGto keep both consistent and prevent log saturation in production environments where this endpoint may be called every few seconds.♻️ Proposed fix
- logger.info("Health check endpoint called"); + logger.debug("Health check endpoint called");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/controller/health/HealthController.java` around lines 39 - 47, In HealthController's health-check entry (the method calling healthService.checkHealth()), change the initial logger.info("Health check endpoint called") to logger.debug(...) so the entry log level matches the completion logger.debug and avoids INFO-level flooding from frequent LB/monitor polls; update the log message text if needed but keep the same context and variable placeholders so both entry and completion use DEBUG consistently.
42-45: Unchecked cast onhealthStatus.get("status")is fragile.If
HealthService.checkHealth()ever returns a non-Stringvalue under the"status"key (e.g., an enum or nested map), line 43 throws aClassCastExceptionthat is silently absorbed by the outer catch and logged as "Unexpected error during health check" — obscuring the real defect. A safer pattern avoids the cast entirely:♻️ Proposed fix
- String overallStatus = (String) healthStatus.get("status"); + Object rawStatus = healthStatus.get("status"); + String overallStatus = (rawStatus instanceof String s) ? s : 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/bengen/controller/health/HealthController.java` around lines 42 - 45, The code blindly casts healthStatus.get("status") to String (in HealthController where healthStatus = healthService.checkHealth()), which can throw ClassCastException; change it to safely extract and normalize the status without a direct cast — e.g., check the value with instanceof or convert via Objects.toString(..., null), handle nulls/defaults, then derive httpStatus based on the normalized string (the existing overallStatus variable and the ternary to HttpStatus.OK/ SERVICE_UNAVAILABLE should remain but use the safe value).
🤖 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/bengen/controller/health/HealthController.java`:
- Around line 39-47: In HealthController's health-check entry (the method
calling healthService.checkHealth()), change the initial logger.info("Health
check endpoint called") to logger.debug(...) so the entry log level matches the
completion logger.debug and avoids INFO-level flooding from frequent LB/monitor
polls; update the log message text if needed but keep the same context and
variable placeholders so both entry and completion use DEBUG consistently.
- Around line 42-45: The code blindly casts healthStatus.get("status") to String
(in HealthController where healthStatus = healthService.checkHealth()), which
can throw ClassCastException; change it to safely extract and normalize the
status without a direct cast — e.g., check the value with instanceof or convert
via Objects.toString(..., null), handle nulls/defaults, then derive httpStatus
based on the normalized string (the existing overallStatus variable and the
ternary to HttpStatus.OK/ SERVICE_UNAVAILABLE should remain but use the safe
value).
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/bengen/service/health/HealthService.java (3)
81-81:executorService != nullis a redundant guard — it is always assigned in the constructor.♻️ Proposed change
-if (executorService != null && !executorService.isShutdown()) { +if (!executorService.isShutdown()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` at line 81, The null-check on executorService is redundant because executorService is always initialized in the constructor; remove the unnecessary "executorService != null" guard and simply check executorService.isShutdown() (e.g., change the condition to use only !executorService.isShutdown()), keeping the existing shutdown logic intact and ensuring any callers relying on executorService remain unaffected; update references in HealthService where executorService is checked to use the single non-null check and run existing unit tests to validate behavior.
76-76: Consider naming the health-check thread pool for easier diagnosis in thread dumps.Threads created by
Executors.newFixedThreadPool(2)get generic names (pool-N-thread-M). A namedThreadFactorymakes them identifiable in thread dumps and APM traces.♻️ Proposed change
-this.executorService = Executors.newFixedThreadPool(2); +this.executorService = Executors.newFixedThreadPool(2, + r -> { + Thread t = new Thread(r, "health-check-" + Thread.currentThread().getId()); + t.setDaemon(true); + return t; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` at line 76, Replace the anonymous pool created with Executors.newFixedThreadPool(2) in HealthService (the executorService field) with a fixed thread pool that uses a custom ThreadFactory that sets meaningful thread names (e.g., "health-check-%d") so threads are identifiable in dumps and APM; implement the ThreadFactory inline or via a utility (or Guava's ThreadFactoryBuilder) and pass it into Executors.newFixedThreadPool(2, yourNamedThreadFactory) where executorService is assigned.
100-101:ConcurrentHashMapis unnecessary for the per-check status maps;LinkedHashMapis sufficient and preserves insertion order.
Future.get()establishes a happens-before relationship, so concurrent access to these maps is not a concern.ConcurrentHashMapalso loses insertion order, making the JSON output key order non-deterministic.♻️ Proposed change
-Map<String, Object> mysqlStatus = new ConcurrentHashMap<>(); -Map<String, Object> redisStatus = new ConcurrentHashMap<>(); +Map<String, Object> mysqlStatus = new LinkedHashMap<>(); +Map<String, Object> redisStatus = new LinkedHashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` around lines 100 - 101, Replace the unnecessary ConcurrentHashMap usage for the per-check status maps in HealthService by declaring mysqlStatus and redisStatus as LinkedHashMap instances to preserve insertion order and produce deterministic JSON; locate the Map<String, Object> mysqlStatus and Map<String, Object> redisStatus declarations in the HealthService class, change their instantiation to new LinkedHashMap<>(), and add the java.util.LinkedHashMap import (removing any unused ConcurrentHashMap import if present).
🤖 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/bengen/service/health/HealthService.java`:
- Around line 96-148: The checkHealth() method is over the 50-line limit because
the future-wait logic is inline; extract the block that waits on mysqlFuture and
redisFuture (the code that computes maxTimeout, deadlineNs, calls
mysqlFuture.get(...) and redisFuture.get(...), and handles
TimeoutException/InterruptedException/Exception and cancellations) into a
private helper method (e.g., private void waitForHealthFutures(Future<?>
mysqlFuture, Future<?> redisFuture)) and call that from checkHealth();
additionally, reduce file length by moving the two inner static classes
AdvancedCheckResult and HealthCheckResult out of HealthService into separate
package-level files named AdvancedCheckResult.java and HealthCheckResult.java
(preserve package and public/static modifiers as needed) so the original file
drops below 500 lines.
- Around line 388-417: The hasDeadlocks(Connection) method currently reports
historical deadlocks (via SHOW ENGINE INNODB STATUS) and thus produces permanent
false positives; remove hasDeadlocks and any callers that mark MySQL as DEGRADED
(e.g., where checkDeadlocks or isDegraded uses hasDeadlocks) and instead rely on
the existing hasLockWaits() real-time check; alternatively, if you want to keep
deadlock counters, replace hasDeadlocks with a delta-based check that queries
performance_schema.events_errors_summary_global_by_error for
ERROR_NAME='ER_LOCK_DEADLOCK' and compares the numeric counter across two
samples (store previous value in a field like lastDeadlockCount and compute
current - last > 0 to indicate a new deadlock), handling permission errors the
same way deadlockCheckDisabled currently does.
- Line 186: The line in HealthService where you call redisTemplate.execute(...)
is too long due to the fully-qualified RedisCallback cast; import
org.springframework.data.redis.core.RedisCallback and refactor the call by
extracting the callback into a separately-typed variable (e.g.,
RedisCallback<String> pingCallback = connection -> connection.ping();) and then
call redisTemplate.execute(pingCallback) to keep the line under 120 chars and
preserve the same behavior for variable pong.
---
Duplicate comments:
In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java`:
- Around line 180-198: The Redis ping call in checkRedisHealthSync() can block
indefinitely; wrap the redisTemplate.execute(...) call in an asynchronous task
with an explicit deadline (e.g., CompletableFuture.supplyAsync(...) and then
get(timeout, TimeUnit.SECONDS) or use orTimeout/completeOnTimeout) so the call
is cancelled and a TimeoutException is handled; on timeout cancel the future,
log the timeout, and return a HealthCheckResult(false, "Redis ping timed out",
false). Keep the existing success/failure checks (checking for "PONG") and
exception handling for other errors around the same code path
(checkRedisHealthSync, redisTemplate.execute, HealthCheckResult).
- Around line 158-178: checkMySQLHealthSync currently calls
dataSource.getConnection() directly which can block indefinitely if the pool is
exhausted; wrap the connection acquisition in a timed task so it fails fast
instead of blocking the health endpoint. Replace the direct call to
dataSource.getConnection() in checkMySQLHealthSync with an
ExecutorService/Future that attempts dataSource.getConnection() and use
Future.get(MYSQL_TIMEOUT_SECONDS, TimeUnit.SECONDS) (or equivalent timeout),
handle TimeoutException by returning a failed/degraded HealthCheckResult, cancel
the future and close any obtained Connection if it arrives late, and then
proceed to use the connection for the existing logic (including
performAdvancedMySQLChecksWithThrottle) only when obtained within the timeout.
---
Nitpick comments:
In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java`:
- Line 81: The null-check on executorService is redundant because
executorService is always initialized in the constructor; remove the unnecessary
"executorService != null" guard and simply check executorService.isShutdown()
(e.g., change the condition to use only !executorService.isShutdown()), keeping
the existing shutdown logic intact and ensuring any callers relying on
executorService remain unaffected; update references in HealthService where
executorService is checked to use the single non-null check and run existing
unit tests to validate behavior.
- Line 76: Replace the anonymous pool created with
Executors.newFixedThreadPool(2) in HealthService (the executorService field)
with a fixed thread pool that uses a custom ThreadFactory that sets meaningful
thread names (e.g., "health-check-%d") so threads are identifiable in dumps and
APM; implement the ThreadFactory inline or via a utility (or Guava's
ThreadFactoryBuilder) and pass it into Executors.newFixedThreadPool(2,
yourNamedThreadFactory) where executorService is assigned.
- Around line 100-101: Replace the unnecessary ConcurrentHashMap usage for the
per-check status maps in HealthService by declaring mysqlStatus and redisStatus
as LinkedHashMap instances to preserve insertion order and produce deterministic
JSON; locate the Map<String, Object> mysqlStatus and Map<String, Object>
redisStatus declarations in the HealthService class, change their instantiation
to new LinkedHashMap<>(), and add the java.util.LinkedHashMap import (removing
any unused ConcurrentHashMap import if present).
src/main/java/com/iemr/common/bengen/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/bengen/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/bengen/service/health/HealthService.java (2)
72-72: Thread pool is oversized — only 2 concurrent tasks are ever submitted.
checkHealth()submits exactly two futures (MySQL + Redis). A pool of 6 wastes resources and leaves 4 threads permanently idle. A pool of 2 (or a dedicatedThreadFactorywith meaningful names for diagnostics) is sufficient.♻️ Proposed fix
- this.executorService = Executors.newFixedThreadPool(6); + this.executorService = Executors.newFixedThreadPool(2, + r -> { + Thread t = new Thread(r, "health-check-worker"); + t.setDaemon(true); + return t; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` at line 72, The executorService in HealthService is oversized for checkHealth(), which only submits two concurrent tasks; change the executor creation (the this.executorService = Executors.newFixedThreadPool(6) site) to use a pool of 2 threads (Executors.newFixedThreadPool(2)) and optionally supply a ThreadFactory to give threads meaningful names for diagnostics; update the HealthService.executorService initialization accordingly and ensure any existing shutdown logic still works with the new pool size.
65-66:ADVANCED_HEALTH_CHECKS_ENABLEDis a dead constant that creates unreachable code.The constant is hardcoded to
true, makingif (!ADVANCED_HEALTH_CHECKS_ENABLED)at line 295 permanently unreachable. If there's no intention to make this configurable (e.g., via@Value), remove the constant and the guard entirely to reduce dead code noise.♻️ Proposed fix
- // Advanced checks always enabled - private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true;private boolean performAdvancedMySQLChecksWithThrottle() { - if (!ADVANCED_HEALTH_CHECKS_ENABLED) { - return false; // Advanced checks disabled - } - long currentTime = System.currentTimeMillis();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java` around lines 65 - 66, ADVANCED_HEALTH_CHECKS_ENABLED is a hardcoded true constant causing unreachable code; remove the constant declaration (ADVANCED_HEALTH_CHECKS_ENABLED) from HealthService and delete the corresponding unreachable guard "if (!ADVANCED_HEALTH_CHECKS_ENABLED)" and its else/early-return branch where it's used (around the advanced checks invocation) OR, if you intend configurability, replace the constant with a configurable field (e.g., a boolean injected via `@Value` or `@ConfigurationProperties`) and use that field instead so the guard becomes reachable. Ensure any dependent logic that assumed the guard is adjusted accordingly.
🤖 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/bengen/service/health/HealthService.java`:
- Around line 314-337: Currently the write lock advancedCheckLock.writeLock() is
held while performing blocking DB I/O (dataSource.getConnection() and
performAdvancedMySQLChecks), causing contention and swallowing thread
interrupts; move the DB I/O outside the write lock: first acquire the read lock
to check the cache (cachedAdvancedCheckResult and lastAdvancedCheckTime),
release it and then perform dataSource.getConnection() and
performAdvancedMySQLChecks(conn) without holding any lock, and only re-acquire
the write lock to atomically update lastAdvancedCheckTime and
cachedAdvancedCheckResult (double-check the cache/state again after obtaining
the write lock to avoid races). Additionally, when catching exceptions from
connection acquisition, detect and restore the interrupt status (e.g., if
SQLException indicates "Interrupted during connection acquisition" or if
InterruptedException is observed) by calling Thread.currentThread().interrupt()
before creating a degraded AdvancedCheckResult so the executor interrupt is not
lost.
---
Duplicate comments:
In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java`:
- Around line 157-158: The checkMySQLHealthSync method currently calls
dataSource.getConnection() without a bounded timeout which allows HikariCP
acquisition to be interrupted and lose the thread interrupt flag; fix by either
(A) creating and using a dedicated small HikariDataSource for health checks and
calling its getConnection(), or (B) configure the shared HikariDataSource used
by this service with a short connectionTimeout before calling
dataSource.getConnection(); also ensure any SQLException that wraps
InterruptedException restores the thread interrupt status
(Thread.currentThread().interrupt()) in the catch block handling exceptions from
checkMySQLHealthSync so executor threads do not lose their interrupt flag.
---
Nitpick comments:
In `@src/main/java/com/iemr/common/bengen/service/health/HealthService.java`:
- Line 72: The executorService in HealthService is oversized for checkHealth(),
which only submits two concurrent tasks; change the executor creation (the
this.executorService = Executors.newFixedThreadPool(6) site) to use a pool of 2
threads (Executors.newFixedThreadPool(2)) and optionally supply a ThreadFactory
to give threads meaningful names for diagnostics; update the
HealthService.executorService initialization accordingly and ensure any existing
shutdown logic still works with the new pool size.
- Around line 65-66: ADVANCED_HEALTH_CHECKS_ENABLED is a hardcoded true constant
causing unreachable code; remove the constant declaration
(ADVANCED_HEALTH_CHECKS_ENABLED) from HealthService and delete the corresponding
unreachable guard "if (!ADVANCED_HEALTH_CHECKS_ENABLED)" and its
else/early-return branch where it's used (around the advanced checks invocation)
OR, if you intend configurability, replace the constant with a configurable
field (e.g., a boolean injected via `@Value` or `@ConfigurationProperties`) and use
that field instead so the guard becomes reachable. Ensure any dependent logic
that assumed the guard is adjusted accordingly.
|
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 BeneficiaryID-Generation-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Improvements
Chores