CAMEL-24477: camel-cxf - add a muteException consumer option - #25742
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed against the project's rules/conventions and verified behaviorally (not just diff-read): built and ran the full affected test suite (CxfConsumerMuteExceptionTest, CxfConsumerContinuationTimeoutTest, CxfConsumerFaultTest, CxfConsumerPayloadFaultTest, CxfCustomizedExceptionTest, JaxWsWebFaultAnnotationToFaultTest) in an isolated worktree against the PR branch — all 13 pass. Also traced the checkFailure/isMuted control flow to confirm the continuationTimeout path genuinely routes through muting via ExchangeTimedOutException (not a @WebFault), matching the PR description.
Confirmed, no issues:
- The
checkFailurerefactor (splitting the oldextractThrowableinto a directgetException()check +extractFromBody) is behavior-preserving for all three "always returned in full" paths: CXFFault/SoapFault,@WebFault-annotated exceptions, and aThrowable/CxfPayloadfault set as the body. @UriParam(label = "consumer", defaultValue = "true")with the description on the setter's Javadoc matches this file's existing convention (skipFaultLogging,mtomEnabled) and mirrors thecamel-http-commonmuteExceptionprecedent from CAMEL-23651. Nosecurity = "insecure:*"needed since this is secure-by-default, not an opt-in risk flag.- Test coverage is solid: covers muted/unmuted/declared-fault/default-value, and
CxfConsumerContinuationTimeoutTestwas correctly narrowed to only the affected route rather than papered over. - All generated artifacts (catalog JSON,
CxfEndpointConfigurer,CxfEndpointUriFactory,CxfEndpointBuilderFactory) are regenerated and included. - Upgrade guide entry on
maincorrectly flags the continuation-timeout diagnostic regression as a behavior change worth a reviewer's eye. - JIRA CAMEL-24477 is correctly typed (Improvement), assigned to the author, In Progress, fixVersion
4.23.0. - Scope is tight to
camel-cxf-soap; the addedassertj-coretest dependency is justified.
No blocking issues. One non-blocking note left inline.
This review does not replace specialized tools like CodeRabbit/Sourcery or static analysis (SonarCloud) — it's a rules/conventions + behavioral-verification pass.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| */ | ||
| private boolean isMuted(Throwable t) { | ||
| return ((CxfEndpoint) getEndpoint()).isMuteException() | ||
| && t.getClass().getAnnotation(WebFault.class) == null; |
There was a problem hiding this comment.
Minor, non-blocking: getAnnotation(WebFault.class) only sees the annotation when declared directly on the exception's own class — @WebFault isn't meta-annotated @Inherited in the JAX-WS spec. If a generated @WebFault exception were ever subclassed without repeating the annotation, muting would incorrectly apply to it. In practice CXF-generated fault classes are essentially never subclassed, so this is a theoretical edge case rather than a real risk — just flagging it for awareness, not requesting a change.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 14 tested, 25 compile-only — current: 11 all testedMaveniverse Scalpel detected 39 affected modules (current approach: 11).
|
221b1b5 to
8bedf39
Compare
CxfConsumer built the SOAP fault from whatever reached Exchange.getException(), so the exception's message - or its class name when it had none - became the faultstring returned to the caller. The http consumers gained a muteException option defaulting to true in CAMEL-23651; camel-cxf had none. Muting is narrower here than in the other consumers because for a SOAP service some faults are the contract. Only an undeclared failure is replaced: a CXF Fault, an exception annotated @webfault, and a Throwable or <soap:Fault> CxfPayload set as the message body are all still returned in full. checkFailure now separates the exception path from the body path to make that distinction, which retires extractThrowable. The stand-in has its stack trace cleared, since CXF can be configured to put the stack trace in the fault detail. Note this also covers framework failures: a continuationTimeout expiry no longer reports its timeout to the caller, so CxfConsumerContinuationTimeoutTest sets muteException=false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8bedf39 to
2d24242
Compare
3 participants
Fixes CAMEL-24477.
The CXF consumer describes route failures in the SOAP fault
CxfConsumer.checkFailure()takes whatever is onExchange.getException()and builds a CXFFaultfromit, so the exception's message becomes the SOAP
faultstring— and its class name does, when it has nomessage:
Measured against the unfixed code, a client of a failing
cxf:route receives:CAMEL-23651 aligned the HTTP consumers on a
muteExceptionoption defaulting totrue.camel-cxfhas no equivalent.The change, and why it is narrower than the other consumers
A
muteExceptionconsumer option onCxfEndpoint, defaulting totrue. Unlike knative/mina, it cannotapply to every fault — for a SOAP service, some faults are the contract. Muting those would break every
client written against the WSDL rather than protect anything.
So muting applies only to an undeclared failure: something on
Exchange.getException()whose classcarries no
@WebFaultannotation. These are all still returned in full:Fault/SoapFaultthrown by the route@WebFaultThrowableor<soap:Fault>CxfPayloadset as the message bodycheckFailurenow separates the exception path from the body path to make that distinction, which alsolet
extractThrowablego — it existed only to merge the two.The muted stand-in has its stack trace cleared, since CXF can be configured to put the stack trace in the
fault detail.
Behaviour change worth a reviewer's eye
Muting covers framework failures too, not just route exceptions. A
continuationTimeoutexpirypreviously returned
The OUT message was not received within: 5000 millis.and now returns the genericfault.
CxfConsumerContinuationTimeoutTestasserts that message, so it now setsmuteException=false—the same migration a deployment relying on that diagnostic performs. The upgrade guide calls this out
specifically.
I kept it muted for consistency with the rest of the family, but this is the one place where the default
costs real diagnostic signal — happy to special-case timeouts if the project would rather.
Testing
CxfConsumerMuteExceptionTestcovers all four behaviours:SOAPFaultException: the-internal-detail-a-caller-must-not-seeas the actual value;muteException=falsedescribes it as before;@WebFault(PingMeFault) is still returned in full while muting is on — the carve-outthat makes the default safe;
true.Cross-checks that the carve-out is correctly scoped rather than inert:
CxfCustomizedExceptionTest,CxfConsumerFaultTest,CxfConsumerPayloadFaultTestandJaxWsWebFaultAnnotationToFaultTestall passunchanged — they throw
SoapFault/@WebFault, which take the preserved branches.camel-cxf-soap166/166 (2 pre-existing skips) and a fullmvn clean install -DskipTestsacross the reactor.camel-cxf-soapdid not declareassertj-core; added withtestscope.Scope
Third of four consumers split out of CAMEL-24428 (rescoped to
camel-knative,#25685). Siblings:
CAMEL-24476 (mina, built) and
CAMEL-24478 (grpc, remaining).
Claude Code on behalf of oscerd