feat(health,version): add health and version endpoints#136
Open
DurgaPrasad-54 wants to merge 9 commits intoPSMRI:mainfrom
Open
feat(health,version): add health and version endpoints#136DurgaPrasad-54 wants to merge 9 commits intoPSMRI:mainfrom
DurgaPrasad-54 wants to merge 9 commits intoPSMRI:mainfrom
Conversation
Contributor
|
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 git metadata generation to the Maven build, adds /version and /health REST endpoints, implements a concurrent HealthService checking MySQL and optional Redis, and updates the JWT filter to skip authentication for Changes
Sequence DiagramssequenceDiagram
participant Client
participant JwtFilter as JwtUserIdValidationFilter
participant HealthController
participant HealthService
participant MySQL
participant Redis
Client->>JwtFilter: GET /health
JwtFilter->>JwtFilter: path startsWith /health? -> skip auth
alt skipped
JwtFilter-->>HealthController: forward request
HealthController->>HealthService: checkHealth()
par MySQL check
HealthService->>MySQL: SELECT 1 (with timeout)
MySQL-->>HealthService: result / error
and Redis check
HealthService->>Redis: PING (if configured)
Redis-->>HealthService: PONG / error
end
HealthService-->>HealthController: aggregated health map
alt overall UP
HealthController-->>Client: 200 OK + health map
else
HealthController-->>Client: 503 Service Unavailable + health map
end
end
sequenceDiagram
participant Client
participant JwtFilter as JwtUserIdValidationFilter
participant VersionController
participant ClasspathResource as git.properties
Client->>JwtFilter: GET /version
JwtFilter->>JwtFilter: path startsWith /version? -> skip auth
JwtFilter-->>VersionController: forward request
VersionController->>ClasspathResource: load git.properties
alt properties found
ClasspathResource-->>VersionController: properties
else
ClasspathResource-->>VersionController: none
end
VersionController-->>Client: 200 OK + {buildTimestamp, version, branch, commitHash}
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java (1)
158-165:⚠️ Potential issue | 🟡 Minor
startsWithis too broad — paths like/healthcheckor/version-adminwould bypass auth.
path.startsWith(contextPath + "/version")matches not only/versionand/version/...but also/versionXYZ,/version-admin, etc. If any such path is ever added, it would silently skip JWT validation. Consider requiring an exact match or a path-segment boundary:🔒 Proposed fix: tighten the path matching
private boolean shouldSkipPath(String path, String contextPath) { + String relativePath = path.substring(contextPath.length()); return path.equals(contextPath + "/user/userAuthenticate") || path.equalsIgnoreCase(contextPath + "/user/logOutUserFromConcurrentSession") || path.startsWith(contextPath + "/swagger-ui") || path.startsWith(contextPath + "/v3/api-docs") - || path.startsWith(contextPath + "/public") - || path.startsWith(contextPath + "/version") - || path.startsWith(contextPath + "/health"); + || path.startsWith(contextPath + "/public/") + || relativePath.equals("/version") + || relativePath.equals("/health"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java` around lines 158 - 165, The shouldSkipPath method currently uses startsWith for entries like contextPath + "/version" and "/health" which erroneously matches partial segments (e.g., "/version-admin"); update shouldSkipPath to only skip exact segment matches or a segment prefix followed by a '/' (i.e., match contextPath + "/version" OR contextPath + "/version/" and same for "/health") instead of raw startsWith; locate and change the checks in method shouldSkipPath to use exact equals or a boundary-aware check for the listed symbols (userAuthenticate, logOutUserFromConcurrentSession, swagger-ui, v3/api-docs, public, version, health) so only intended paths bypass JWT validation.
🧹 Nitpick comments (6)
src/main/java/com/wipro/fhir/controller/version/VersionController.java (2)
49-67:git.propertiesis re-loaded from the classpath on every request — consider caching.Build metadata is immutable at runtime. Loading and parsing the properties file on each invocation is wasteful, especially if monitoring systems poll this endpoint frequently. Load it once at construction or lazily cache it.
♻️ Proposed refactor: 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"); - return ResponseEntity.ok(response); + return ResponseEntity.ok(versionInfo); + } + + 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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/controller/version/VersionController.java` around lines 49 - 67, The versionInformation() method currently calls loadGitProperties() on every request; change VersionController to cache the git Properties (or the resulting Map) instead: load git.properties once either in the controller constructor or in a private final field initialized lazily and thread-safely, store UNKNOWN_VALUE fallbacks into that cached Map/Properties, and then have versionInformation() read from the cached object instead of calling loadGitProperties() per request; ensure thread-safety and preserve existing keys (buildTimestamp, version, branch, commitHash) and behavior when load fails.
40-41: Missing@CrossOriginannotation.Other controllers in the codebase consistently include
@CrossOriginwithout parameters. This controller should follow the same convention to avoid CORS issues for browser-based consumers.+@CrossOrigin `@RestController` public class VersionController {Based on learnings: "In the Java Spring Boot project, all controllers use the
CrossOriginannotation without parameters to maintain consistency across the codebase."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/controller/version/VersionController.java` around lines 40 - 41, Add the missing CrossOrigin annotation to the VersionController class to match other controllers: import org.springframework.web.bind.annotation.CrossOrigin and annotate the VersionController class with `@CrossOrigin` (placed directly above the class declaration) so it follows the same CORS convention as the rest of the codebase.src/main/java/com/wipro/fhir/controller/health/HealthController.java (2)
22-25: Missing@CrossOriginannotation.Same as
VersionController— add@CrossOriginfor consistency with other controllers.+@CrossOrigin `@RestController` `@RequestMapping`("/health")Based on learnings: "In the Java Spring Boot project, all controllers use the
CrossOriginannotation without parameters to maintain consistency across the codebase."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/controller/health/HealthController.java` around lines 22 - 25, HealthController is missing the class-level `@CrossOrigin` annotation used across controllers; add the `@CrossOrigin` annotation to the HealthController class declaration (the class named HealthController) so it matches VersionController and other controllers, placing it alongside `@RestController` and `@RequestMapping` annotations to maintain consistency.
72-117: Duplicated JWT extraction logic fromJwtUserIdValidationFilter.
isUserAuthenticatedandgetJwtTokenFromCookiesreplicate token-resolution logic from the filter. This creates a maintenance risk — if token extraction logic changes (e.g., new token sources, header name changes), both locations must be updated in lockstep.Additionally, there's a subtle inconsistency: the filter's
getJwtTokenFromCookiesusescookie.getName().equals("Jwttoken")(case-sensitive), while this controller usescookie.getName().equalsIgnoreCase("Jwttoken").Consider extracting a shared utility method (e.g., into
JwtAuthenticationUtil) for resolving a JWT from a request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/controller/health/HealthController.java` around lines 72 - 117, Replace the duplicated token extraction in HealthController.isUserAuthenticated and remove the local getJwtTokenFromCookies; instead add a single shared resolver on JwtAuthenticationUtil (e.g., JwtAuthenticationUtil.resolveToken(HttpServletRequest)) and call that from isUserAuthenticated, then pass the resolved token to jwtAuthenticationUtil.validateUserIdAndJwtToken(token). Ensure the shared resolver matches the filter's cookie-name behavior (use the exact "Jwttoken" match as in JwtUserIdValidationFilter) so cookie matching is consistent across JwtUserIdValidationFilter and HealthController.src/main/java/com/wipro/fhir/service/health/HealthService.java (2)
243-297: JDBC URL parsing is fragile — only handlesjdbc:mysql://scheme.The
extractHost,extractPort, andextractDatabaseNamehelpers hard-code thejdbc:mysql://prefix. If the datasource URL uses a different scheme (e.g.,jdbc:mariadb://, connection pooling wrappers, or URL parameters before the host), parsing will return incorrect results or"unknown". Since the fallback is"unknown", this is non-breaking but worth a comment in the code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/service/health/HealthService.java` around lines 243 - 297, The helpers extractHost, extractPort and extractDatabaseName are fragile because they hard-code "jdbc:mysql://" — update these methods (extractHost, extractPort, extractDatabaseName) to robustly strip any "jdbc:" prefix and scheme (e.g., jdbc:mysql, jdbc:mariadb, or other wrappers) and then parse the remaining URI part (or use a regex/URI parser) to locate host, optional port and database path; also handle optional credentials, query parameters and connection-pool prefixes by trimming leading wrapper tokens before parsing, preserve current fallbacks ("unknown"/"3306") and keep the existing exception logging (logger.debug) behavior.
56-81: Health checks run sequentially; consider parallelizing MySQL and Redis checks.Both
checkMySQLHealthandcheckRedisHealthinvolve I/O with timeouts. Running them in parallel using the executor service would reduce the worst-case response time from the sum of both timeouts to the max of either.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/service/health/HealthService.java` around lines 56 - 81, The health check currently calls checkMySQLHealth and (optionally) checkRedisHealth sequentially inside checkHealth; change checkHealth to submit these checks as Callable tasks to the existing ExecutorService (use submit) so MySQL and Redis run in parallel, collect their Future results (handle ExecutionException/TimeoutException) and then merge results into components and compute overallHealth; guard submission for Redis behind the existing redisTemplate != null check, ensure timeouts/future.get are used appropriately and that any exceptions are logged and converted into a failed component result so isHealthy(mysqlStatus)/isHealthy(redisStatus) still works.
🤖 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 459-484: The git-commit-id-maven-plugin is pinned to an older
version (7.0.0); update the <version> value for the plugin with artifactId
git-commit-id-maven-plugin to the latest stable release (e.g., 9.0.2) in the
pom.xml, then run a build to verify there are no breaking configuration changes
(check the plugin's release notes if needed) and adjust any deprecated
configuration keys if the updated plugin reports warnings or errors.
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 87-95: The health check currently leaks MySQL details when
includeDetails is true; update checkMySQLHealth (and similarly checkRedisHealth)
so sensitive fields (host, port, database, server versions) are only populated
when the caller is authorized (e.g., has an ADMIN role) rather than any valid
JWT. Implement a role check before using includeDetails (or replace
includeDetails with an isAdmin boolean passed into
checkMySQLHealth/checkRedisHealth), and only call extractHost, extractPort,
extractDatabaseName and add version info when that admin check passes; otherwise
omit or redact those fields. Ensure the unique symbols
mentioned—checkMySQLHealth, includeDetails, extractHost, extractPort,
extractDatabaseName—are updated accordingly and mirrored for Redis health logic.
- Line 36: HealthService currently creates a static ExecutorService via
Executors.newFixedThreadPool(4) (executorService) that is never shut down,
causing thread/classloader leaks on WAR redeploy; replace this with a
Spring-managed TaskExecutor (injecting a bean) or add a lifecycle shutdown
method: remove the static executorService or stop using
Executors.newFixedThreadPool(4) directly and either (a) inject a
TaskExecutor/ThreadPoolTaskExecutor into HealthService and use that for async
tasks, or (b) keep the executor but add a `@PreDestroy-annotated` method (e.g.,
shutdownExecutor) that calls executorService.shutdownNow() / shutdown() and
awaits termination to ensure threads are terminated during application undeploy.
---
Outside diff comments:
In `@src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java`:
- Around line 158-165: The shouldSkipPath method currently uses startsWith for
entries like contextPath + "/version" and "/health" which erroneously matches
partial segments (e.g., "/version-admin"); update shouldSkipPath to only skip
exact segment matches or a segment prefix followed by a '/' (i.e., match
contextPath + "/version" OR contextPath + "/version/" and same for "/health")
instead of raw startsWith; locate and change the checks in method shouldSkipPath
to use exact equals or a boundary-aware check for the listed symbols
(userAuthenticate, logOutUserFromConcurrentSession, swagger-ui, v3/api-docs,
public, version, health) so only intended paths bypass JWT validation.
---
Nitpick comments:
In `@src/main/java/com/wipro/fhir/controller/health/HealthController.java`:
- Around line 22-25: HealthController is missing the class-level `@CrossOrigin`
annotation used across controllers; add the `@CrossOrigin` annotation to the
HealthController class declaration (the class named HealthController) so it
matches VersionController and other controllers, placing it alongside
`@RestController` and `@RequestMapping` annotations to maintain consistency.
- Around line 72-117: Replace the duplicated token extraction in
HealthController.isUserAuthenticated and remove the local
getJwtTokenFromCookies; instead add a single shared resolver on
JwtAuthenticationUtil (e.g.,
JwtAuthenticationUtil.resolveToken(HttpServletRequest)) and call that from
isUserAuthenticated, then pass the resolved token to
jwtAuthenticationUtil.validateUserIdAndJwtToken(token). Ensure the shared
resolver matches the filter's cookie-name behavior (use the exact "Jwttoken"
match as in JwtUserIdValidationFilter) so cookie matching is consistent across
JwtUserIdValidationFilter and HealthController.
In `@src/main/java/com/wipro/fhir/controller/version/VersionController.java`:
- Around line 49-67: The versionInformation() method currently calls
loadGitProperties() on every request; change VersionController to cache the git
Properties (or the resulting Map) instead: load git.properties once either in
the controller constructor or in a private final field initialized lazily and
thread-safely, store UNKNOWN_VALUE fallbacks into that cached Map/Properties,
and then have versionInformation() read from the cached object instead of
calling loadGitProperties() per request; ensure thread-safety and preserve
existing keys (buildTimestamp, version, branch, commitHash) and behavior when
load fails.
- Around line 40-41: Add the missing CrossOrigin annotation to the
VersionController class to match other controllers: import
org.springframework.web.bind.annotation.CrossOrigin and annotate the
VersionController class with `@CrossOrigin` (placed directly above the class
declaration) so it follows the same CORS convention as the rest of the codebase.
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 243-297: The helpers extractHost, extractPort and
extractDatabaseName are fragile because they hard-code "jdbc:mysql://" — update
these methods (extractHost, extractPort, extractDatabaseName) to robustly strip
any "jdbc:" prefix and scheme (e.g., jdbc:mysql, jdbc:mariadb, or other
wrappers) and then parse the remaining URI part (or use a regex/URI parser) to
locate host, optional port and database path; also handle optional credentials,
query parameters and connection-pool prefixes by trimming leading wrapper tokens
before parsing, preserve current fallbacks ("unknown"/"3306") and keep the
existing exception logging (logger.debug) behavior.
- Around line 56-81: The health check currently calls checkMySQLHealth and
(optionally) checkRedisHealth sequentially inside checkHealth; change
checkHealth to submit these checks as Callable tasks to the existing
ExecutorService (use submit) so MySQL and Redis run in parallel, collect their
Future results (handle ExecutionException/TimeoutException) and then merge
results into components and compute overallHealth; guard submission for Redis
behind the existing redisTemplate != null check, ensure timeouts/future.get are
used appropriately and that any exceptions are logged and converted into a
failed component result so isHealthy(mysqlStatus)/isHealthy(redisStatus) still
works.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/com/wipro/fhir/controller/health/HealthController.java (1)
39-39: Reduce health-check access log toDEBUGlevel.Health endpoints are typically polled every few seconds by load balancers and monitoring agents. Logging at
INFOon every call will generate excessive noise in production logs. The outcome is already logged atDEBUGon line 48.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/wipro/fhir/controller/health/HealthController.java` at line 39, The access log for the health-check in HealthController should be lowered from INFO to DEBUG to avoid noisy production logs: update the logger call (the statement currently calling logger.info("Health check endpoint called")) to use logger.debug(...) so the endpoint polling only appears at DEBUG level while preserving the existing DEBUG-level outcome logging already present in the HealthController.src/main/java/com/wipro/fhir/service/health/HealthService.java (2)
229-231:performHealthCheckreturn value is always ignored.The method returns
Map<String, Object>but is called viaRunnablelambdas incheckHealth()(lines 139-142), so the return value is discarded. Consider changing the return type tovoidto match actual usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/service/health/HealthService.java` around lines 229 - 231, performHealthCheck currently returns a Map<String,Object> but its return value is never used by the Runnable lambdas in checkHealth(), so change performHealthCheck to return void and update its signature and any internal usage accordingly: modify the method signature (performHealthCheck) to void, remove any return statements that return the status Map, and ensure any callers (the Runnable lambdas in checkHealth) simply invoke performHealthCheck(componentName, status, checker) without expecting a result; keep all existing logic that mutates the provided status Map and uses the HealthCheckResult from the checker intact.
79-79: Unused constantADVANCED_CHECKS_TIMEOUT_MS.This constant is declared but never referenced anywhere in the file. The individual advanced checks use hardcoded
stmt.setQueryTimeout(2)instead. Either remove this constant or wire it into the query timeouts for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/service/health/HealthService.java` at line 79, The constant ADVANCED_CHECKS_TIMEOUT_MS is declared but not used; update the advanced-check SQL statements to use this constant instead of hardcoded values (replace occurrences of stmt.setQueryTimeout(2) with stmt.setQueryTimeout((int)(ADVANCED_CHECKS_TIMEOUT_MS / 1000)) in the methods performing advanced checks) or delete the constant if you prefer the hardcoded timeout; ensure any conversion from milliseconds to seconds is correct and update all places that currently call stmt.setQueryTimeout(2) so they reference ADVANCED_CHECKS_TIMEOUT_MS for consistency.
🤖 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/wipro/fhir/controller/health/HealthController.java`:
- Line 1: Add the same GPL license header used in HealthService.java (lines
1-21) to the top of this file so the header appears before the package
declaration; update
src/main/java/com/wipro/fhir/controller/health/HealthController.java by
inserting the identical license block above "package
com.wipro.fhir.controller.health;" ensuring the class HealthController now
starts with the GPL header for consistency across the codebase.
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 419-448: hasDeadlocks currently returns true for any historical
"LATEST DETECTED DEADLOCK" string in the INNODB STATUS, producing permanent
false positives; update hasDeadlocks(Connection) to, after fetching
innodbStatus, detect the "LATEST DETECTED DEADLOCK" section and extract the
associated timestamp (using a regex that matches common MySQL timestamp formats
found in the status output), parse it into a ZonedDateTime/Instant, compare it
to Instant.now() and only return true if the deadlock timestamp is within a
recent threshold (e.g., 5 minutes); preserve existing permission-error handling
(deadlockCheckDisabled and logger.warn) and ensure any parse failures fall back
to returning false (and log debug) rather than treating the presence of the
section as an implicit recent deadlock.
- Around line 135-164: mysqlStatus and redisStatus are LinkedHashMap instances
that are mutated by runAsync tasks (performHealthCheck via
checkMySQLHealthSync/checkRedisHealthSync) and can be concurrently modified
after CompletableFuture.cancel(true) is called, risking LinkedHashMap
corruption; fix by replacing those maps with thread-safe maps (e.g., use
ConcurrentHashMap for mysqlStatus and redisStatus) or ensure no shared mutation
while futures may still be running (e.g., collect results in thread-safe
structures and only copy into LinkedHashMap after CompletableFuture.allOf
completes or timeouts), and then convert to LinkedHashMap copies when populating
components after ensurePopulated so insertion order is preserved.
---
Nitpick comments:
In `@src/main/java/com/wipro/fhir/controller/health/HealthController.java`:
- Line 39: The access log for the health-check in HealthController should be
lowered from INFO to DEBUG to avoid noisy production logs: update the logger
call (the statement currently calling logger.info("Health check endpoint
called")) to use logger.debug(...) so the endpoint polling only appears at DEBUG
level while preserving the existing DEBUG-level outcome logging already present
in the HealthController.
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 229-231: performHealthCheck currently returns a Map<String,Object>
but its return value is never used by the Runnable lambdas in checkHealth(), so
change performHealthCheck to return void and update its signature and any
internal usage accordingly: modify the method signature (performHealthCheck) to
void, remove any return statements that return the status Map, and ensure any
callers (the Runnable lambdas in checkHealth) simply invoke
performHealthCheck(componentName, status, checker) without expecting a result;
keep all existing logic that mutates the provided status Map and uses the
HealthCheckResult from the checker intact.
- Line 79: The constant ADVANCED_CHECKS_TIMEOUT_MS is declared but not used;
update the advanced-check SQL statements to use this constant instead of
hardcoded values (replace occurrences of stmt.setQueryTimeout(2) with
stmt.setQueryTimeout((int)(ADVANCED_CHECKS_TIMEOUT_MS / 1000)) in the methods
performing advanced checks) or delete the constant if you prefer the hardcoded
timeout; ensure any conversion from milliseconds to seconds is correct and
update all places that currently call stmt.setQueryTimeout(2) so they reference
ADVANCED_CHECKS_TIMEOUT_MS for consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/wipro/fhir/service/health/HealthService.java (1)
295-325: Redundant severity checks incomputeOverallStatus.
SEVERITY_CRITICALis always co-located withSTATUS_DOWN(bothdetermineSeverity()andensurePopulated()write them together), so theSEVERITY_CRITICAL.equals(severity)branch can never fire independently. Likewise forSEVERITY_WARNINGandSTATUS_DEGRADED. The extra checks are dead conditions.♻️ Proposed simplification
- if (STATUS_DOWN.equals(status) || SEVERITY_CRITICAL.equals(severity)) { + if (STATUS_DOWN.equals(status)) { hasCritical = true; } if (STATUS_DEGRADED.equals(status)) { hasDegraded = true; } - if (SEVERITY_WARNING.equals(severity)) { - hasDegraded = true; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/service/health/HealthService.java` around lines 295 - 325, In computeOverallStatus, remove the redundant severity-based branches and rely solely on the component status values: check STATUS_DOWN to set hasCritical and STATUS_DEGRADED to set hasDegraded (remove SEVERITY_CRITICAL.equals(severity) and SEVERITY_WARNING.equals(severity) checks), since determineSeverity()/ensurePopulated() always co-assign severity with status; leave the final return logic (return STATUS_DOWN if hasCritical, STATUS_DEGRADED if hasDegraded, else STATUS_UP) unchanged to simplify and avoid dead conditions.
🤖 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/wipro/fhir/service/health/HealthService.java`:
- Around line 450-466: The hasSlowQueries method currently counts all
server-wide slow queries and can produce false positives on shared DBs; update
the SQL inside hasSlowQueries to filter by the application DB user (add a user =
? or user LIKE ? predicate instead of relying only on NOT IN), make the time
threshold a parameter (avoid hardcoding 10) sourced from configuration (e.g.,
slowQueryThresholdSeconds), and bind both parameters on the PreparedStatement
(setInt for the threshold/time and setString for the app DB user obtained from
your datasource/config). Ensure the PreparedStatement still uses
try-with-resources, remove or adapt the NOT IN for system users if using an
explicit app user filter, and return the same boolean logic (slowQueryCount >
configuredAlertCount) based on a configurable alert threshold.
- Around line 399-417: The hasLockWaits(Connection) method currently queries
INFORMATION_SCHEMA.PROCESSLIST and compares user = USER(), which never matches
because USER() returns username@host; update the PreparedStatement SQL in
hasLockWaits to query performance_schema.processlist (not deprecated
INFORMATION_SCHEMA) and compare the processlist user to SUBSTRING_INDEX(USER(),
'@', 1) so the bare username matches; keep the existing try-with-resources using
PreparedStatement and ResultSet, retain stmt.setQueryTimeout(2) and the
rs.getInt(1) logic, and preserve logging by expanding the catch to include the
exception message via logger.debug with the caught exception to aid debugging.
---
Duplicate comments:
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 419-448: The current hasDeadlocks method falsely reports
historical deadlocks forever; change it to parse a timestamp from the INNODB
status and only treat a detected deadlock as active if its timestamp is recent.
Add a new field (e.g., Instant lastDeadlockDetectedAt) and a configurable
threshold (e.g., Duration deadlockAgeThreshold = Duration.ofMinutes(5)); in
hasDeadlocks (and using deadlockCheckDisabled/logger) extract a timestamp from
the innodbStatus string via a regex (look for common patterns like yyyy-MM-dd
HH:mm:ss or MySQL status timestamp formats), convert to Instant, update
lastDeadlockDetectedAt, and return true only if that instant is within
deadlockAgeThreshold; keep the existing permission-handling branch
(deadlockCheckDisabled) unchanged.
---
Nitpick comments:
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 295-325: In computeOverallStatus, remove the redundant
severity-based branches and rely solely on the component status values: check
STATUS_DOWN to set hasCritical and STATUS_DEGRADED to set hasDegraded (remove
SEVERITY_CRITICAL.equals(severity) and SEVERITY_WARNING.equals(severity)
checks), since determineSeverity()/ensurePopulated() always co-assign severity
with status; leave the final return logic (return STATUS_DOWN if hasCritical,
STATUS_DEGRADED if hasDegraded, else STATUS_UP) unchanged to simplify and avoid
dead conditions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/wipro/fhir/service/health/HealthService.java (1)
101-101:deadlockCheckDisabledis dead code —hasDeadlocks()was removed.The field is declared but never read or written anywhere in the class. It can be safely removed.
♻️ Proposed removal
- // Deadlock check resilience - disable after first permission error - private volatile boolean deadlockCheckDisabled = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wipro/fhir/service/health/HealthService.java` at line 101, Remove the unused volatile boolean field deadlockCheckDisabled from the HealthService class: locate the declaration private volatile boolean deadlockCheckDisabled = false; in HealthService and delete it (no other code references remain since hasDeadlocks() was removed), and ensure no getters/setters or usages (e.g., any methods named hasDeadlocks or setDeadlockCheckDisabled) are left dangling — if such accessor methods exist, remove them as well.
🤖 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/wipro/fhir/service/health/HealthService.java`:
- Around line 211-229: checkMySQLHealthSync currently opens a Connection in a
try-with-resources and passes it into performAdvancedMySQLChecksWithThrottle
which runs tasks on advancedCheckExecutor — that allows the background task to
use a connection that gets closed/returned when the calling method exits; change
performAdvancedMySQLChecksWithThrottle (and the underlying
performAdvancedMySQLChecks/hasLockWaits/hasSlowQueries usage) so it does not
accept or use the caller's Connection: instead have the advanced check task
acquire its own Connection from dataSource inside the Runnable/Callable (use
try-with-resources so the connection is closed there), and update the call site
in checkMySQLHealthSync to stop passing the local connection (or pass dataSource
if needed); keep existing timeout/cancel logic (future.cancel(true)) but ensure
the background task cleans up its own connection to avoid pool corruption.
---
Duplicate comments:
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Around line 446-464: The hasLockWaits method currently queries the deprecated
INFORMATION_SCHEMA.PROCESSLIST; update the SQL to query
performance_schema.processlist instead and keep the same filters (state checks
and user match) so the logic remains unchanged; locate the method
hasLockWaits(Connection) and replace the prepared statement SQL string with an
equivalent against performance_schema.processlist, preserving the
stmt.setQueryTimeout(2), ResultSet handling, and exception logging as-is.
- Around line 467-483: The slow-query check counts server-wide processes and can
return false positives; update hasSlowQueries(Connection) to restrict to the
application DB user by adding a user filter to the SQL (e.g. "AND
SUBSTRING_INDEX(USER,'@',1) = ?") and bind the current connection user as the
second parameter (obtain via connection.getMetaData().getUserName()); keep the
time parameter as the first bind (stmt.setInt(1, 10)) and then call
stmt.setString(2, appUser), adjust parameter order accordingly, and preserve the
existing timeout/result handling in hasSlowQueries.
---
Nitpick comments:
In `@src/main/java/com/wipro/fhir/service/health/HealthService.java`:
- Line 101: Remove the unused volatile boolean field deadlockCheckDisabled from
the HealthService class: locate the declaration private volatile boolean
deadlockCheckDisabled = false; in HealthService and delete it (no other code
references remain since hasDeadlocks() was removed), and ensure no
getters/setters or usages (e.g., any methods named hasDeadlocks or
setDeadlockCheckDisabled) are left dangling — if such accessor methods exist,
remove them as well.
|
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
1 participant
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 FHIR-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit