feat: Implement Observability, OPS Features, and API Documentation #10
Conversation
- Add dedicated Observability & Ops features (Actuator, Structured Logging) - Implement centralized Correlation ID propagation - Fix feign dependency issues in people-service - Fix Docker database configuration with application-docker.properties - Add Swagger/OpenAPI documentation for both services - Add comprehensive API Test Plan
📝 WalkthroughWalkthroughAdds observability and documentation support: correlation ID propagation (HTTP filter + Feign interceptor), centralized exception handling, structured JSON logging, OpenAPI configuration, dependency updates in common-lib and services, and related documentation and Docker/profile config changes. Changes
Sequence Diagram(s)sequenceDiagram
%% Participants
actor Client
participant Filter as CorrelationIdFilter
participant MDC as MDC Context
participant Service as Business Service
participant Feign as Feign Client
participant Interceptor as CorrelationIdInterceptor
participant Downstream as Downstream Service
participant Handler as GlobalExceptionHandler
Client->>Filter: HTTP request (optional X-Correlation-ID)
alt header missing
Filter->>Filter: generate UUID
end
Filter->>MDC: put(correlationId)
Filter->>Service: chain.doFilter(request,response)
rect rgb(230,240,255)
Note over Service: Business logic executes\n(logs read correlationId from MDC)
Service->>Feign: outbound call
Feign->>Interceptor: apply(RequestTemplate)
Interceptor->>MDC: read(correlationId)
Interceptor->>Feign: set X-Correlation-ID header
Feign->>Downstream: HTTP request (with X-Correlation-ID)
Downstream-->>Feign: response
Feign-->>Service: response
end
alt success
Service-->>Filter: return response
Filter->>Client: response + X-Correlation-ID header
else exception
Service->>Handler: exception thrown
Handler->>MDC: read(correlationId)
Handler->>Handler: build ErrorResponse (message, correlationId, status, timestamp)
Handler-->>Client: JSON ErrorResponse
end
Filter->>MDC: clear correlationId (finally)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 6
Fix all issues with AI Agents 🤖
In @common-lib/pom.xml:
- Around line 35-39: The pom's logstash-logback-encoder dependency (groupId
net.logstash.logback, artifactId logstash-logback-encoder) is pinned to
vulnerable version 7.4; update its <version> to 9.0 or later to mitigate the
cited RCE CVEs, then run mvn dependency:tree to verify transitive logback-core
is upgraded and execute the test suite to catch any behavioral/API changes; if
you use encoder-specific configuration or appenders, review and adjust code
referencing LogstashEncoder/LogstashLayout or related config keys because
major-version changes may introduce incompatible API/format changes.
In
@common-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.java:
- Around line 20-23: The handler method handleRuntimeException in
GlobalExceptionHandler incorrectly maps all RuntimeException to HTTP 400; remove
or modify it so internal server errors still return 500. Replace this broad
RuntimeException handler with specific client-error handlers (e.g., add handlers
for IllegalArgumentException and MethodArgumentNotValidException) that call
buildErrorResponse(..., HttpStatus.BAD_REQUEST), and let uncaught
RuntimeException fall through to the generic 500 handler (or delete
handleRuntimeException entirely).
- Around line 15-23: Add logging to GlobalExceptionHandler so exceptions are not
swallowed: in handleAllExceptions(Exception ex) and
handleRuntimeException(RuntimeException ex) call the class logger to log the
exception and its stack trace before calling buildErrorResponse; instantiate a
Logger for GlobalExceptionHandler (e.g., using LoggerFactory) and log full
exception details at error level for handleAllExceptions and at warn or error
level for handleRuntimeException to ensure errors are recorded for
debugging/alerting.
In @people-management-service/pom.xml:
- Around line 50-54: The springdoc-openapi dependency currently uses version
2.3.0 which is outdated and vulnerable; update the
<groupId>org.springdoc</groupId>
<artifactId>springdoc-openapi-starter-webmvc-ui</artifactId> <version> to 3.0.1
(or the latest 3.0.x) in the pom, then rebuild and run a dependency/OSS scan
(Snyk/OSV/OSS Index) on the resultant dependency tree to verify no transitive
CVEs remain; if compilation or runtime breakages occur, consult springdoc 3.0.x
migration notes and adjust imports/configuration in code paths that reference
springdoc APIs or auto-configuration.
In @tax-engine-service/src/main/resources/application.properties:
- Around line 8-11: Remove the actuator properties from the base
application.properties: delete management.endpoints.web.exposure.include,
management.endpoint.health.show-details, and management.info.env.enabled, then
place safer defaults into profile-specific files (e.g., add to
application-test.properties for tests only). For docker and production profiles
(e.g., application-docker.properties / prod profile), either omit
management.endpoints.web.exposure.include entirely or set
management.endpoint.health.show-details=when-authorized and ensure
management.info.env.enabled=false so environment variables are not exposed;
mirror the people-management-service approach for consistency.
🧹 Nitpick comments (12)
README.md (1)
23-23: Consider warning about data loss.The troubleshooting tip is helpful. However,
docker-compose down -vremoves all volumes and deletes persistent data. Consider adding a brief note that this will delete all data.🔎 Proposed enhancement
-> **Note**: If you face database errors like `database "peopledb" does not exist`, run `docker-compose down -v` to reset the volumes and restart. +> **Note**: If you face database errors like `database "peopledb" does not exist`, run `docker-compose down -v` to reset the volumes and restart. ⚠️ This will delete all persistent data.docs/API_TEST_PLAN.md (2)
13-28: Minor: Consider hyphenating compound adjective.The example payload and endpoint documentation are correct. As noted by static analysis, "Full-Time Employee" should be hyphenated when used as a compound adjective modifying a noun.
🔎 Proposed fix
-### 1.1 Create Person (Full Time Employee) +### 1.1 Create Person (Full-Time Employee)
81-87: Consider unchecking verification items for user execution.The verification checklist is comprehensive. However, since this is a test plan document, consider leaving the checkboxes unchecked
[ ]so users can check them off as they perform their own verification.🔎 Proposed change
## 🛠️ Verification Checklist -- [x] Swagger UI loads for both services. -- [x] `POST /people` creates data successfully. -- [x] `GET /people/{id}` retrieves correct data. -- [x] `POST /tax/calculate` returns valid tax computation. -- [x] `GET /tax/calculate/{id}` works and shows orchestration success. -- [x] Logs show matching `X-Correlation-ID` for the orchestrated call. +- [ ] Swagger UI loads for both services. +- [ ] `POST /people` creates data successfully. +- [ ] `GET /people/{id}` retrieves correct data. +- [ ] `POST /tax/calculate` returns valid tax computation. +- [ ] `GET /tax/calculate/{id}` works and shows orchestration success. +- [ ] Logs show matching `X-Correlation-ID` for the orchestrated call.common-lib/src/main/java/com/example/common/exception/ErrorResponse.java (1)
5-9: Consider using Instant or ZonedDateTime for explicit timezone handling.The record structure is clean and appropriate. However,
LocalDateTimedoesn't carry timezone information, which can be ambiguous in distributed systems. Consider usingInstant(always UTC) orZonedDateTimefor explicit timezone semantics.🔎 Proposed enhancement
Option 1: Using Instant (recommended for UTC timestamps)
-import java.time.LocalDateTime; +import java.time.Instant; public record ErrorResponse( String message, String correlationId, int status, - LocalDateTime timestamp) { + Instant timestamp) { }Option 2: Using ZonedDateTime (if timezone context is needed)
-import java.time.LocalDateTime; +import java.time.ZonedDateTime; public record ErrorResponse( String message, String correlationId, int status, - LocalDateTime timestamp) { + ZonedDateTime timestamp) { }tax-engine-service/src/main/resources/logback-spring.xml (2)
5-27: Refactor: Remove redundant timestamp configuration.The logging configuration mixes individual providers (
<timestamp>) with a composite pattern provider. The<timestamp>provider at lines 8-10 is redundant since timestamp is already defined within the JSON pattern at line 14. Consider removing the separate timestamp provider.🔎 Proposed fix
Option 1: Remove redundant timestamp provider (simpler)
<appender name="CONSOLE" class="ch.qos.logback.core.ConsoleAppender"> <encoder class="net.logstash.logback.encoder.LoggingEventCompositeJsonEncoder"> <providers> - <timestamp> - <timeZone>UTC</timeZone> - </timestamp> <pattern> <pattern> { "timestamp": "%d{yyyy-MM-dd HH:mm:ss.SSS}",Option 2: Use individual providers (more explicit)
<appender name="CONSOLE" class="ch.qos.logback.core.ConsoleAppender"> <encoder class="net.logstash.logback.encoder.LoggingEventCompositeJsonEncoder"> <providers> <timestamp> <timeZone>UTC</timeZone> </timestamp> - <pattern> - <pattern> - { - "timestamp": "%d{yyyy-MM-dd HH:mm:ss.SSS}", - "level": "%level", - "service": "tax-engine-service", - "correlationId": "%X{correlationId:-undefined}", - "thread": "%thread", - "logger": "%logger{36}", - "message": "%message", - "exception": "%ex" - } - </pattern> - </pattern> + <logLevel/> + <loggerName> + <shortenedLoggerNameLength>36</shortenedLoggerNameLength> + </loggerName> + <mdc/> + <threadName/> + <message/> + <stackTrace/> </providers> </encoder> </appender>
16-16: Optional: Consider using Spring property for service name.The hardcoded service name works correctly. For better maintainability, consider using a Spring property placeholder:
"service": "${spring.application.name:-tax-engine-service}". This allows configuration-driven service naming.people-management-service/src/main/java/com/example/javamigrationlab/modern/service/PersonService.java (1)
31-31: Consider guarding the log statement for consistency.The log statement lacks an
isInfoEnabled()guard, whileTaxCalculationService(lines 28-30) guards its log. The PR summary mentions addressing PMD "GuardLogStatement" violations, suggesting the project prefers explicit guards.🔎 Proposed fix for consistency
- log.info("Fetching person with ID: {}", id); + if (log.isInfoEnabled()) { + log.info("Fetching person with ID: {}", id); + }people-management-service/src/main/resources/logback-spring.xml (1)
7-24: Potential duplicate timestamp fields in JSON output.The configuration includes both a
<timestamp>provider (lines 8-10) and a"timestamp"field in the pattern (line 14). This may result in duplicate timestamp entries in the JSON output. Consider removing one:🔎 Proposed fix to remove duplicate timestamp
<encoder class="net.logstash.logback.encoder.LoggingEventCompositeJsonEncoder"> <providers> - <timestamp> - <timeZone>UTC</timeZone> - </timestamp> <pattern> <pattern> { - "timestamp": "%d{yyyy-MM-dd HH:mm:ss.SSS}", + "timestamp": "%d{yyyy-MM-dd HH:mm:ss.SSS, UTC}", "level": "%level",common-lib/src/main/java/com/example/common/config/OpenApiConfig.java (2)
12-20: Consider adding@ConditionalOnMissingBeanfor customization flexibility.Consuming services may want to provide their own
OpenAPIbean with service-specific metadata. Adding@ConditionalOnMissingBeanallows override without conflicts.🔎 Proposed fix
+import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; + @AutoConfiguration public class OpenApiConfig { @Bean + @ConditionalOnMissingBean public OpenAPI customOpenAPI() {
17-17: Hardcoded version may drift from project version.Consider injecting the version from
@Value("${spring.application.version:0.0.1-SNAPSHOT}")or using build-time substitution to keep documentation in sync with releases.common-lib/src/main/java/com/example/common/logging/CorrelationIdFilter.java (1)
12-13: Consider adding@Orderto ensure early filter execution.If other filters need the correlation ID in MDC (e.g., security filters for audit logging), an explicit ordering ensures this filter runs first.
🔎 Proposed fix
+import org.springframework.core.annotation.Order; +import org.springframework.core.Ordered; + @Component +@Order(Ordered.HIGHEST_PRECEDENCE) public class CorrelationIdFilter implements Filter {common-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.java (1)
25-32: Guard against null exception messages and potential info leakage.
ex.getMessage()can returnnull, and exposing raw exception messages may leak internal details (stack traces, SQL errors, etc.) to clients.🔎 Proposed fix
private ResponseEntity<ErrorResponse> buildErrorResponse(String message, HttpStatus status) { String correlationId = MDC.get(CorrelationIdFilter.CORRELATION_ID_LOG_VAR); + String safeMessage = (message != null) ? message : "An unexpected error occurred"; ErrorResponse error = new ErrorResponse( - message, + safeMessage, correlationId, status.value(), LocalDateTime.now()); return new ResponseEntity<>(error, status); }For production, consider returning generic messages for 500 errors while logging the actual exception.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.mdcommon-lib/pom.xmlcommon-lib/src/main/java/com/example/common/config/OpenApiConfig.javacommon-lib/src/main/java/com/example/common/exception/ErrorResponse.javacommon-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.javacommon-lib/src/main/java/com/example/common/logging/CorrelationIdFilter.javacommon-lib/src/main/java/com/example/common/logging/CorrelationIdInterceptor.javacommon-lib/src/main/java/com/example/common/logging/ObservabilityAutoConfiguration.javacommon-lib/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.importsdocs/API_TEST_PLAN.mddocs/DOCKER_SETUP_SUMMARY.mdpeople-management-service/pom.xmlpeople-management-service/src/main/java/com/example/javamigrationlab/exception/GlobalExceptionHandler.javapeople-management-service/src/main/java/com/example/javamigrationlab/modern/service/PersonService.javapeople-management-service/src/main/resources/application-docker.propertiespeople-management-service/src/main/resources/logback-spring.xmltax-engine-service/pom.xmltax-engine-service/src/main/java/com/example/tax/service/TaxCalculationService.javatax-engine-service/src/main/resources/application.propertiestax-engine-service/src/main/resources/logback-spring.xml
💤 Files with no reviewable changes (1)
- people-management-service/src/main/java/com/example/javamigrationlab/exception/GlobalExceptionHandler.java
🧰 Additional context used
🧬 Code graph analysis (6)
common-lib/src/main/java/com/example/common/logging/ObservabilityAutoConfiguration.java (1)
common-lib/src/main/java/com/example/common/config/OpenApiConfig.java (1)
AutoConfiguration(9-21)
tax-engine-service/src/main/java/com/example/tax/service/TaxCalculationService.java (1)
people-management-service/src/main/java/com/example/javamigrationlab/modern/service/PersonService.java (1)
Slf4j(14-94)
common-lib/src/main/java/com/example/common/logging/CorrelationIdFilter.java (1)
common-lib/src/main/java/com/example/common/logging/CorrelationIdInterceptor.java (1)
Component(8-18)
people-management-service/src/main/java/com/example/javamigrationlab/modern/service/PersonService.java (1)
tax-engine-service/src/main/java/com/example/tax/service/TaxCalculationService.java (1)
Slf4j(17-77)
common-lib/src/main/java/com/example/common/config/OpenApiConfig.java (1)
common-lib/src/main/java/com/example/common/logging/ObservabilityAutoConfiguration.java (1)
AutoConfiguration(8-17)
common-lib/src/main/java/com/example/common/logging/CorrelationIdInterceptor.java (1)
common-lib/src/main/java/com/example/common/logging/CorrelationIdFilter.java (1)
Component(12-39)
🪛 LanguageTool
docs/API_TEST_PLAN.md
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...lhost:8080 ### 1.1 Create Person (Full Time Employee) **Endpoint**:POST /peop...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (21)
people-management-service/src/main/resources/application-docker.properties (1)
1-7: Verify AI summary consistency and SQL initialization script configuration.The AI summary claims extensive configuration blocks were removed, but the code still contains the database and JPA configuration (lines 1–6). Only the new property
spring.sql.init.mode=always(line 7) is added. Please clarify the actual changes.Additionally,
spring.sql.init.mode=alwayswill execute SQL initialization scripts on every startup. Verify that:
- SQL initialization scripts are properly configured (e.g., under
resources/data.sqlorschema.sql)- The
alwaysmode is idempotent and appropriate for your deployment topology (e.g., no data loss or conflicts in multi-pod scenarios)- Alternative modes like
neverorembeddedaren't more suitable for your use caseREADME.md (2)
9-9: Good addition to documentation structure.The API Test Plan reference is well-placed and follows the existing documentation pattern.
59-60: LGTM - Checklist accurately reflects completed work.The checklist updates correctly mark the observability features as complete, aligning with the PR's implementation of Actuator endpoints, structured logging, and correlation ID tracking.
docs/API_TEST_PLAN.md (3)
1-9: LGTM - Clear introduction with helpful Swagger links.The document provides a clear purpose and convenient Swagger UI access points for both services.
30-51: LGTM - Comprehensive examples covering different person types.The contractor and retrieval examples provide good coverage of the API surface, with correct income calculation (1,200,000 / 12 = 100,000).
54-78: LGTM - Tax examples demonstrate both standalone and orchestrated flows.The examples effectively showcase both direct tax calculation and cross-service orchestration. The correlation ID verification note is particularly valuable for observability testing.
tax-engine-service/pom.xml (1)
48-52: LGTM - Lombok dependency properly configured.The Lombok dependency with
optionalscope is correctly configured for compile-time code generation. Version inheritance from the parent POM promotes consistency.tax-engine-service/src/main/resources/logback-spring.xml (1)
29-31: LGTM - Root logger configuration is appropriate.The INFO log level and CONSOLE appender configuration follow standard practices for production logging.
people-management-service/src/main/java/com/example/javamigrationlab/modern/service/PersonService.java (1)
6-6: LGTM! Lombok logging integration is clean.The addition of
@Slf4jannotation and import follows the standard Lombok pattern and aligns with the observability goals of this PR.Also applies to: 14-14
tax-engine-service/src/main/java/com/example/tax/service/TaxCalculationService.java (1)
8-8: LGTM! Proper implementation of guarded logging.The addition of
@Slf4jand the guarded log statement correctly addresses the PMD "GuardLogStatement" violation mentioned in the PR summary. TheisInfoEnabled()check prevents unnecessary object creation when INFO logging is disabled.Also applies to: 17-17, 28-30
people-management-service/pom.xml (2)
40-44: LGTM! H2 dependency configured correctly.The runtime scope is appropriate for an in-memory database driver used during testing and development.
45-48: No changes needed. The spring-cloud-starter-openfeign dependency is correctly aligned.The spring-cloud 2023.0.0 BOM (imported in the parent pom.xml) manages spring-cloud-starter-openfeign at version 4.1.0, which matches the explicit version in common-lib. Both modules will use version 4.1.0 with no version mismatch. The compile scope is appropriate for this dependency in people-management-service.
Likely an incorrect or invalid review comment.
common-lib/pom.xml (4)
30-34: LGTM! Appropriate use of provided scope.The
spring-boot-starter-webdependency withprovidedscope is correctly configured for a shared library module, as consuming services will supply their own web dependencies.
40-43: LGTM! Standard SLF4J API dependency.Version inheritance from the Spring Boot BOM is appropriate and ensures compatibility.
50-55: LGTM! Consistent SpringDoc version with appropriate scope.The
providedscope is correct for a library module, and version 2.3.0 matches the declaration inpeople-management-service/pom.xml, ensuring consistency.
44-49: No action needed. Spring Cloud OpenFeign version 4.1.0 is explicitly designed for Spring Boot 3.x compatibility (part of the Spring Cloud 4.x release line). The specified version is stable and compatible. If desired, version 4.1.1 (released March 2024) is available as a minor update.Likely an incorrect or invalid review comment.
common-lib/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports (1)
1-1: LGTM! Correct auto-configuration registration.The
ObservabilityAutoConfigurationis properly registered using the Spring Boot 3.x auto-configuration mechanism. This enables automatic wiring of observability components (CorrelationIdFilter, CorrelationIdInterceptor, GlobalExceptionHandler, OpenApiConfig) when the common-lib is on the classpath.people-management-service/src/main/resources/logback-spring.xml (1)
1-32: LGTM overall — structured logging configuration is well-designed.The JSON encoder setup with correlation ID from MDC, service name, and exception handling is appropriate for observability. The
correlationId:-undefinedfallback ensures logging works even without the filter.common-lib/src/main/java/com/example/common/logging/CorrelationIdInterceptor.java (1)
8-17: LGTM!The interceptor correctly propagates the correlation ID from MDC to outgoing Feign requests. The null check prevents adding empty headers, and using constants from
CorrelationIdFilterensures consistency.common-lib/src/main/java/com/example/common/logging/CorrelationIdFilter.java (1)
12-38: LGTM — solid implementation of correlation ID propagation.The filter correctly:
- Generates UUID when header is missing/empty
- Stores in MDC for logging
- Echoes to response header
- Cleans up MDC in
finallyblock (prevents leaks in servlet container thread pools)common-lib/src/main/java/com/example/common/logging/ObservabilityAutoConfiguration.java (1)
8-16: The pattern is correct; no duplicate bean registration risk.The imported classes are properly registered through Spring's auto-configuration mechanism via
META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports. Since consuming services use@SpringBootApplicationwithout explicitbasePackages, component scanning only covers their own packages (e.g.,com.example.javamigrationlab,com.example.tax). The common-lib beans are in a sibling package (com.example.common.*), so they won't be picked up by automatic component scanning. The@Importmechanism inObservabilityAutoConfigurationis the intended and only way these beans are registered, making this a safe, correct pattern for shared library auto-configuration.
common-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.java
Show resolved
Hide resolved
common-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.java
Outdated
Show resolved
Hide resolved
- Update logstash-logback-encoder from 7.4 to 9.0 (CVE mitigation) - Update springdoc-openapi from 2.3.0 to 3.0.1 (security patches) - Add logging to GlobalExceptionHandler with guarded log statements - Replace broad RuntimeException handler with specific handlers (IllegalArgumentException, IllegalStateException) - Remove actuator config from base application.properties in tax-engine-service - Add profile-specific actuator configs (docker, test) with safer defaults - Add sequence and entity diagrams to Architectural Blueprint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tax-engine-service/pom.xml (1)
43-47: Centralize springdoc version in parent POM's<dependencyManagement>section.While the version upgrade to 3.0.1 addresses the security concerns from the previous review, the version declaration still needs to be centralized in the parent POM's
<dependencyManagement>section (as requested in the previous review). Both this service and people-management-service use this dependency, so managing the version centrally will ensure consistency and simplify future upgrades.🔎 Recommended approach
- Add to parent POM's
<dependencyManagement>:<dependency> <groupId>org.springdoc</groupId> <artifactId>springdoc-openapi-starter-webmvc-ui</artifactId> <version>3.0.1</version> </dependency>
- Remove version from this file:
<dependency> <groupId>org.springdoc</groupId> <artifactId>springdoc-openapi-starter-webmvc-ui</artifactId> - <version>3.0.1</version> </dependency>
- Apply the same version removal to people-management-service/pom.xml.
people-management-service/pom.xml (1)
50-54: LGTM! SpringDoc OpenAPI upgraded to address CVEs.The upgrade to version 3.0.1 successfully addresses the security vulnerabilities (CVE-2025-61795, CVE-2025-55754, CVE-2025-55752) flagged in the previous review.
common-lib/pom.xml (1)
30-56: LGTM! Dependency additions support observability features.The dependencies are well-chosen to support the new observability infrastructure:
- logstash-logback-encoder v9.0 successfully addresses critical RCE vulnerabilities (CVE-2024-12798, CVE-2024-12801, CVE-2025-11226) from the previous v7.4.
- springdoc-openapi v3.0.1 provides up-to-date OpenAPI documentation capabilities.
- spring-cloud-starter-openfeign v4.1.0 enables correlation ID propagation via CorrelationIdInterceptor.
- Using
<scope>provided</scope>for web, OpenFeign, and SpringDoc is appropriate for a library that provides auto-configuration components consumed by downstream services.
🧹 Nitpick comments (1)
common-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.java (1)
36-42: Optional: Consider including exception details for IllegalStateException.While
IllegalStateExceptionoften indicates client-facing precondition violations, it can also signal unexpected server-side state issues. If debugging becomes challenging, consider logging the full exception:@ExceptionHandler(IllegalStateException.class) public ResponseEntity<ErrorResponse> handleIllegalStateException(IllegalStateException ex) { if (logger.isWarnEnabled()) { - logger.warn("Illegal state: {}", ex.getMessage()); + logger.warn("Illegal state: {}", ex.getMessage(), ex); } return buildErrorResponse(ex.getMessage(), HttpStatus.BAD_REQUEST); }This is optional and depends on how
IllegalStateExceptionis used in your codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
common-lib/pom.xmlcommon-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.javadocs/ARCHITECTURAL_BLUEPRINT.mdpeople-management-service/pom.xmltax-engine-service/pom.xmltax-engine-service/src/main/resources/application-docker.propertiestax-engine-service/src/main/resources/application-test.propertiestax-engine-service/src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (2)
- tax-engine-service/src/main/resources/application-test.properties
- tax-engine-service/src/main/resources/application.properties
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
tax-engine-service/src/main/resources/application-docker.properties (1)
10-12: Sound actuator configuration strategy for containerized environments.The actuator endpoint restrictions follow security best practices: exposing only
healthandinfoendpoints, gating detailed health information behind authorization, and disabling environment property exposure. This reduces the attack surface and prevents unintended information leakage in Docker/Kubernetes deployments.However, note that the PR objectives reference "correct PostgreSQL configuration" to fix Docker startup database issues, but this file shows only actuator and service endpoint configuration. If database-specific properties are needed here (e.g.,
spring.datasource.url,spring.datasource.hikari.*), they may be missing or handled via environment variables. Please verify that database connectivity is properly configured for the Docker environment—either in this file, via Docker environment variables, or through orchestration configuration.tax-engine-service/pom.xml (1)
49-53: Lombok version is inherited from Spring Boot parent BOM.The Lombok dependency is correctly configured with
<optional>true</optional>. The version is not explicitly declared in the local parent POM's<dependencyManagement>section because the root POM (people-tax-ecosystem) inherits fromspring-boot-starter-parent:3.2.0, which manages the Lombok version. This is a standard pattern and requires no changes.docs/ARCHITECTURAL_BLUEPRINT.md (1)
69-139: LGTM! Excellent documentation additions.The sequence diagram clearly illustrates the correlation ID propagation and service orchestration flow, while the class diagram effectively documents the sealed hierarchy design. These additions provide valuable architectural context for the observability features introduced in this PR.
common-lib/src/main/java/com/example/common/exception/GlobalExceptionHandler.java (1)
15-52: LGTM! Exception handler successfully addresses previous review feedback.The implementation demonstrates solid engineering practices:
- Guarded logging (lines 22, 30, 38) prevents expensive operations when logging is disabled.
- Specific exception handlers for
IllegalArgumentExceptionandIllegalStateExceptionreplace the previous overly-broadRuntimeExceptionhandler, providing appropriate HTTP status codes.- Stack trace included for unhandled exceptions (line 23) aids debugging.
- Correlation ID propagation via MDC (line 45) enables distributed tracing across services.
The warn-level logs for client errors (lines 31, 39) intentionally omit stack traces to reduce noise, which is a reasonable trade-off for known client error scenarios.
people-management-service/pom.xml (1)
45-48: Version resolution for spring-cloud-starter-openfeign is properly managed—no action needed.The dependency declaration is correct. The root pom's
<dependencyManagement>section importsspring-cloud-dependencies(version 2023.0.0) with<scope>import</scope>, which transitively manages the version forspring-cloud-starter-openfeign. The provided scope in common-lib does not affect version inheritance from the parent BOM; it only controls packaging behavior. The build will resolve the version successfully.Likely an incorrect or invalid review comment.
🎯 Objective
This PR implements comprehensive observability and operations features for the People and Tax Ecosystem. It addresses application startup issues, standardizes logging with distributed tracing (Correlation IDs), and provides complete API documentation via Swagger/OpenAPI.
✨ Key Changes
CorrelationIdFilterandCorrelationIdInterceptorto propagateX-Correlation-IDacross services.logstash-logback-encoderinlogback-spring.xmlfor both services.healthandinfoendpoints.BeanDefinitionStoreExceptioninpeople-management-serviceby adding missingspring-cloud-starter-openfeignruntime dependency.application-docker.propertieswith correct PostgreSQL configuration.TaxCalculationService.springdoc-openapito generate Swagger UI for both services.docs/API_TEST_PLAN.mdwith detailed manual verification steps and curl examples.README.mdwith troubleshooting tips for Docker.🛡️ Quality Checklist
mvn verifylocally and it passes.mvn test).README.mdor/docsif necessary.📸 Screenshots / Logs
Distributed Tracing Verification:
Logs showing matching Correlation ID across services (verified manually).
🔗 Related Issues
README.md.Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.