Add /health endpoint and standardize /version response #331
Conversation
📝 WalkthroughWalkthroughAdds HealthController and HealthService to expose GET /health with component checks (database, Redis) and status mapping; refactors VersionController to return a new VersionInfo DTO (commitHash, buildTime) loaded from git.properties. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant HC as HealthController
participant HS as HealthService
participant DB as Database
participant RD as Redis
Client->>HC: GET /health
HC->>HS: checkHealth()
HS->>HS: build components map
alt DataSource configured
HS->>DB: open connection & run test query
DB-->>HS: success / error
else DataSource not configured
HS-->>HS: mark database NOT_CONFIGURED
end
alt Redis configured
HS->>RD: PING
RD-->>HS: PONG / error
else Redis not configured
HS-->>HS: mark redis NOT_CONFIGURED
end
HS-->>HC: return Map(status, components)
alt status == "UP"
HC-->>Client: 200 OK + body
else
HC-->>Client: 503 Service Unavailable + body
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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
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.
src/main/java/com/iemr/common/controller/health/HealthController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/controller/health/HealthController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/controller/health/HealthController.java
Outdated
Show resolved
Hide resolved
|
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! |
|
Keep controllers lean. |
Thanks for the feedback! I have refactored the code to follow the separation of concerns. Please let me know if any other changes are needed.@drtechie |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/iemr/common/service/health/HealthService.java`:
- Around line 49-55: The constructor in HealthService currently injects
DataSource and RedisConnectionFactory as mandatory dependencies but the runtime
null checks (in methods referencing dataSource and redisConnectionFactory) are
unreachable; either remove those redundant null checks if DataSource and
RedisConnectionFactory must always be present, or make the dependencies optional
by replacing constructor parameters with ObjectProvider<DataSource> and
ObjectProvider<RedisConnectionFactory> (store the providers) and update the
health-check methods to call getIfAvailable(), handling a null return to return
degraded/unavailable health. Ensure references to DataSource and
RedisConnectionFactory inside methods like the database and redis health check
functions use the provider-getter logic or the directly injected fields
consistently.
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/service/health/HealthService.java (1)
80-83: Add timeouts to keep/healthresponsive during dependency stalls
Line 80–83 and Line 101–102 can block indefinitely if DB/Redis hangs, potentially tying up request threads. Consider a query timeout and ensure Redis socket timeouts are configured.⏱ Example for DB query timeout
try (Connection connection = dataSource.getConnection(); Statement statement = connection.createStatement()) { + statement.setQueryTimeout(2); // seconds statement.execute("SELECT 1");Also applies to: 101-102
|
2 participants
📋 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.