Add /health endpoint and standardize /version response #331
Conversation
📝 WalkthroughWalkthroughAdds a HealthController exposing GET /health that reports overall status and per-component (database, redis) states; refactors VersionController to return a new VersionInfo DTO (commitHash, buildTime) loaded from git.properties. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthController
participant Database
participant Redis
Client->>HealthController: GET /health
HealthController->>HealthController: init components map
alt DataSource configured
HealthController->>Database: open connection & run test query
Database-->>HealthController: success / exception
else DataSource not configured
HealthController-->>HealthController: mark database NOT_CONFIGURED
end
alt Redis configured
HealthController->>Redis: open connection & PING
Redis-->>HealthController: PONG / error
else Redis not configured
HealthController-->>HealthController: mark redis NOT_CONFIGURED
end
HealthController->>HealthController: compute overall status (UP/DOWN)
alt overall UP
HealthController-->>Client: 200 OK + body
else overall DOWN
HealthController-->>Client: 503 Service Unavailable + body
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (6)
src/main/java/com/iemr/common/controller/health/HealthController.java (2)
22-27: Javadoc placement before package declaration is non-standard.In Java, the class-level Javadoc should appear after imports and immediately before the class definition, not before the
packagestatement.🔎 Suggested fix
* along with this program. If not, see https://www.gnu.org/licenses/. */ -/** - * Health check controller for Common-API. - * - * @author vaishnavbhosale - */ package com.iemr.common.controller.health; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.RestController; import java.util.Map; +/** + * Health check controller for Common-API. + * + * @author vaishnavbhosale + */ @RestController public class HealthController {
33-40: Consider adding@Operationannotation for API documentation consistency.The
VersionControlleruses@Operation(summary = "Get version")for Swagger documentation. For consistency across endpoints, consider adding similar documentation here.🔎 Suggested fix
+import io.swagger.v3.oas.annotations.Operation; + @RestController public class HealthController { + @Operation(summary = "Health check") @GetMapping("/health") public Map<String, String> health() { return Map.of("status", "UP"); } }src/main/java/com/iemr/common/controller/version/VersionController.java (2)
22-31: Javadoc placement before package declaration is non-standard.Same as in
HealthController, the Javadoc should appear after imports and before the class definition.
56-66: Consider logging whengit.propertiesis missing.If
git.propertiesis absent (e.g., running from IDE without a build), the code silently returns defaults. A debug/warn log would help diagnose why version info shows "unknown".🔎 Suggested improvement
try (InputStream is = getClass() .getClassLoader() .getResourceAsStream("git.properties")) { if (is != null) { properties.load(is); + } else { + logger.warn("git.properties not found on classpath"); } } catch (Exception e) { logger.error("Error reading git.properties", e); }src/main/java/com/iemr/common/controller/version/VersionInfo.java (2)
22-27: Javadoc placement before package declaration is non-standard.Same as the other files - move Javadoc after imports and before the class definition.
29-46: Consider making fieldsfinalfor true immutability.Since this is a DTO with only getters, marking the fields as
finalclearly communicates immutability and prevents accidental modification.🔎 Suggested fix
public class VersionInfo { - private String commitHash; - private String buildTime; + private final String commitHash; + private final String buildTime; public VersionInfo(String commitHash, String buildTime) { this.commitHash = commitHash; this.buildTime = buildTime; }Alternatively, given the project targets Java 17 and already uses records extensively, this could be simplified to a
record:public record VersionInfo(String commitHash, String buildTime) {}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/common/controller/health/HealthController.javasrc/main/java/com/iemr/common/controller/version/VersionController.javasrc/main/java/com/iemr/common/controller/version/VersionInfo.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-07T19:32:42.660Z
Learnt from: helenKaryamsetty
Repo: PSMRI/Common-API PR: 123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.
Applied to files:
src/main/java/com/iemr/common/controller/health/HealthController.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/common/controller/health/HealthController.java (1)
src/main/java/com/iemr/common/controller/version/VersionController.java (1)
RestController(44-73)
🔇 Additional comments (1)
src/main/java/com/iemr/common/controller/version/VersionController.java (1)
52-71: Implementation looks correct and handles edge cases well.Good use of try-with-resources for automatic resource cleanup, proper null checking, and sensible default values when properties are unavailable. The refactoring to return
VersionInfoinstead of a String provides a cleaner API contract.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/controller/health/HealthController.java:
- Around line 85-99: The catch block in checkRedis currently swallows
exceptions; update checkRedis (which uses redisConnectionFactory and
RedisConnection) to log the exception when connectivity fails: in the
catch(Exception e) for checkRedis, call the controller's logger (e.g.,
logger.error or LOG.error) with a descriptive message like "Redis connectivity
check failed" and include the exception e so the stacktrace/error details are
recorded, then continue to set components.put("redis","DOWN") and return false.
- Around line 55-68: The health() method currently always returns 200; modify
HealthController.health() to return a ResponseEntity<Map<String,Object>> instead
of raw Map so you can set the HTTP status based on the computed health: call
checkDatabase(components) and checkRedis(components) as before, build the
response map with "status" and "components", then return
ResponseEntity.ok(response) when both are UP and
ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(response) when either
is DOWN; ensure imports for ResponseEntity and HttpStatus are added and
references to health(), checkDatabase(), and checkRedis() are preserved.
- Around line 70-83: The checkDatabase method currently swallows exceptions;
update the catch block in checkDatabase to log the failure (use the class logger
at WARN or ERROR level) including the exception message and stacktrace and the
components map entry for "database" before returning false. While here, after
obtaining the Connection from dataSource in checkDatabase, optionally run a
lightweight validation query (e.g., "SELECT 1") via
connection.createStatement()/executeQuery() and only mark "database" as "UP" if
the query succeeds; on any SQLException log the error and set
components.put("database","DOWN").
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/controller/health/HealthController.java (1)
23-32: Consider moving Javadoc to class-level orpackage-info.java.Package-level Javadoc before the
packagestatement is non-standard. Most tools and conventions expect:
- Package documentation in a separate
package-info.javafile, or- Class documentation directly above the class declaration
Moving this to a class-level Javadoc would improve clarity and tool compatibility.
♻️ Suggested refactor
Move the Javadoc to just above the class:
-/** - * Health check controller for Common-API. - * <p> - * Verifies application liveness and connectivity to underlying - * runtime dependencies such as Database and Redis. - * </p> - * - * @author vaishnavbhosale - */ package com.iemr.common.controller.health; import java.sql.Connection; import java.util.HashMap; import java.util.Map; import javax.sql.DataSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.redis.connection.RedisConnection; import org.springframework.data.redis.connection.RedisConnectionFactory; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.RestController; +/** + * Health check controller for Common-API. + * <p> + * Verifies application liveness and connectivity to underlying + * runtime dependencies such as Database and Redis. + * </p> + * + * @author vaishnavbhosale + */ @RestController public class HealthController {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/controller/health/HealthController.java
🔇 Additional comments (2)
src/main/java/com/iemr/common/controller/health/HealthController.java (2)
1-21: License header is properly formatted.The GPLv3 license header is complete and follows standard conventions.
46-53: Appropriate use of optional dependencies.Using
@Autowired(required = false)forDataSourceandRedisConnectionFactorycorrectly handles environments where these dependencies might not be configured.
|
|
Hi @drtechie , I’ve updated the Common-API /health and /version implementation based on the review feedback. Changes completed:
All CI workflows and SonarCloud checks are passing. Please let me know if you’d like me to apply the same pattern to other AMRIT API repositories or make any further refinements. Thanks! |
1 participant
📋 Description
JIRA ID: N/A
This PR implements standardized /health and /version endpoints in the Common-API as part of Issue #102.
✅ Type of Change
ℹ️ Additional Information
Testing:
Notes:
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.