feat(health,version): add health and version endpoints without auth#120
Open
DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
Open
feat(health,version): add health and version endpoints without auth#120DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds /health and /version REST endpoints, a HealthService performing MySQL and Redis checks, a Maven plugin to generate git.properties (duplicated in pom.xml), and updates the JWT filter to bypass authentication for /health and /version. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HealthController
participant JwtAuthenticationUtil
participant HealthService
participant DataSource
participant Redis
Client->>HealthController: GET /health (with/without token)
HealthController->>JwtAuthenticationUtil: validate token (if present)
JwtAuthenticationUtil-->>HealthController: isAuthenticated (true/false)
HealthController->>HealthService: checkHealth(isAuthenticated)
par MySQL check
HealthService->>DataSource: execute simple health query
DataSource-->>HealthService: success / exception (with optional version)
and Redis check
HealthService->>Redis: ping() with 3s timeout
Redis-->>HealthService: PONG / timeout/exception (with optional version)
end
HealthService-->>HealthController: aggregated health map (mysql, redis, overall)
HealthController-->>Client: HTTP 200 (UP) or 503 (DOWN) with payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 4
🧹 Nitpick comments (4)
src/main/java/com/iemr/ecd/service/health/HealthService.java (2)
91-94: Unauthenticated requests still expose host, port, and database name ifincludeDetailsistrue.Looking at the flow:
HealthController.isUserAuthenticated()determinesincludeDetails. ThecheckHealth()no-arg overload (line 83-85) defaults toincludeDetails=true. Ensure this no-arg overload isn't called from any unauthenticated code path, as it would leak infrastructure details (host, port, database name) to anonymous users.Currently only
HealthControllercallscheckHealth(isAuthenticated), so this is safe today, but the no-arg overload is public and could be misused.Consider making the no-arg overload package-private or removing it
- public Map<String, Object> checkHealth() { + // Consider restricting visibility or removing if not needed externally + Map<String, Object> checkHealth() { return checkHealth(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 91 - 94, The no-arg public overload HealthService.checkHealth() currently defaults includeDetails=true and can leak host/port/database; change this public no-arg method to either be removed or made package-private (or protected) so it cannot be called from unauthenticated callers, and update any callers to explicitly call checkHealth(boolean includeDetails) (e.g., HealthController should continue calling checkHealth(isUserAuthenticated())); ensure the sensitive-detail population logic in checkHealth(boolean) (extractHost, extractPort, extractDatabaseName) remains gated by the includeDetails flag.
243-297: JDBC URL parsing is fragile for non-standard URL formats.The
extractHost/extractPort/extractDatabaseNamehelpers assume a simplejdbc:mysql://host:port/db?paramsformat. They won't handle failover URLs (jdbc:mysql://host1:3306,host2:3306/db), URLs with embedded credentials, or other JDBC URL variants. The defensivetry/catchwithUNKNOWN_VALUEfallback makes this safe but potentially misleading.This is acceptable for a health display, but consider documenting the limitation or using
java.net.URIfor more robust parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 243 - 297, The helper methods extractHost, extractPort and extractDatabaseName are fragile for non-standard JDBC strings (failover lists, embedded credentials, etc.); update them to robustly parse JDBC URLs by first stripping the "jdbc:mysql://" prefix, then if the authority portion contains credentials (user@host...) remove the credentials, split on commas to handle failover lists and take the first host:port entry, and then parse host and optional port (fall back to 3306) and database (strip query params) from that entry; alternatively use java.net.URI by replacing the "jdbc:" prefix and handling missing scheme/authority to extract host, port and path safely. Apply these changes in the methods extractHost, extractPort and extractDatabaseName and add a short comment documenting remaining limitations if any.src/main/java/com/iemr/ecd/controller/version/VersionController.java (1)
49-67: Consider loadinggit.propertiesonce at startup instead of per-request.
loadGitProperties()reads from the classpath on every invocation of/version. Since this file is static after build, loading it once (e.g., in the constructor or a@PostConstructmethod) avoids repeated I/O and simplifies the method.♻️ Cache properties at startup
`@RestController` public class VersionController { private final Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); - private static final String UNKNOWN_VALUE = "unknown"; + private final Map<String, String> versionInfo; + + public VersionController() { + this.versionInfo = loadVersionInfo(); + } `@Operation`(summary = "Get version information") `@GetMapping`(value = "/version", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity<Map<String, String>> versionInformation() { - Map<String, String> response = new LinkedHashMap<>(); - try { - logger.info("version Controller Start"); - Properties gitProperties = loadGitProperties(); - response.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); - response.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); - response.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); - response.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); - } catch (Exception e) { - logger.error("Failed to load version information", e); - response.put("buildTimestamp", UNKNOWN_VALUE); - response.put("version", UNKNOWN_VALUE); - response.put("branch", UNKNOWN_VALUE); - response.put("commitHash", UNKNOWN_VALUE); - } - logger.info("version Controller End"); + logger.info("version Controller Start"); + Map<String, String> response = new LinkedHashMap<>(versionInfo); + logger.info("version Controller End"); return ResponseEntity.ok(response); } + private Map<String, String> loadVersionInfo() { + Map<String, String> info = new LinkedHashMap<>(); + try { + Properties gitProperties = loadGitProperties(); + info.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); + info.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); + info.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); + info.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); + } catch (Exception e) { + logger.error("Failed to load version information", e); + info.put("buildTimestamp", UNKNOWN_VALUE); + info.put("version", UNKNOWN_VALUE); + info.put("branch", UNKNOWN_VALUE); + info.put("commitHash", UNKNOWN_VALUE); + } + return info; + } + private Properties loadGitProperties() throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/controller/version/VersionController.java` around lines 49 - 67, The versionInformation() method calls loadGitProperties() on every request; change this by loading and caching the Properties once at startup (e.g., in the class constructor or a `@PostConstruct` method) into a private field like cachedGitProperties, and have versionInformation() read from cachedGitProperties (falling back to UNKNOWN_VALUE if cache is null or missing keys). Ensure loadGitProperties() is invoked only during startup, preserve the existing exception handling (log errors when initializing the cache), and update references in versionInformation() to use cachedGitProperties instead of calling loadGitProperties() per-request.src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java (1)
111-113:startsWithmay unintentionally bypass auth for future sub-paths.Using
path.startsWith(contextPath + "/health")also matches paths like/healthcheck,/health-admin, etc. Since these are leaf endpoints, consider usingpath.equals(...)or matching with a trailing slash as well (path.startsWith(contextPath + "/health/")) to avoid accidentally opening future endpoints.This is consistent with the existing approach (e.g.,
/public,/swagger-ui), but those are intentionally prefix-based, whereas/healthand/versionare single endpoints.Tighter path matching
- || path.startsWith(contextPath + "/health") - || path.startsWith(contextPath + "/version")) { + || path.equals(contextPath + "/health") + || path.equals(contextPath + "/version")) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java` around lines 111 - 113, The current path checks in JwtUserIdValidationFilter use path.startsWith(contextPath + "/health") and path.startsWith(contextPath + "/version"), which may unintentionally match longer endpoints (e.g., /healthcheck); update the conditional to only allow the exact leaf endpoints by replacing those two checks with either path.equals(contextPath + "/health") || path.equals(contextPath + "/health/") (and same for "/version") or use startsWith(contextPath + "/health/") plus an exact equals fallback, so the logic only bypasses auth for the intended /health and /version endpoints while still leaving prefix-based checks (e.g., /public) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 427-456: Remove the four unused git properties from the plugin's
includeOnlyProperties to shrink the generated git.properties: delete entries for
git.commit.message.short, git.commit.user.name, git.dirty, and
git.total.commit.count in the git-commit-id-maven-plugin configuration (the
includeOnlyProperties block). Keep only the properties referenced by
VersionController (git.build.time, git.build.version, git.branch,
git.commit.id.abbrev) so the generated file contains just the values consumed by
VersionController.
In `@src/main/java/com/iemr/ecd/controller/health/HealthController.java`:
- Around line 71-89: isUserAuthenticated currently only reads the
"Authorization" header so requests that supply the JWT via the application's
"Jwttoken" header (Constants.JWT_TOKEN) or cookies will be treated as
unauthenticated; update isUserAuthenticated to first check for a token in
request.getHeader(Constants.JWT_TOKEN) (and if absent fall back to Authorization
handling including "Bearer " prefix) and optionally fall back to the cookie
lookup logic used by JwtUserIdValidationFilter, then pass the resolved token to
jwtAuthenticationUtil.validateUserIdAndJwtToken(token) inside the existing
try/catch (keep the logger.debug on exception).
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Line 36: The static ExecutorService executorService in HealthService is an
unbounded cached thread pool that is never shut down; replace it with a bounded,
Spring-managed executor and ensure lifecycle cleanup: make the field non-static
(remove private static final ExecutorService executorService), inject a
TaskExecutor/Executor (e.g., via a constructor parameter annotated
`@Qualifier`("taskExecutor") or `@Autowired`) or create a fixed-size ExecutorService
with a clear maxThreads, and add a `@PreDestroy` method to call
shutdown/shutdownNow for the ExecutorService if you instantiate it locally;
update any usages in HealthService to use the injected executor field.
- Around line 46-48: The HealthService constructor is using outdated Redis
property names; update the `@Value` annotations for the redis parameters in the
HealthService class to use Spring Boot 3.2 keys: change
`@Value`("${spring.redis.host:localhost}") String redisHost to
`@Value`("${spring.data.redis.host:localhost}") String redisHost and change
`@Value`("${spring.redis.port:6379}") int redisPort to
`@Value`("${spring.data.redis.port:6379}") int redisPort so the health check
reflects configured values; keep dbUrl unchanged.
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/controller/version/VersionController.java`:
- Around line 49-67: The versionInformation() method calls loadGitProperties()
on every request; change this by loading and caching the Properties once at
startup (e.g., in the class constructor or a `@PostConstruct` method) into a
private field like cachedGitProperties, and have versionInformation() read from
cachedGitProperties (falling back to UNKNOWN_VALUE if cache is null or missing
keys). Ensure loadGitProperties() is invoked only during startup, preserve the
existing exception handling (log errors when initializing the cache), and update
references in versionInformation() to use cachedGitProperties instead of calling
loadGitProperties() per-request.
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 91-94: The no-arg public overload HealthService.checkHealth()
currently defaults includeDetails=true and can leak host/port/database; change
this public no-arg method to either be removed or made package-private (or
protected) so it cannot be called from unauthenticated callers, and update any
callers to explicitly call checkHealth(boolean includeDetails) (e.g.,
HealthController should continue calling checkHealth(isUserAuthenticated()));
ensure the sensitive-detail population logic in checkHealth(boolean)
(extractHost, extractPort, extractDatabaseName) remains gated by the
includeDetails flag.
- Around line 243-297: The helper methods extractHost, extractPort and
extractDatabaseName are fragile for non-standard JDBC strings (failover lists,
embedded credentials, etc.); update them to robustly parse JDBC URLs by first
stripping the "jdbc:mysql://" prefix, then if the authority portion contains
credentials (user@host...) remove the credentials, split on commas to handle
failover lists and take the first host:port entry, and then parse host and
optional port (fall back to 3306) and database (strip query params) from that
entry; alternatively use java.net.URI by replacing the "jdbc:" prefix and
handling missing scheme/authority to extract host, port and path safely. Apply
these changes in the methods extractHost, extractPort and extractDatabaseName
and add a short comment documenting remaining limitations if any.
In `@src/main/java/com/iemr/ecd/utils/mapper/JwtUserIdValidationFilter.java`:
- Around line 111-113: The current path checks in JwtUserIdValidationFilter use
path.startsWith(contextPath + "/health") and path.startsWith(contextPath +
"/version"), which may unintentionally match longer endpoints (e.g.,
/healthcheck); update the conditional to only allow the exact leaf endpoints by
replacing those two checks with either path.equals(contextPath + "/health") ||
path.equals(contextPath + "/health/") (and same for "/version") or use
startsWith(contextPath + "/health/") plus an exact equals fallback, so the logic
only bypasses auth for the intended /health and /version endpoints while still
leaving prefix-based checks (e.g., /public) unchanged.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/iemr/ecd/service/health/HealthService.java (3)
299-309: Consider using a Javarecordsince the project targets Java 17.
HealthCheckResultis a simple immutable data carrier — a perfect fit for a record.Suggested refactor
- private static class HealthCheckResult { - final boolean isHealthy; - final String version; - final String error; - - HealthCheckResult(boolean isHealthy, String version, String error) { - this.isHealthy = isHealthy; - this.version = version; - this.error = error; - } - } + private record HealthCheckResult(boolean isHealthy, String version, String error) {}Note: record accessor methods are
isHealthy(),version(),error()— update field access inperformHealthCheckaccordingly (e.g.,result.isHealthy→result.isHealthy()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 299 - 309, Replace the inner immutable class HealthCheckResult with a Java record (e.g., record HealthCheckResult(boolean isHealthy, String version, String error)) to simplify the data carrier; then update all usages in performHealthCheck (and elsewhere) to use record accessor methods (result.isHealthy() / result.version() / result.error()) instead of field access (result.isHealthy, result.version, result.error). Ensure the constructor semantics remain the same and remove the old final fields and explicit constructor.
243-277:extractHostandextractPortduplicate the same URL-parsing preamble.Both methods repeat the
replaceFirst/indexOf('/')/indexOf(':')sequence. Consider extracting a shared helper that returns thehost:portpair once.Suggested refactor
+ private String[] extractHostAndPort(String jdbcUrl) { + if (jdbcUrl == null || UNKNOWN_VALUE.equals(jdbcUrl)) { + return new String[]{UNKNOWN_VALUE, UNKNOWN_VALUE}; + } + try { + String withoutPrefix = jdbcUrl.replaceFirst("jdbc:mysql://", ""); + int slashIndex = withoutPrefix.indexOf('/'); + String hostPort = slashIndex > 0 + ? withoutPrefix.substring(0, slashIndex) + : withoutPrefix; + int colonIndex = hostPort.indexOf(':'); + String host = colonIndex > 0 ? hostPort.substring(0, colonIndex) : hostPort; + String port = colonIndex > 0 ? hostPort.substring(colonIndex + 1) : "3306"; + return new String[]{host, port}; + } catch (Exception e) { + logger.debug("Could not parse host/port from URL", e); + return new String[]{UNKNOWN_VALUE, "3306"}; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` around lines 243 - 277, extractHost and extractPort duplicate the JDBC URL parsing; extract the common preamble into a private helper (e.g., getHostPortFromJdbcUrl(String jdbcUrl)) that returns the "host:port" string or null/UNKNOWN_VALUE on error, reuse that helper inside extractHost and extractPort to split on ':' for host or port, preserve the same defaults ("3306") and UNKNOWN_VALUE behavior and keep the existing logger.debug exception handling by catching exceptions inside the helper and logging with logger.debug("Could not extract host/port from URL", e).
78-78: Health checkINFOlog on every invocation may be noisy.Load balancers and monitoring systems typically poll
/healthevery few seconds. Logging atINFOlevel for each call will flood the logs. ConsiderDEBUGlevel instead.Suggested change
- logger.info("Health check completed - Overall status: {}", overallHealth ? STATUS_UP : STATUS_DOWN); + logger.debug("Health check completed - Overall status: {}", overallHealth ? STATUS_UP : STATUS_DOWN);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/ecd/service/health/HealthService.java` at line 78, The health check currently logs every invocation at INFO in HealthService (the logger.info call that prints "Health check completed - Overall status: {}"), which is noisy; change this to logger.debug (or guard it behind logger.isDebugEnabled()) so routine /health polls don't flood logs—update the logging statement in HealthService to use debug-level logging while preserving the same message and status expression (STATUS_UP/STATUS_DOWN).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 286-291: The substring extraction in HealthService (method
handling jdbcUrl using variables jdbcUrl, lastSlash, afterSlash, queryStart)
incorrectly uses the condition "queryStart > 0" which lets a leading '?' (index
0) fall through; change the condition to "queryStart >= 0" so that when
afterSlash starts with '?' (e.g., "/?useSSL...") you correctly return an empty
database name (afterSlash.substring(0, queryStart)) instead of the raw query
string.
---
Duplicate comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Line 36: HealthService currently holds a static ExecutorService
(executorService) that is never shut down; add a clean shutdown to the class by
either converting executorService to a Spring-managed bean (injecting a
TaskExecutor/ThreadPoolTaskExecutor) or keep the field and add a `@PreDestroy`
method (e.g., shutdownExecutor or destroy) that calls
executorService.shutdownNow()/shutdown() and awaits termination with a timeout,
handling InterruptedException and logging appropriately to ensure the thread
pool is terminated on context close/redeploy.
---
Nitpick comments:
In `@src/main/java/com/iemr/ecd/service/health/HealthService.java`:
- Around line 299-309: Replace the inner immutable class HealthCheckResult with
a Java record (e.g., record HealthCheckResult(boolean isHealthy, String version,
String error)) to simplify the data carrier; then update all usages in
performHealthCheck (and elsewhere) to use record accessor methods
(result.isHealthy() / result.version() / result.error()) instead of field access
(result.isHealthy, result.version, result.error). Ensure the constructor
semantics remain the same and remove the old final fields and explicit
constructor.
- Around line 243-277: extractHost and extractPort duplicate the JDBC URL
parsing; extract the common preamble into a private helper (e.g.,
getHostPortFromJdbcUrl(String jdbcUrl)) that returns the "host:port" string or
null/UNKNOWN_VALUE on error, reuse that helper inside extractHost and
extractPort to split on ':' for host or port, preserve the same defaults
("3306") and UNKNOWN_VALUE behavior and keep the existing logger.debug exception
handling by catching exceptions inside the helper and logging with
logger.debug("Could not extract host/port from URL", e).
- Line 78: The health check currently logs every invocation at INFO in
HealthService (the logger.info call that prints "Health check completed -
Overall status: {}"), which is noisy; change this to logger.debug (or guard it
behind logger.isDebugEnabled()) so routine /health polls don't flood logs—update
the logging statement in HealthService to use debug-level logging while
preserving the same message and status expression (STATUS_UP/STATUS_DOWN).
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 ECD-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Behavior Change
Chores