feat(health,version): add health and version endpoints#136
Open
DurgaPrasad-54 wants to merge 6 commits intoPSMRI:mainfrom
Open
feat(health,version): add health and version endpoints#136DurgaPrasad-54 wants to merge 6 commits intoPSMRI:mainfrom
DurgaPrasad-54 wants to merge 6 commits intoPSMRI:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a /health REST endpoint and HealthService performing MySQL/Redis/Elasticsearch checks, refactors /version to return JSON map, updates Maven git-commit-id plugin and adds Elasticsearch REST client dependency, and skips JWT validation for public /health and /version paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as HealthController
participant Auth as JwtAuthenticationUtil
participant Service as HealthService
participant MySQL as MySQL
participant Redis as Redis
participant ES as Elasticsearch
Client->>Controller: GET /health (token in header/cookie)
Controller->>Auth: validateToken(token)
alt token valid
Auth-->>Controller: valid
else token invalid or absent
Auth-->>Controller: invalid
end
Controller->>Service: checkHealth(isAuthenticated)
par MySQL check
Service->>MySQL: execute health query
MySQL-->>Service: result (+ version)
and Redis check
Service->>Redis: PING (timeout)
Redis-->>Service: PONG (+ version)
and Elasticsearch check
Service->>ES: REST ping / cluster health
ES-->>Service: status
end
Service-->>Controller: aggregated health map
alt overall UP
Controller-->>Client: HTTP 200 + health details
else overall DOWN
Controller-->>Client: HTTP 503 + health details
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/identity/controller/health/HealthController.java (1)
94-127: Token extraction logic is duplicated fromJwtUserIdValidationFilter.
isUserAuthenticatedre-implements JWT extraction from headers and cookies, mirroring logic already present in the filter. Consider extracting a shared utility method (e.g., inJwtAuthenticationUtilor a new helper) to avoid maintaining the same extraction logic in two places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/controller/health/HealthController.java` around lines 94 - 127, The isUserAuthenticated method duplicates JWT extraction logic from JwtUserIdValidationFilter; refactor by adding a shared extractor (e.g., JwtAuthenticationUtil.extractTokenFromRequest(HttpServletRequest)) that encapsulates reading JwtToken header, Authorization Bearer header, and cookies (preserving current precedence), then update HealthController.isUserAuthenticated to call that extractor and then jwtAuthenticationUtil.validateUserIdAndJwtToken(token); also modify JwtUserIdValidationFilter to use the same new extractor so token parsing logic is maintained in one place.src/main/java/com/iemr/common/identity/service/health/HealthService.java (2)
121-154: Health checks run sequentially — consider parallel execution for lower latency.MySQL, Redis, and Elasticsearch checks are independent but invoked sequentially. If any one blocks for its full timeout (2-3 s), the total latency stacks up. Since you already have an
ExecutorService, you could run them concurrently withCompletableFuture.allOfto keep response time bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 121 - 154, The health checks in checkHealth currently run sequentially; modify checkHealth to run checkMySQLHealth, checkRedisHealth (only if redisTemplate!=null) and checkElasticsearchHealth (only if elasticsearchEnabled && elasticsearchClientReady) concurrently using CompletableFuture.supplyAsync with your existing ExecutorService and combine them with CompletableFuture.allOf, then gather each result future (handling exceptions by returning a failing component map) and populate components and overallHealth using isHealthy; ensure timestamp, STATUS_KEY, logging and includeDetails behavior remain the same and that exceptions are logged and translated into component DOWN entries so a hung call doesn't block the whole check.
362-416: JDBC URL parsers are fragile — considerjava.net.URIor a regex.The manual
indexOf/substringparsing only handles thejdbc:mysql://host:port/dbformat. URLs with credentials (user:pass@host), IPv6 addresses, or multiple hosts (failover URLs) will be mis-parsed. UsingURI.create(jdbcUrl.substring(5))(stripping thejdbc:prefix) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java` around lines 362 - 416, The current manual parsing in extractHost, extractPort, and extractDatabaseName is fragile; replace it by stripping the "jdbc:" prefix and using java.net.URI to parse the remaining URI (or URI.create) so you correctly handle credentials, IPv6, and multi-host forms: in extractHost call new URI(withoutJdbc).getHost() and return UNKNOWN_VALUE if null, in extractPort call new URI(...).getPort() and return "3306" when getPort() returns -1, and in extractDatabaseName use new URI(...).getPath() (trim leading '/') and strip any query component; wrap URI creation in try/catch and log debug on failure preserving existing fallback values so behavior remains safe for malformed inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 295-317: Update the git-commit-id-maven-plugin version: in the
plugin block for io.github.git-commit-id:git-commit-id-maven-plugin (the
execution id "get-the-git-infos" and goal "revision"), change the <version> from
7.0.0 to the latest stable release (e.g. 9.0.2) so the plugin benefits from
recent fixes and improvements while keeping the existing configuration elements
like includeOnlyProperties and failOnNoGitDirectory.
- Around line 248-253: The dependency entry for
org.elasticsearch.client:elasticsearch-rest-client is pinned to 8.10.0; update
the <version> to 9.3.0 in the pom and run a full build to surface any API or
transitive dependency changes, then evaluate and migrate usage from the legacy
artifactId (elasticsearch-rest-client) to the new RestClient / Rest5Client API
where applicable (search for code referencing classes from
elasticsearch-rest-client and adapt calls to the RestClient API, updating
imports and client construction accordingly); also run a dependency tree scan
(mvn dependency:tree) and security scan to confirm there are no problematic
transitive CVEs after the upgrade.
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Line 249: The HealthService currently returns a hardcoded Elasticsearch
version string in the HealthCheckResult; update the code in HealthService where
it constructs new HealthCheckResult(...) to either (a) issue a GET "/" (or
another version endpoint) to the Elasticsearch cluster, parse the JSON response
to extract the "version.number" field, and pass that value instead of the
literal "Elasticsearch 8.10.0", or (b) remove the guessed version and pass
null/omit the version like the MySQL/Redis checks; implement the JSON parsing
and error handling where the current return new HealthCheckResult(true,
"Elasticsearch 8.10.0", null) occurs so HealthCheckResult receives the real
version string (or null) instead of the hardcoded value.
- Around line 94-119: The Elasticsearch RestClient opened in
initializeElasticsearchClient (assigned to the elasticsearchRestClient field) is
never closed; add a `@PreDestroy` lifecycle method (e.g., destroy or
closeResources) that checks if elasticsearchRestClient is non-null and open,
calls close() inside a try/catch, logs any errors, and sets
elasticsearchClientReady to false; ensure the method also mirrors the
ExecutorService cleanup pattern used elsewhere so resources are reliably
released on shutdown.
- Line 63: The static ExecutorService created with
Executors.newFixedThreadPool(4) (executorService) is never shut down and will
leak threads on WAR redeploys; change it to an instance field or add a lifecycle
cleanup method that shuts it down and closes related resources: add a
`@PreDestroy-annotated` method (e.g., cleanup) that calls
executorService.shutdownNow() (or orderly shutdown with timeout), handles
InterruptedException, and also closes elasticsearchRestClient with try/catch
logging any IOException to prevent resource leaks on undeploy/redeploy.
- Around line 265-311: performHealthCheck currently exposes raw error messages
into the returned details map; change its signature to accept a boolean
includeDetails and use that flag to avoid placing sensitive error strings into
details (but continue logging full errors internally). Specifically, update
performHealthCheck(componentName, details, checker) ->
performHealthCheck(componentName, details, checker, includeDetails), and when
result.isHealthy is false or an exception is caught, set details.put("error",
includeDetails ? safeErrorOrExceptionMessage : "suppressed") and set
details.put("errorType", includeDetails ? "CheckFailed"/"InternalError" :
"Sensitive"); keep logger.error/logger.warn calls unchanged so full diagnostics
remain in logs but not returned to unauthenticated callers. Ensure you handle
HealthCheckResult.error, e.getCause()/e.getMessage() branches accordingly so no
internal messages are added to details when includeDetails is false.
In `@src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java`:
- Line 89: There is a typo in the log message inside JwtUserIdValidationFilter:
change the logger.info call that currently logs "Common-API incoming userAget:
{}" to correctly spell "userAgent" so it reads "Common-API incoming userAgent:
{}", keeping the same logger invocation and the userAgent variable reference.
- Around line 48-53: In JwtUserIdValidationFilter, fix the logger typo by
renaming "userAget" to "userAgent" and update the public-endpoint matching logic
that currently uses path.endsWith("/health") and path.endsWith("/version"):
replace the permissive suffix checks with stricter checks (e.g., exact equals or
a defined prefix match such as path.equals("/health") / path.equals("/version")
or path.startsWith("/public/") as appropriate) so JWT validation isn't
accidentally bypassed; update the logger.info call that references the path to
use the corrected "userAgent" variable name.
---
Nitpick comments:
In
`@src/main/java/com/iemr/common/identity/controller/health/HealthController.java`:
- Around line 94-127: The isUserAuthenticated method duplicates JWT extraction
logic from JwtUserIdValidationFilter; refactor by adding a shared extractor
(e.g., JwtAuthenticationUtil.extractTokenFromRequest(HttpServletRequest)) that
encapsulates reading JwtToken header, Authorization Bearer header, and cookies
(preserving current precedence), then update
HealthController.isUserAuthenticated to call that extractor and then
jwtAuthenticationUtil.validateUserIdAndJwtToken(token); also modify
JwtUserIdValidationFilter to use the same new extractor so token parsing logic
is maintained in one place.
In `@src/main/java/com/iemr/common/identity/service/health/HealthService.java`:
- Around line 121-154: The health checks in checkHealth currently run
sequentially; modify checkHealth to run checkMySQLHealth, checkRedisHealth (only
if redisTemplate!=null) and checkElasticsearchHealth (only if
elasticsearchEnabled && elasticsearchClientReady) concurrently using
CompletableFuture.supplyAsync with your existing ExecutorService and combine
them with CompletableFuture.allOf, then gather each result future (handling
exceptions by returning a failing component map) and populate components and
overallHealth using isHealthy; ensure timestamp, STATUS_KEY, logging and
includeDetails behavior remain the same and that exceptions are logged and
translated into component DOWN entries so a hung call doesn't block the whole
check.
- Around line 362-416: The current manual parsing in extractHost, extractPort,
and extractDatabaseName is fragile; replace it by stripping the "jdbc:" prefix
and using java.net.URI to parse the remaining URI (or URI.create) so you
correctly handle credentials, IPv6, and multi-host forms: in extractHost call
new URI(withoutJdbc).getHost() and return UNKNOWN_VALUE if null, in extractPort
call new URI(...).getPort() and return "3306" when getPort() returns -1, and in
extractDatabaseName use new URI(...).getPath() (trim leading '/') and strip any
query component; wrap URI creation in try/catch and log debug on failure
preserving existing fallback values so behavior remains safe for malformed
inputs.
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/service/health/HealthService.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pom.xml`:
- Around line 248-253: The pom dependency for
org.elasticsearch.client:elasticsearch-rest-client should be upgraded from
8.10.0 to 9.3.0 and the codebase evaluated for API changes (consider migrating
to the newer Rest5Client API). Update the
<artifactId>org.elasticsearch.client:elasticsearch-rest-client</artifactId>
<version> to 9.3.0 in the pom, run your full build/tests, and search for usages
of classes/methods from RestClient/RestClientBuilder to adapt to breaking
changes (replace with Rest5Client equivalents where needed); fix
compilation/test failures and update any configuration or client instantiation
code accordingly. Ensure compatibility by reading the 9.x migration notes and
adjust serialization/response handling if APIs differ.
|
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 Identity-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Accessibility
Chores