luigidemasi commented on PR #1898:
URL:
https://github.com/apache/camel-spring-boot/pull/1898#issuecomment-5423946601
## Review verdict: Request changes
Reviewed exact head `2f56679689c5811227e890dd8ee5b3ea173a5a7d` against
base `438b015ea3aaff65cca047a318f9385e835bf18e`.
### Findings
#### [P1] Do not silently redefine the established default health-exposure
contract
[`CamelHealthHelper.java:62-82`](https://github.com/apache/camel-spring-boot/blob/2f56679689c5811227e890dd8ee5b3ea173a5a7d/core/camel-spring-boot/src/main/java/org/
apache/camel/spring/boot/actuate/health/CamelHealthHelper.java#L62-L82)
CAMEL-18832 introduced this helper to align Spring Boot with MicroProfile
health. MicroProfile’s default-exposure regression still explicitly requires
`error.stacktrace`, while Camel’s documentation says `default` includes
detailed information for failed checks.
This PR removes that longstanding JSON field only from Spring Boot and
characterizes its placement as accidental without acknowledging the
compatibility change or cross-
runtime divergence.
References:
- [CAMEL-18832
implementation](https://github.com/apache/camel-spring-boot/commit/22a6e0429b9fc995594cf334298c7f617027d1e6)
- [Current MicroProfile default-exposure
test](https://github.com/apache/camel/blob/9b26b811fd2798bda130d441538cd339d8ccf4e2/components/camel-microprofile/camel-
microprofile-health/src/test/java/org/apache/camel/microprofile/health/CamelMicroProfileHealthCheckTest.java#L324-L363)
- [Exposure-level
documentation](https://github.com/apache/camel/blob/9b26b811fd2798bda130d441538cd339d8ccf4e2/docs/user-manual/modules/ROOT/pages/health-
check.adoc#L291-L299)
Either retain the established default behavior or explicitly approve and
document the compatibility change, including the resulting MicroProfile
divergence.
#### [P2] Preserve the message for every failing health check
[`CamelHealthHelper.java:63-77`](https://github.com/apache/camel-spring-boot/blob/2f56679689c5811227e890dd8ee5b3ea173a5a7d/core/camel-spring-boot/src/main/java/org/
apache/camel/spring/boot/actuate/health/CamelHealthHelper.java#L63-L77)
Default exposure can return multiple DOWN results. The indicator applies
them to one `Health.Builder`, but each result writes to the same top-level
`error.message`.
Spring Boot uses `Map.put`, so the last message replaces every earlier one.
An exact-head probe with two failures produced:
```text
error.message=second-message
first.data={error.type=IllegalArgumentException}
second.data={error.type=IllegalStateException}
```
`first-message` is lost. Before this PR, it remained available inside
`first.data.error.stacktrace`.
Store `error.message` inside each check-scoped data map—retaining the
top-level key if compatibility requires it—and add a regression test with two
distinct DOWN
results.
References:
- [Indicator
loop](https://github.com/apache/camel-spring-boot/blob/2f56679689c5811227e890dd8ee5b3ea173a5a7d/core/camel-spring-boot/src/main/java/org/apache/camel/
spring/boot/actuate/health/CamelHealthCheckIndicator.java#L52-L69)
- [Spring Boot detail replacement
semantics](https://github.com/spring-projects/spring-boot/blob/v4.1.0/module/spring-boot-health/src/main/java/org/springframework/boot/
health/contributor/Health.java#L231-L241)
#### [P2] Establish the asynchronous route-failure precondition in the test
[`CamelRoutesEndpointStartExceptionTest.java:33-70`](https://github.com/apache/camel-spring-boot/blob/2f56679689c5811227e890dd8ee5b3ea173a5a7d/core/camel-spring-boot/
src/test/java/org/apache/camel/spring/boot/actuate/endpoint/CamelRoutesEndpointStartExceptionTest.java#L33-L70)
The supervising controller starts asynchronously after a configured 100 ms
delay. The tests immediately serialize the route and only assert that
`route.start.exception`
is absent.
If serialization occurs before the failed start installs that property,
both tests pass against the unfixed implementation without exercising the leak.
Use Awaitility with an explicit `atMost` timeout to:
1. Wait until the raw route properties contain a `Throwable` under
`route.start.exception`.
2. Build the endpoint view.
3. Verify serialization omits that key while preserving benign properties.
#### [P3] Remove or implement the server-log guarantee
[`CamelHealthHelper.java:66-76`](https://github.com/apache/camel-spring-boot/blob/2f56679689c5811227e890dd8ee5b3ea173a5a7d/core/camel-spring-boot/src/main/java/org/
apache/camel/spring/boot/actuate/health/CamelHealthHelper.java#L66-L76)
The comment states that the full trace remains in the server log, but
neither the helper nor its caller logs or attaches the throwable using
`down(Throwable)`.
Spring Boot only logs exceptions attached to its builder or thrown by the
indicator. Some Camel health checks merely store exceptions in their result, so
the trace is
not guaranteed to exist in server logs.
[Spring Boot exception-logging
path](https://github.com/spring-projects/spring-boot/blob/v4.1.0/module/spring-boot-health/src/main/java/org/springframework/boot/health/
contributor/AbstractHealthIndicator.java#L76-L93)
Remove or qualify the claim unless intentional, non-repetitive logging is
implemented.
### Verification
- Exact PR head: both new test classes passed — 5 tests, 0 failures.
- The same tests applied to the base produced the two intended regression
failures.
- All hosted CI checks currently pass.
- The route DTO implementation itself behaves correctly under Jackson 2
and Jackson 3.
- The inherited `getProperties()` remains source- and binary-callable.
- No code, PR metadata, comments, or reviews were modified during this
review.
This review does not replace static analysis or specialized review tooling.
_Generated by Codex on behalf of Luigi De Masi. This review was generated
by an AI agent and may contain inaccuracies. Please verify all suggestions
before applying._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
