Implement /health and /version endpoints for Admin API #95
Conversation
WalkthroughAdds Maven build metadata generation, registers a JWT validation servlet filter that skips /health and /version, and introduces Health and Version endpoints with corresponding services that perform health checks and aggregate version/build/git information. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as ServletContainer
participant JwtFilter as JwtUserIdValidationFilter
participant HealthCtrl as HealthController
participant HealthSvc as HealthService
participant VersionCtrl as VersionController
participant VersionSvc as VersionService
Client->>API: GET /health
API->>JwtFilter: doFilter(request)
JwtFilter-->>API: skip validation for /health
API->>HealthCtrl: health()
HealthCtrl->>HealthSvc: checkHealth()
HealthSvc-->>HealthCtrl: health map (components, status)
HealthCtrl-->>API: ResponseEntity (200 or 503 JSON)
API-->>Client: HTTP response
Client->>API: GET /version
API->>JwtFilter: doFilter(request)
JwtFilter-->>API: skip validation for /version
API->>VersionCtrl: versionInformation()
VersionCtrl->>VersionSvc: getVersionInfo()
VersionSvc-->>VersionCtrl: Map<String,String> (git/build/current.time)
VersionCtrl-->>API: 200 JSON
API-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
🔭 Outside diff range comments (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
165-170: Remove duplicate codeLines 168-169 are exact duplicates of lines 165-166.
logger.warn("No valid authentication token found"); response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token"); - - logger.warn("No valid authentication token found"); - response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token");
🧹 Nitpick comments (1)
src/main/java/com/iemr/admin/controller/health/HealthController.java (1)
31-40: Consider adding OpenAPI documentationFor consistency with other endpoints like
/version, consider adding Swagger/OpenAPI documentation.+import io.swagger.v3.oas.annotations.Operation; + @RestController public class HealthController { + @Operation(summary = "Health check endpoint") @GetMapping("/health") public ResponseEntity<Map<String, String>> health() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(1 hunks)src/main/java/com/iemr/admin/config/SecurityFilterConfig.java(1 hunks)src/main/java/com/iemr/admin/controller/health/HealthController.java(1 hunks)src/main/java/com/iemr/admin/controller/version/VersionController.java(2 hunks)src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/admin/controller/health/HealthController.java (1)
src/main/java/com/iemr/admin/controller/version/VersionController.java (1)
RestController(43-160)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
src/main/java/com/iemr/admin/utils/http/AuthorizationHeaderRequestWrapper.java (1)
AuthorizationHeaderRequestWrapper(10-42)
🔇 Additional comments (6)
pom.xml (1)
489-500: LGTM!The addition of the
build-infogoal correctly generates build metadata that the/versionendpoint can consume.src/main/java/com/iemr/admin/config/SecurityFilterConfig.java (1)
12-32: LGTM!Clean Spring configuration that properly registers the JWT filter with URL exclusions.
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (2)
24-48: LGTM!Proper Spring integration with no-args constructor and FilterConfig storage.
224-236: LGTM!Proper Spring setter injection with appropriate null handling.
src/main/java/com/iemr/admin/controller/version/VersionController.java (2)
48-64: LGTM!Proper JSON content type declaration and structured response handling.
137-160: LGTM!Well-implemented helper methods with proper resource management and error handling.
src/main/java/com/iemr/admin/controller/version/VersionController.java
Outdated
Show resolved
Hide resolved
| // Check for excluded URLs first | ||
| String path = request.getRequestURI(); | ||
| if (isExcludedUrl(path) || | ||
| path.equals("/health") || | ||
| path.equals("/version")) { | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hardcoded URL exclusions
The /health and /version paths are already configured as excluded URLs in SecurityFilterConfig. Remove the hardcoded checks to follow DRY principle.
// Check for excluded URLs first
String path = request.getRequestURI();
- if (isExcludedUrl(path) ||
- path.equals("/health") ||
- path.equals("/version")) {
+ if (isExcludedUrl(path)) {
filterChain.doFilter(servletRequest, servletResponse);
return;
}📝 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.
| // Check for excluded URLs first | |
| String path = request.getRequestURI(); | |
| if (isExcludedUrl(path) || | |
| path.equals("/health") || | |
| path.equals("/version")) { | |
| filterChain.doFilter(servletRequest, servletResponse); | |
| return; | |
| } | |
| // Check for excluded URLs first | |
| String path = request.getRequestURI(); | |
| if (isExcludedUrl(path)) { | |
| filterChain.doFilter(servletRequest, servletResponse); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java between
lines 74 and 82, remove the hardcoded checks for "/health" and "/version" URL
paths since these are already included in the excluded URLs configured in
SecurityFilterConfig. Keep only the call to isExcludedUrl(path) to check for
exclusions, ensuring no duplication and adherence to the DRY principle.
| @GetMapping("/health") | ||
| public ResponseEntity<Map<String, String>> health() { | ||
| Map<String, String> response = new HashMap<>(); | ||
| response.put("status", "UP"); |
There was a problem hiding this comment.
This alone won't be enough.
Can we test associated services like DB and Redis connection?
|
@vishwab1 / @vanitha1822 |
| json.append(" \"build.time\": \"").append(getCurrentIstTimeFormatted()).append("\",\n"); | ||
| } else { | ||
| logger.warn("Neither build-info.properties nor Maven properties found."); | ||
| json.append(" \"build.version\": \"3.1.0\",\n"); // Default version |
There was a problem hiding this comment.
We can move the logic (like reading Git properties and building the JSON) into a separate service method instead of writing everything inside the controller. Also, let’s review all the suggestions given by CodeRabbit once again. One of the points to check is the use of StringBuilder, as it might cause issues in some cases.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/com/iemr/admin/service/version/VersionService.java (2)
45-46: Consider using @ConfigurationProperties for better configuration management.Using
@Valuefor individual properties works but@ConfigurationPropertieswould provide better type safety and validation for application properties.
107-107: Hardcoded Maven path may not be portable across different projects.The hardcoded path
META-INF/maven/com.iemr.admin/admin-api/pom.propertiesis specific to this project's group/artifact ID and may break if the project structure changes.Consider making this configurable or using a more generic approach:
- Properties mavenProps = loadPropertiesFile("META-INF/maven/com.iemr.admin/admin-api/pom.properties"); + Properties mavenProps = loadPropertiesFile("META-INF/maven/" + + System.getProperty("project.groupId", "com.iemr.admin") + "/" + + System.getProperty("project.artifactId", "admin-api") + "/pom.properties");src/main/java/com/iemr/admin/controller/health/HealthController.java (1)
54-55: Consider using constants for status values.The string literal "UP" is used for comparison. Consider defining constants to avoid magic strings and ensure consistency across the codebase.
+ private static final String STATUS_UP = "UP"; + private static final String STATUS_DOWN = "DOWN"; + // Return 503 if any service is down, 200 if all are up String status = (String) healthStatus.get("status"); - HttpStatus httpStatus = "UP".equals(status) ? HttpStatus.OK : HttpStatus.SERVICE_UNAVAILABLE; + HttpStatus httpStatus = STATUS_UP.equals(status) ? HttpStatus.OK : HttpStatus.SERVICE_UNAVAILABLE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pom.xml(3 hunks)src/main/java/com/iemr/admin/config/SecurityFilterConfig.java(1 hunks)src/main/java/com/iemr/admin/controller/health/HealthController.java(1 hunks)src/main/java/com/iemr/admin/controller/version/VersionController.java(1 hunks)src/main/java/com/iemr/admin/service/health/HealthService.java(1 hunks)src/main/java/com/iemr/admin/service/version/VersionService.java(1 hunks)src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/iemr/admin/config/SecurityFilterConfig.java
- src/main/java/com/iemr/admin/controller/version/VersionController.java
- pom.xml
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/iemr/admin/controller/health/HealthController.java (1)
src/main/java/com/iemr/admin/controller/version/VersionController.java (1)
RestController(37-60)
src/main/java/com/iemr/admin/service/version/VersionService.java (1)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
Service(39-198)
🔇 Additional comments (8)
src/main/java/com/iemr/admin/service/version/VersionService.java (4)
50-58: LGTM!Good error handling with graceful fallback to error response when version information cannot be built.
75-91: Robust fallback logic for Git information.The multiple fallback property keys ensure compatibility with different git-commit-id-plugin configurations, and the graceful handling of missing files with warning logs is well-implemented.
126-138: Proper resource management with try-with-resources.Good use of try-with-resources for automatic resource cleanup and appropriate exception handling with logging.
140-153: Comprehensive error response handling.The nested try-catch ensures that even if JSON serialization of the error response fails, a basic JSON error message is returned. This prevents the endpoint from returning non-JSON responses.
src/main/java/com/iemr/admin/controller/health/HealthController.java (1)
46-59: Addresses previous review feedback with comprehensive health checks.The implementation now includes database and Redis connection testing through the
HealthService, directly addressing the previous comment about testing associated services. The HTTP status code mapping (200 for UP, 503 for DOWN) follows health check best practices.src/main/java/com/iemr/admin/service/health/HealthService.java (3)
87-134: Comprehensive database health check implementation.Excellent implementation that tests connection validity, executes a simple query, collects metadata, measures response time, and provides detailed error information. The try-with-resources ensures proper connection cleanup.
147-153: Redis operations test provides thorough validation.The set/get/delete test ensures Redis is not only reachable but also functional for actual operations. Good cleanup of test data to avoid pollution.
189-197: Proper security practice with URL sanitization.Good security implementation to prevent password exposure in logs and responses by sanitizing database URLs.
|
|
Mostly LGTM. cc: @vishwab1 |
| dbStatus.put("status", "UP"); | ||
| dbStatus.put("database", connection.getMetaData().getDatabaseProductName()); | ||
| dbStatus.put("driver", connection.getMetaData().getDriverName()); | ||
| dbStatus.put("url", sanitizeUrl(connection.getMetaData().getURL())); |
There was a problem hiding this comment.
We don't need to expose this to public.
|
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
53-55: Fix ambiguous RedisTemplate injection; prefer StringRedisTemplate to avoid NoUniqueBeanDefinitionExceptionWith multiple RedisTemplate beans in the context, field autowiring by type remains ambiguous even with required=false. Inject StringRedisTemplate (auto-configured and unique) to eliminate ambiguity and align with String value operations used in your check.
Apply this diff:
- @Autowired(required = false) - private RedisTemplate<String, Object> redisTemplate; + @Autowired(required = false) + private StringRedisTemplate stringRedisTemplate;And add the missing import (see import diff below).
🧹 Nitpick comments (5)
src/main/java/com/iemr/admin/service/health/HealthService.java (5)
24-31: Optional: import Duration instead of using FQCNOnly needed if you adopt the TTL change above.
import java.sql.PreparedStatement; import java.sql.ResultSet; import java.time.Instant; +import java.time.Duration; import java.util.HashMap; import java.util.Map;
88-89: Reduce log verbosity for frequently-polled endpointHealth endpoints are often polled. Logging at INFO on every call can flood logs. Switch to DEBUG.
- logger.info("Health check completed - Overall status: {}", overallHealth ? "UP" : "DOWN"); + logger.debug("Health check completed - Overall status: {}", overallHealth ? "UP" : "DOWN");
56-90: Separate liveness and readiness; avoid exposing detailed internals on public /healthCurrent payload includes DB/Redis details. PR objective mentions a lightweight “status: UP” health endpoint; also, a previous comment said “We don't need to expose this to public.” Consider:
- Keep /health minimal (liveness): only status and timestamp.
- Provide /health/ready or a secured endpoint for detailed checks (DB/Redis).
- Or gate inclusion of the "services" map behind a property.
If you want a quick toggle via property, apply:
@Value("${app.version:unknown}") private String appVersion; + @Value("${health.details.enabled:false}") + private boolean healthDetailsEnabled;and:
- healthStatus.put("services", services); + if (healthDetailsEnabled) { + healthStatus.put("services", services); + }Please confirm whether /health is intended to be publicly accessible and if detailed service statuses should be suppressed by default. I can follow up with a small controller change to split endpoints if needed.
41-46: Consider leveraging Spring Boot Actuator HealthIndicatorsSpring Boot Actuator provides production-grade /actuator/health with DB and Redis indicators, standardized status codes, and config-driven detail toggles. Migrating reduces custom code and maintenance.
If Actuator is already on the classpath, you can:
- Expose health endpoint and include Redis/DB indicators via properties.
- Optionally add a custom HealthIndicator if extra checks are still needed.
47-55: Prefer constructor injection over field injectionConstructor injection improves immutability, makes dependencies explicit, and simplifies testing.
I can provide a small refactor to constructor-based injection if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/health/HealthService.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
src/main/java/com/iemr/admin/service/version/VersionService.java (1)
Service(40-154)
🔇 Additional comments (2)
src/main/java/com/iemr/admin/service/health/HealthService.java (2)
142-146: Use StringRedisTemplate for ping executionEnsure consistency with the injected bean and avoid serializer surprises.
- String pong = redisTemplate.execute((RedisCallback<String>) connection -> { - return connection.ping(); - }); + String pong = stringRedisTemplate.execute((RedisCallback<String>) connection -> connection.ping());Likely an incorrect or invalid review comment.
37-39: Add missing import for StringRedisTemplateRequired for the suggested change to inject the dedicated string template.
import org.springframework.data.redis.core.RedisCallback; import org.springframework.data.redis.core.RedisTemplate; +import org.springframework.data.redis.core.StringRedisTemplate; import org.springframework.stereotype.Service;Likely an incorrect or invalid review comment.
| if (redisTemplate != null) { | ||
| Map<String, Object> redisStatus = checkRedisHealth(); | ||
| services.put("redis", redisStatus); | ||
| if (!"UP".equals(redisStatus.get("status"))) { | ||
| overallHealth = false; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update null-check to use StringRedisTemplate
Continue the variable rename to the string template to avoid NPEs and mismatches.
- if (redisTemplate != null) {
+ if (stringRedisTemplate != null) {
Map<String, Object> redisStatus = checkRedisHealth();
services.put("redis", redisStatus);
if (!"UP".equals(redisStatus.get("status"))) {
overallHealth = false;
}
} else {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/service/health/HealthService.java around lines
69 to 75, the null-check and subsequent usage still reference redisTemplate
instead of the renamed stringRedisTemplate which can cause NPEs/mismatches;
update the conditional and any calls to use stringRedisTemplate (e.g., if
(stringRedisTemplate != null) { Map<String,Object> redisStatus =
checkRedisHealth(); ... }) and ensure checkRedisHealth and related
methods/fields accept and use stringRedisTemplate consistently.
| // Additional test: set and get a test key | ||
| String testKey = "health:check:" + System.currentTimeMillis(); | ||
| String testValue = "test-value"; | ||
|
|
||
| redisTemplate.opsForValue().set(testKey, testValue); | ||
| Object retrievedValue = redisTemplate.opsForValue().get(testKey); | ||
| redisTemplate.delete(testKey); // Clean up test key | ||
|
|
||
| if (testValue.equals(retrievedValue)) { | ||
| redisStatus.put("status", "UP"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix Redis value serialization mismatch; set TTL to avoid lingering keys on failure
When using RedisTemplate<String, Object>, retrievedValue may not be a String (e.g., JDK-serialized), causing equality to fail. Using StringRedisTemplate ensures String-in/String-out. Also, add a short TTL to prevent orphaned keys if delete is skipped due to an exception.
- // Additional test: set and get a test key
+ // Additional test: set and get a test key
String testKey = "health:check:" + System.currentTimeMillis();
String testValue = "test-value";
- redisTemplate.opsForValue().set(testKey, testValue);
- Object retrievedValue = redisTemplate.opsForValue().get(testKey);
- redisTemplate.delete(testKey); // Clean up test key
+ stringRedisTemplate.opsForValue().set(testKey, testValue, java.time.Duration.ofSeconds(5));
+ String retrievedValue = stringRedisTemplate.opsForValue().get(testKey);
+ stringRedisTemplate.delete(testKey); // Clean up test key
- if (testValue.equals(retrievedValue)) {
+ if (testValue.equals(retrievedValue)) {Note: If you prefer an import over FQCN, add import java.time.Duration; (see import diff suggestion).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/service/health/HealthService.java around lines
150 to 159, the health-check writes and reads a test key using
RedisTemplate<String, Object>, which can return a non-String (JDK-serialized)
value and break equality; replace or cast to a String-safe API by using
StringRedisTemplate (or ensure the template is configured for String
serialization) so retrievedValue is a String, set the test key with a short TTL
(e.g., Duration.ofSeconds(5)) to avoid orphaned keys if delete fails, and ensure
the delete is performed in a finally block or guarded catch so cleanup is
attempted even on exceptions; add import java.time.Duration if you switch to
using Duration.
|
/health API is fine.
|
| } | ||
|
|
||
| private void addGitInformation(Map<String, String> versionInfo) { | ||
| Properties gitProps = loadPropertiesFile("git.properties"); |
There was a problem hiding this comment.
When does this file get generated? Will this always be reliably generated during build phase.?
|
Sure will fix it. |
|
@drtechie please review. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
38-47: Remove hardcoded URL exclusions (duplicate concern).The
/healthand/versionpaths are already configured as excluded URLs inSecurityFilterConfig. These hardcoded checks are redundant and violate the DRY principle. If the filter configuration is correct, this filter should not execute for those endpoints at all, making this code unreachable.Apply this diff to remove the hardcoded checks:
- String path = request.getRequestURI(); - String contextPath = request.getContextPath(); - - // FIRST: Check for health and version endpoints - skip ALL processing - if (path.equals("/health") || path.equals("/version") || - path.equals(contextPath + "/health") || path.equals(contextPath + "/version")) { - logger.info("Skipping JWT validation for monitoring endpoint: {}", path); - filterChain.doFilter(servletRequest, servletResponse); - return; - } - String origin = request.getHeader("Origin");Then retrieve
pathandcontextPathat line 64 where they're actually needed:response.setStatus(HttpServletResponse.SC_OK); return; } + + String path = request.getRequestURI(); + String contextPath = request.getContextPath(); logger.info("JwtUserIdValidationFilter invoked for path: " + path);src/main/java/com/iemr/admin/service/health/HealthService.java (3)
53-54: Unresolved: Bean ambiguity and type mismatch for Redis template.This issue was flagged in a previous review but remains unaddressed. Multiple
@Beanmethods namedredisTemplateexist (in both RoleMasterApplication and RedisConfig), causing injection ambiguity. Additionally,RedisTemplate<String, Object>uses Object serialization which breaks String equality checks in lines 154-158.Apply this diff to fix both issues:
- @Autowired(required = false) - private RedisTemplate<String, Object> redisTemplate; + @Autowired(required = false) + private StringRedisTemplate stringRedisTemplate;Then update all usages of
redisTemplatetostringRedisTemplatethroughout the file (lines 69, 70, 143, 154-156).
97-104: Unresolved: DB health check lacks proper timeout configuration.A previous review recommended shortening the connection validity timeout and adding a query timeout, but this remains unaddressed. Without these safeguards, slow database connections can cause the health endpoint to hang.
Apply this diff:
- boolean isConnectionValid = connection.isValid(5); // 5 second timeout + boolean isConnectionValid = connection.isValid(2); // 2 second timeout if (isConnectionValid) { // Execute a simple query to ensure database is responsive - try (PreparedStatement stmt = connection.prepareStatement(DB_HEALTH_CHECK_QUERY); - ResultSet rs = stmt.executeQuery()) { + try (PreparedStatement stmt = connection.prepareStatement(DB_HEALTH_CHECK_QUERY)) { + stmt.setQueryTimeout(3); // 3 second timeout + try (ResultSet rs = stmt.executeQuery()) { if (rs.next() && rs.getInt(1) == 1) { long responseTime = System.currentTimeMillis() - startTime; dbStatus.put("status", "UP"); dbStatus.put("responseTime", responseTime + "ms"); dbStatus.put("message", "Database connection successful"); logger.debug("Database health check: UP ({}ms)", responseTime); } else { dbStatus.put("status", "DOWN"); dbStatus.put("message", "Database query returned unexpected result"); logger.warn("Database health check: Query returned unexpected result"); } + } }
150-159: Unresolved: Redis serialization mismatch and missing TTL.A previous review identified that
RedisTemplate<String, Object>causes serialization mismatches (line 158 equality check fails becauseretrievedValueis JDK-serialized, not a String) and that the test key lacks a TTL (line 156), which can leak memory ifdeleteis skipped due to exceptions. This issue remains unaddressed.Apply this diff (assumes field renamed to
stringRedisTemplateper earlier comment):// Additional test: set and get a test key String testKey = "health:check:" + System.currentTimeMillis(); String testValue = "test-value"; - redisTemplate.opsForValue().set(testKey, testValue); - Object retrievedValue = redisTemplate.opsForValue().get(testKey); - redisTemplate.delete(testKey); // Clean up test key + stringRedisTemplate.opsForValue().set(testKey, testValue, java.time.Duration.ofSeconds(5)); + String retrievedValue = stringRedisTemplate.opsForValue().get(testKey); + stringRedisTemplate.delete(testKey); // Clean up test key if (testValue.equals(retrievedValue)) {Add import if needed:
+import java.time.Duration;
🧹 Nitpick comments (3)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
176-181: Remove error type from health check response.While the error message is now generic (good), exposing
error.getClass().getSimpleName()(line 181) still leaks implementation details about the Redis client library being used, which can aid reconnaissance.Apply this diff:
} catch (Exception e) { long responseTime = System.currentTimeMillis() - startTime; redisStatus.put("status", "DOWN"); redisStatus.put("message", "Redis connection failed"); redisStatus.put("responseTime", responseTime + "ms"); - redisStatus.put("error", e.getClass().getSimpleName()); logger.error("Redis health check failed", e); }The full exception details remain in server logs for debugging while the public response stays generic.
src/main/java/com/iemr/admin/service/version/VersionService.java (2)
62-71: Consider removing deprecated method if unused.The deprecated
getVersionInformation()method is retained for backward compatibility. If there are no existing callers, consider removing it to simplify the codebase.Run this script to check for usages:
#!/bin/bash # Check for usages of deprecated getVersionInformation() method rg -nP --type=java -C3 '\bgetVersionInformation\s*\(' -g '!VersionService.java'
133-137: Consider using java.time API for better maintainability.Creating a fresh
SimpleDateFormaton each call is thread-safe but dated. Consider usingjava.time.ZonedDateTimeandDateTimeFormatterfor cleaner, more efficient code.private String getCurrentIstTimeFormatted() { - SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); - sdf.setTimeZone(TimeZone.getTimeZone("Asia/Kolkata")); - return sdf.format(new Date()); + return java.time.ZonedDateTime.now(java.time.ZoneId.of("Asia/Kolkata")) + .format(java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/iemr/admin/controller/health/HealthController.java(1 hunks)src/main/java/com/iemr/admin/controller/version/VersionController.java(1 hunks)src/main/java/com/iemr/admin/service/health/HealthService.java(1 hunks)src/main/java/com/iemr/admin/service/version/VersionService.java(1 hunks)src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/admin/controller/health/HealthController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T11:55:39.499Z
Learnt from: 5Amogh
Repo: PSMRI/Admin-API PR: 110
File: src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java:153-166
Timestamp: 2025-11-17T11:55:39.499Z
Learning: In the Admin-API Java codebase, small self-contained utility methods like `isOriginAllowed` can be tolerated as duplicates across classes (e.g., HTTPRequestInterceptor and JwtUserIdValidationFilter) if it improves local readability and avoids unnecessary abstraction.
Applied to files:
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🧬 Code graph analysis (3)
src/main/java/com/iemr/admin/service/version/VersionService.java (1)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
Service(41-188)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
src/main/java/com/iemr/admin/service/version/VersionService.java (1)
Service(40-167)
src/main/java/com/iemr/admin/controller/version/VersionController.java (1)
src/main/java/com/iemr/admin/controller/health/HealthController.java (1)
RestController(38-60)
🔇 Additional comments (5)
src/main/java/com/iemr/admin/controller/version/VersionController.java (1)
42-52: LGTM! Clean refactoring that addresses previous feedback.The controller now properly delegates to
VersionService, and returningMap<String, String>directly aligns with drtechie's feedback to remove unnecessary wrapper fields (statusCode, ErrorMessage, status). The separation of concerns is much improved.src/main/java/com/iemr/admin/service/version/VersionService.java (4)
45-56: Well-structured approach to version retrieval.The use of
@Value("${app.version:unknown}")with a default fallback properly externalizes version configuration, avoiding hardcoded values. The service method cleanly delegates to the internal builder.
139-151: LGTM! Proper resource management.The use of try-with-resources ensures the
InputStreamis closed properly, and returningnullon failure allows callers to handle missing files gracefully with fallbacks.
153-166: Error handling is sound.The double try-catch provides robust fallback behavior. Note that this method is only called by the deprecated
getVersionInformation(), so if that method is removed, this can be deleted as well.
88-104: The git.properties file is properly generated during build via Maven configuration.The project already has git-commit-id-maven-plugin (v4.9.10) configured in pom.xml with generateGitPropertiesFile enabled and the file being written to the standard location. However, note that the plugin is configured to include only
git.commit.idandgit.build.time. Your code's fallback properties (git.commit.id.abbrev,git.commit.time,git.commit.timestamp) won't be available in the generated file, though the graceful null-handling on lines 100-102 ensures the application still functions.
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
Outdated
Show resolved
Hide resolved
- Simplify /health to return minimal response {status: UP/DOWN}
- Make Maven properties path configurable via @value annotation
- Remove exception message exposure in JWT filter error response
- Remove unused methods and imports
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/service/version/VersionService.java (1)
136-140: Consider using DateTimeFormatter for better performance.While the current approach is thread-safe (a new
SimpleDateFormatinstance is created on each call), it's inefficient.DateTimeFormatteris thread-safe, can be declared as a static constant, and follows modern Java best practices.🔎 Refactor to use DateTimeFormatter
+import java.time.ZonedDateTime; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; + @Service public class VersionService { private static final Logger logger = LoggerFactory.getLogger(VersionService.class); + private static final DateTimeFormatter IST_FORMATTER = + DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss").withZone(ZoneId.of("Asia/Kolkata")); @Value("${app.version:unknown}") private String appVersion;private String getCurrentIstTimeFormatted() { - SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); - sdf.setTimeZone(TimeZone.getTimeZone("Asia/Kolkata")); - return sdf.format(new Date()); + return IST_FORMATTER.format(ZonedDateTime.now()); }src/main/java/com/iemr/admin/service/health/HealthService.java (1)
101-118: LGTM: Redis health check properly uses callback to prevent resource leaks.The implementation correctly uses
redisTemplate.execute()with a callback, which ensures the Redis connection is properly released back to the pool. This addresses the critical resource leak concern from past reviews. Exception details are logged but not exposed in the return value.Optional enhancement: Consider adding a timeout to the Redis operation for defense-in-depth, though PING is typically fast:
private boolean checkRedisHealthInternal() { long startTime = System.currentTimeMillis(); try { // Execute with a timeout wrapper if your Redis client supports it String pong = redisTemplate.execute((RedisCallback<String>) connection -> connection.ping()); if ("PONG".equals(pong)) { long responseTime = System.currentTimeMillis() - startTime; logger.debug("Redis health check: UP ({}ms)", responseTime); return true; } logger.warn("Redis health check: Ping returned unexpected response"); return false; } catch (Exception e) { logger.error("Redis health check failed: {}", e.getMessage()); return false; } }Note: Redis command timeouts are typically configured at the connection factory level rather than per-operation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/admin/service/health/HealthService.javasrc/main/java/com/iemr/admin/service/version/VersionService.javasrc/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🔇 Additional comments (6)
src/main/java/com/iemr/admin/service/version/VersionService.java (4)
45-49: Past review feedback addressed.The configurable
mavenPropertiesPathwith@Valueannotation addresses the previous concern about the hard-coded Maven artifact path being brittle.Based on learnings, this resolves the feedback from coderabbitai[bot].
57-59: LGTM!Clean implementation that returns the version map directly, allowing the controller to serialize it without unnecessary wrapper fields.
65-74: LGTM!Proper deprecation pattern with clear guidance to use the newer
getVersionInfo()method.
91-107: Robust fallback handling for Git properties.The method correctly handles multiple property naming conventions and provides graceful degradation when
git.propertiesis not found.Regarding the previous question about when this file gets generated:
git.propertiesis generated during the Maven build phase by the git-commit-id-plugin (as indicated in the PR objectives).src/main/java/com/iemr/admin/service/health/HealthService.java (2)
51-73: Excellent implementation of minimal health response.The method now returns exactly what the PR objectives specified: a lightweight response with only
{"status": "UP"}or{"status": "DOWN"}. This fully addresses the major security concern from past reviews about exposing excessive internal details (database status, Redis status, response times, error types, timestamps) that could aid reconnaissance.The internal health checks are properly performed and logged for observability without exposing implementation details publicly.
75-99: LGTM: Database health check properly implemented.The implementation addresses all past review feedback:
- 2-second
isValid()timeout for fast failure- 3-second query timeout to prevent hanging
- Proper resource cleanup with try-with-resources
- Appropriate logging without exposing details in the return value
| @Autowired(required = false) | ||
| private RedisTemplate<String, Object> redisTemplate; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all RedisTemplate bean definitions and analyze duplicate beans
echo "=== Searching for RedisTemplate @Bean definitions ==="
rg -n -B 2 -A 5 '@Bean.*RedisTemplate|@Bean.*redisTemplate' --type java
echo -e "\n=== Searching for any RedisTemplate bean creation ==="
rg -n -B 2 -A 5 'RedisTemplate.*new.*|return.*RedisTemplate' --type java
echo -e "\n=== Checking RoleMasterApplication class ==="
fd 'RoleMasterApplication' --type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {}'
echo -e "\n=== Checking RedisConfig class ==="
fd 'RedisConfig' --type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {}'
echo -e "\n=== Checking application properties for Redis pool config ==="
fd 'application.*properties' --type f | xargs -I {} sh -c 'echo "=== {} ===" && rg "redis|pool" {} -i'
echo -e "\n=== Checking HealthService imports and annotation ==="
head -60 src/main/java/com/iemr/admin/service/health/HealthService.java | cat -nRepository: PSMRI/Admin-API
Length of output: 9443
Resolve duplicate RedisTemplate bean definitions.
Two @bean methods create redisTemplate beans with the same name in RoleMasterApplication and RedisConfig, causing Spring startup failure. HealthService cannot inject the correct bean without disambiguation.
Immediate fix: Add @Qualifier to specify the intended bean:
@Autowired(required = false)
+@Qualifier("redisTemplate")
private RedisTemplate<String, Object> redisTemplate;Long-term: Rename or remove one conflicting bean definition. RoleMasterApplication creates RedisTemplate<String, Object> (matches HealthService), while RedisConfig creates RedisTemplate<String, M_User>. Determine which is actually needed elsewhere and either rename one or consolidate the configurations.
Also add missing Redis connection pool configuration to application.properties:
spring.redis.lettuce.pool.max-active=8
spring.redis.lettuce.pool.max-idle=8
spring.redis.lettuce.pool.min-idle=0🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/service/health/HealthService.java around lines
48-49, the class autowires a RedisTemplate without disambiguation while two
beans with the same type/name exist, causing startup failures; update the field
injection to use @Qualifier referencing the exact bean name you intend (the one
declared in RoleMasterApplication or the one in RedisConfig) so Spring injects
the correct RedisTemplate, and as a long-term fix rename or remove the duplicate
bean definition (or change one bean name/type to RedisTemplate<String, M_User>
accordingly) to avoid conflicts; additionally add the suggested Redis Lettuce
pool properties to application.properties: spring.redis.lettuce.pool.max-active,
spring.redis.lettuce.pool.max-idle, and spring.redis.lettuce.pool.min-idle with
the provided values.
drtechie
left a comment
drtechie
left a comment
There was a problem hiding this comment.
@vishwab1 @vanitha1822
Please check this. LGTM.
3 participants
📋 Description
JIRA ID:
This PR resolve issue
This PR implements both the
/healthand/versionendpoints for the Admin API service as requested in issue #102What has been implemented:
/healthEndpoint:/healthendpoint that returns a minimal response with status "UP"/versionEndpoint:/versionendpoint that returns:git.commit.id)git.build.time)build.version)build.time)✅ Type of Change
ℹ️ Additional Information
Tested in Docker environment
Expected Responses:
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.