feat(health,version): add health and version endpoints#136
Open
DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
Open
feat(health,version): add health and version endpoints#136DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds health and version REST endpoints, a HealthService performing MySQL and Redis checks, JWT authentication for /health, a skip for /health and /version in the JWT filter, and a Maven plugin to generate git.properties during the build. Changes
Sequence DiagramssequenceDiagram
participant Client
participant HealthController
participant JwtAuthenticationUtil
participant HealthService
participant MySQL
participant Redis
Client->>HealthController: GET /health (with JWT)
HealthController->>JwtAuthenticationUtil: validate token (header/cookie)
alt Authenticated
JwtAuthenticationUtil-->>HealthController: OK
HealthController->>HealthService: checkHealth(includeDetails=true)
par MySQL check
HealthService->>MySQL: SELECT 1 (3s timeout)
MySQL-->>HealthService: result / error
and Redis check
HealthService->>Redis: PING (3s timeout) / skip if absent
Redis-->>HealthService: PONG / error
end
HealthService-->>HealthController: aggregated health map
alt status UP
HealthController-->>Client: 200 OK + health map
else
HealthController-->>Client: 503 Service Unavailable + health map
end
else
JwtAuthenticationUtil-->>HealthController: Auth failed
HealthController-->>Client: 503 Service Unavailable + sanitized error
end
sequenceDiagram
participant Client
participant JwtUserIdValidationFilter
participant VersionController
participant ClasspathResource
Client->>JwtUserIdValidationFilter: GET /version
JwtUserIdValidationFilter->>JwtUserIdValidationFilter: path startsWith /version? -> skip auth
alt skipped
JwtUserIdValidationFilter-->>VersionController: forward request
end
VersionController->>ClasspathResource: load git.properties
alt properties found
ClasspathResource-->>VersionController: properties
else
ClasspathResource-->>VersionController: none (use unknown)
end
VersionController-->>Client: 200 OK + {buildTimestamp, version, branch, commitHash}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issuesPoem
🚥 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.
|
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
New Features
Reliability