Add /health endpoint and update /version endpoint#188
Add /health endpoint and update /version endpoint#188DurgaPrasad-54 wants to merge 2 commits intoPSMRI:mainfrom
Conversation
📝 WalkthroughWalkthroughThe changes introduce a health check system with new /health and /version endpoints that return structured JSON responses. A Maven plugin is added to generate Git metadata at build time. Both endpoints are configured to bypass JWT authentication for public access. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HealthController
participant HealthService
participant MySQL
participant Redis
Client->>HealthController: GET /health
HealthController->>HealthService: checkHealth()
HealthService->>MySQL: Test connection & query version
MySQL-->>HealthService: Version info or error
HealthService->>Redis: Test connection & query info
Redis-->>HealthService: Info response or error
HealthService->>HealthService: Aggregate status<br/>(UP/DOWN per component)<br/>with response times
HealthService-->>HealthController: Map with overall status<br/>& component details
HealthController-->>Client: 200 OK with health JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🤖 Fix all issues with AI agents
In `@src/main/java/com/iemr/hwc/controller/health/HealthController.java`:
- Around line 23-28: The health() endpoint in HealthController always returns
200; change it to return 503 when the service reports DOWN by inspecting the map
returned from healthService.checkHealth() (e.g., check
healthStatus.get("status") or similar), and if the status equals "DOWN"
(case-insensitive) return
ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(healthStatus),
otherwise return ResponseEntity.ok(healthStatus); keep the existing logging and
ensure you import/use HttpStatus and do null-safety when reading the "status"
key.
In `@src/main/java/com/iemr/hwc/service/health/HealthService.java`:
- Around line 44-71: The public checkHealth() currently returns detailed
component data (host, port, database, version, and error messages) that exposes
infra too broadly; change it so checkHealth() only returns minimal data (e.g.,
{"status":"UP"|"DOWN","timestamp":...}) and do not include the "components" map
for unauthenticated callers, and either (a) move the detailed components payload
into a new authenticated method/endpoint (e.g., getHealthDetails() or
checkHealthDetails()) that calls checkMySQLHealth() and checkRedisHealth(), or
(b) sanitize the components map before returning by removing sensitive keys
("host","port","database","version") and stripping raw exception/error messages;
update calls to checkMySQLHealth() and checkRedisHealth() so they still perform
checks but only surface non-sensitive boolean/status flags to the public
checkHealth().
- Around line 134-145: The details map is missing responseTimeMs when
result.isHealthy is false; in HealthService (the block that checks
result.isHealthy) ensure you always put responseTimeMs into details regardless
of success or failure—add details.put("responseTimeMs", responseTimeMs) inside
the else branch that handles result.isHealthy == false (near the code
referencing result.error and status.put("status", "DOWN")) so the response shape
matches the success and exception paths.
In `@src/main/resources/application.properties`:
- Line 47: The trailing newline after the comment "Keep a line at the end so
that environment.ts file can be appended properly" was removed; restore the
final newline in src/main/resources/application.properties so that future
appends to environment.ts begin on a new line, or if the requirement is
obsolete, remove that comment line entirely—update the file to either re-add the
trailing newline after the existing comment text or delete the comment.
🧹 Nitpick comments (6)
src/main/java/com/iemr/hwc/controller/version/VersionController.java (3)
42-42: PreferLoggerFactory.getLogger(VersionController.class)overgetSimpleName().Using
getSimpleName()strips the package prefix from the logger category, which breaks package-level log configuration (e.g.,logging.level.com.iemr.hwc.controller=DEBUGwon't apply to this logger).♻️ Suggested fix
- private final Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); + private final Logger logger = LoggerFactory.getLogger(VersionController.class);
46-63: The endpoint logic is sound overall. A couple of optional improvements:
- The
HashMapproduces non-deterministic key ordering in JSON. ConsiderLinkedHashMapif consistent output is desired.- The four
"unknown"string literals are duplicated between the default parameters (lines 51-54) and the catch block (lines 57-60). A constant (e.g.,private static final String UNKNOWN = "unknown") would reduce duplication.Both are cosmetic — the logic itself is correct.
66-75: Log a warning whengit.propertiesis not found on the classpath.If
getResourceAsStreamreturnsnull, the method silently returns emptyProperties. This makes it harder to diagnose missing build-time plugin configuration. A warning log would help operators during deployment.♻️ Suggested fix
private Properties loadGitProperties() throws IOException { Properties properties = new Properties(); try (InputStream input = getClass().getClassLoader() .getResourceAsStream("git.properties")) { if (input != null) { properties.load(input); + } else { + logger.warn("git.properties not found on classpath"); } } return properties; }src/main/java/com/iemr/hwc/service/health/HealthService.java (2)
69-69: Health check logs at INFO on every invocation — consider DEBUG level.If this endpoint is polled by a load balancer or monitoring system every few seconds, INFO-level logging will be very noisy. Use
logger.debug(...)here, consistent with Line 135.Proposed fix
- logger.info("Health check completed - Overall status: {}", overallHealth ? "UP" : "DOWN"); + logger.debug("Health check completed - Overall status: {}", overallHealth ? "UP" : "DOWN");
82-100: MySQL health check acquires a connection from the pool on every call.Each health check call gets a JDBC connection, validates it, runs
SELECT 1, and runsSELECT VERSION(). Under frequent polling, this adds load to both the connection pool and the database.Consider caching the health result for a short TTL (e.g., 5–10 seconds) to avoid hammering the database and pool on rapid successive calls.
src/main/java/com/iemr/hwc/controller/health/HealthController.java (1)
3-3: Unused importHashMap.
java.util.HashMapis imported but never used in this file.Proposed fix
-import java.util.HashMap; import java.util.Map;
| @GetMapping("/health") | ||
| public ResponseEntity<Map<String, Object>> health() { | ||
| logger.debug("Health check endpoint called"); | ||
| Map<String, Object> healthStatus = healthService.checkHealth(); | ||
| return ResponseEntity.ok(healthStatus); | ||
| } |
There was a problem hiding this comment.
Return HTTP 503 when health status is DOWN.
The endpoint always returns 200 OK, even when the service reports "status": "DOWN". Load balancers, orchestrators (Kubernetes), and monitoring tools rely on the HTTP status code to determine service health. Returning 200 for a degraded service means it will continue receiving traffic.
Proposed fix
`@GetMapping`("/health")
public ResponseEntity<Map<String, Object>> health() {
logger.debug("Health check endpoint called");
Map<String, Object> healthStatus = healthService.checkHealth();
- return ResponseEntity.ok(healthStatus);
+ boolean isUp = "UP".equals(healthStatus.get("status"));
+ return ResponseEntity.status(isUp ? 200 : 503).body(healthStatus);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping("/health") | |
| public ResponseEntity<Map<String, Object>> health() { | |
| logger.debug("Health check endpoint called"); | |
| Map<String, Object> healthStatus = healthService.checkHealth(); | |
| return ResponseEntity.ok(healthStatus); | |
| } | |
| `@GetMapping`("/health") | |
| public ResponseEntity<Map<String, Object>> health() { | |
| logger.debug("Health check endpoint called"); | |
| Map<String, Object> healthStatus = healthService.checkHealth(); | |
| boolean isUp = "UP".equals(healthStatus.get("status")); | |
| return ResponseEntity.status(isUp ? 200 : 503).body(healthStatus); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/iemr/hwc/controller/health/HealthController.java` around
lines 23 - 28, The health() endpoint in HealthController always returns 200;
change it to return 503 when the service reports DOWN by inspecting the map
returned from healthService.checkHealth() (e.g., check
healthStatus.get("status") or similar), and if the status equals "DOWN"
(case-insensitive) return
ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(healthStatus),
otherwise return ResponseEntity.ok(healthStatus); keep the existing logging and
ensure you import/use HttpStatus and do null-safety when reading the "status"
key.
| public Map<String, Object> checkHealth() { | ||
| Map<String, Object> healthStatus = new LinkedHashMap<>(); | ||
| Map<String, Object> components = new LinkedHashMap<>(); | ||
| boolean overallHealth = true; | ||
|
|
||
| // Check MySQL connectivity | ||
| Map<String, Object> mysqlStatus = checkMySQLHealth(); | ||
| components.put("mysql", mysqlStatus); | ||
| if (!isHealthy(mysqlStatus)) { | ||
| overallHealth = false; | ||
| } | ||
|
|
||
| // Check Redis connectivity if configured | ||
| if (redisTemplate != null) { | ||
| Map<String, Object> redisStatus = checkRedisHealth(); | ||
| components.put("redis", redisStatus); | ||
| if (!isHealthy(redisStatus)) { | ||
| overallHealth = false; | ||
| } | ||
| } | ||
|
|
||
| healthStatus.put("status", overallHealth ? "UP" : "DOWN"); | ||
| healthStatus.put("timestamp", Instant.now().toString()); | ||
| healthStatus.put("components", components); | ||
|
|
||
| logger.info("Health check completed - Overall status: {}", overallHealth ? "UP" : "DOWN"); | ||
| return healthStatus; | ||
| } |
There was a problem hiding this comment.
Unauthenticated endpoint exposes infrastructure details — information disclosure risk.
The /health endpoint is bypassed from JWT auth and returns host, port, database name, component versions, and error messages to any caller. This leaks infrastructure topology to unauthenticated users, which can aid reconnaissance attacks.
Consider:
- Returning only
{"status": "UP"}or{"status": "DOWN"}for the unauthenticated endpoint. - Gating the detailed component information behind authentication, or using a separate authenticated
/health/detailsendpoint. - At minimum, strip
host,port,database, andversionfrom the public response.
🤖 Prompt for AI Agents
In `@src/main/java/com/iemr/hwc/service/health/HealthService.java` around lines 44
- 71, The public checkHealth() currently returns detailed component data (host,
port, database, version, and error messages) that exposes infra too broadly;
change it so checkHealth() only returns minimal data (e.g.,
{"status":"UP"|"DOWN","timestamp":...}) and do not include the "components" map
for unauthenticated callers, and either (a) move the detailed components payload
into a new authenticated method/endpoint (e.g., getHealthDetails() or
checkHealthDetails()) that calls checkMySQLHealth() and checkRedisHealth(), or
(b) sanitize the components map before returning by removing sensitive keys
("host","port","database","version") and stripping raw exception/error messages;
update calls to checkMySQLHealth() and checkRedisHealth() so they still perform
checks but only surface non-sensitive boolean/status flags to the public
checkHealth().
| if (result.isHealthy) { | ||
| logger.debug("{} health check: UP ({}ms)", componentName, responseTime); | ||
| status.put("status", "UP"); | ||
| details.put("responseTimeMs", responseTime); | ||
| if (result.version != null) { | ||
| details.put("version", result.version); | ||
| } | ||
| } else { | ||
| logger.warn("{} health check: {}", componentName, result.error); | ||
| status.put("status", "DOWN"); | ||
| details.put("error", result.error); | ||
| } |
There was a problem hiding this comment.
responseTimeMs is only added to details on success, not on the unhealthy (non-exception) path.
When result.isHealthy is false (Line 141–144), responseTimeMs is not added to the details map, though it is added on success (Line 137) and on exception (Line 154). This creates inconsistent response shapes.
Proposed fix
} else {
logger.warn("{} health check: {}", componentName, result.error);
status.put("status", "DOWN");
details.put("error", result.error);
+ details.put("responseTimeMs", responseTime);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (result.isHealthy) { | |
| logger.debug("{} health check: UP ({}ms)", componentName, responseTime); | |
| status.put("status", "UP"); | |
| details.put("responseTimeMs", responseTime); | |
| if (result.version != null) { | |
| details.put("version", result.version); | |
| } | |
| } else { | |
| logger.warn("{} health check: {}", componentName, result.error); | |
| status.put("status", "DOWN"); | |
| details.put("error", result.error); | |
| } | |
| if (result.isHealthy) { | |
| logger.debug("{} health check: UP ({}ms)", componentName, responseTime); | |
| status.put("status", "UP"); | |
| details.put("responseTimeMs", responseTime); | |
| if (result.version != null) { | |
| details.put("version", result.version); | |
| } | |
| } else { | |
| logger.warn("{} health check: {}", componentName, result.error); | |
| status.put("status", "DOWN"); | |
| details.put("error", result.error); | |
| details.put("responseTimeMs", responseTime); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/iemr/hwc/service/health/HealthService.java` around lines
134 - 145, The details map is missing responseTimeMs when result.isHealthy is
false; in HealthService (the block that checks result.isHealthy) ensure you
always put responseTimeMs into details regardless of success or failure—add
details.put("responseTimeMs", responseTimeMs) inside the else branch that
handles result.isHealthy == false (near the code referencing result.error and
status.put("status", "DOWN")) so the response shape matches the success and
exception paths.
|
|
||
|
|
||
| # Keep a line at the end so that environment.ts file can be appended properly | ||
| # Keep a line at the end so that environment.ts file can be appended properly No newline at end of file |
There was a problem hiding this comment.
Removing the trailing newline contradicts the comment's own instruction.
The comment on Line 47 says to keep a line at the end so that the environment file can be appended properly. If the trailing newline was indeed removed, subsequent appends may concatenate onto this comment line instead of starting on a new line. Either restore the trailing newline or remove the comment if it's no longer applicable.
🤖 Prompt for AI Agents
In `@src/main/resources/application.properties` at line 47, The trailing newline
after the comment "Keep a line at the end so that environment.ts file can be
appended properly" was removed; restore the final newline in
src/main/resources/application.properties so that future appends to
environment.ts begin on a new line, or if the requirement is obsolete, remove
that comment line entirely—update the file to either re-add the trailing newline
after the existing comment text or delete the comment.
1 participant
📋 Description
JIRA ID:
This PR implements /health and /version endpoints in the HWC-API as part of Issue PSMRI/Common-API#102.
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
Release Notes
New Features
Chores