gnodet-bot commented on code in PR #29876: URL: https://github.com/apache/camel/pull/29876#discussion_r4573063198
########## components/camel-cxf/camel-cxf-soap/src/test/java/org/apache/camel/component/cxf/jaxws/CxfClientCallbackTest.java: ########## @@ -0,0 +1,396 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.cxf.jaxws; + +import java.net.URI; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.AsyncCallback; +import org.apache.camel.Exchange; +import org.apache.camel.component.cxf.common.CxfBinding; +import org.apache.camel.http.base.cookie.CookieHandler; +import org.apache.cxf.endpoint.ConduitSelector; +import org.apache.cxf.message.Message; +import org.apache.cxf.service.model.BindingOperationInfo; +import org.apache.cxf.service.model.OperationInfo; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link CxfClientCallback} verifying that {@code camelAsyncCallback.done(false)} is always called + * exactly once, and that exception handling preserves the root cause. + * + * CAMEL-26696 + */ +class CxfClientCallbackTest { + + private Exchange camelExchange; + private org.apache.cxf.message.Exchange cxfExchange; + private BindingOperationInfo boi; + private CxfEndpoint endpoint; + private CxfBinding cxfBinding; + + @BeforeEach + void setUp() { + camelExchange = mock(Exchange.class); + cxfExchange = mock(org.apache.cxf.message.Exchange.class); + boi = mock(BindingOperationInfo.class); + endpoint = mock(CxfEndpoint.class); + cxfBinding = mock(CxfBinding.class); + + OperationInfo operationInfo = mock(OperationInfo.class); + when(boi.getOperationInfo()).thenReturn(operationInfo); + when(operationInfo.isOneWay()).thenReturn(false); + + when(endpoint.getCxfBinding()).thenReturn(cxfBinding); + when(endpoint.getCookieHandler()).thenReturn(null); // no cookie handler + } + + // --- handleResponse tests --- + + @Test + void handleResponse_normalPath_callsDoneExactlyOnce() { + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + Map<String, Object> ctx = new HashMap<>(); + + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleResponse(ctx, new Object[0]); + + assertEquals(1, doneCount.get(), "done(false) must be called exactly once"); + } + + @Test + void handleResponse_bindingThrows_callsDoneAndSetsException() { + RuntimeException bindingError = new RuntimeException("binding failure"); + doThrow(bindingError).when(cxfBinding).populateExchangeFromCxfResponse(any(), any(), any()); + + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + // Capture the exception set on the exchange + AtomicReference<Throwable> captured = new AtomicReference<>(); + camelExchange = mock(Exchange.class); + doAnswer(inv -> { + captured.set((Throwable) inv.getArgument(0)); + return null; + }).when(camelExchange).setException(any(Throwable.class)); + + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleResponse(ctx, new Object[0]); + + assertEquals(1, doneCount.get(), "done(false) must be called even when binding throws"); + assertSame(bindingError, captured.get()); + } + + @Test + void handleResponse_bindingThrowsError_callsDoneAndSetsException() { + OutOfMemoryError bindingError = new OutOfMemoryError("simulated OOM in binding"); + doThrow(bindingError).when(cxfBinding).populateExchangeFromCxfResponse(any(), any(), any()); + + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + AtomicReference<Throwable> captured = new AtomicReference<>(); + camelExchange = mock(Exchange.class); + doAnswer(inv -> { + captured.set((Throwable) inv.getArgument(0)); + return null; + }).when(camelExchange).setException(any(Throwable.class)); + + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleResponse(ctx, new Object[0]); + + assertEquals(1, doneCount.get(), "done(false) must be called even when binding throws an Error"); + assertSame(bindingError, captured.get()); + } + + // --- handleException tests --- + + @Test + void handleException_normalPath_callsDoneExactlyOnce() { + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + RuntimeException transportEx = new RuntimeException("connection refused"); + + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleException(ctx, transportEx); + + assertEquals(1, doneCount.get(), "done(false) must be called exactly once"); + verify(camelExchange).setException(transportEx); + } + + @Test + void handleException_withConduitSelector_usesResolvedExceptionFromOutMessage() { + // When a ConduitSelector is set and outMessage carries a richer exception, that takes priority + RuntimeException transportEx = new RuntimeException("connection refused"); + RuntimeException resolvedEx = new RuntimeException("resolved from outMessage"); + + ConduitSelector conduitSelector = mock(ConduitSelector.class); + Message outMessage = mock(Message.class); + when(cxfExchange.get(ConduitSelector.class)).thenReturn(conduitSelector); + when(cxfExchange.getOutMessage()).thenReturn(outMessage); + when(outMessage.getContent(Exception.class)).thenReturn(resolvedEx); + + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleException(ctx, transportEx); + + assertEquals(1, doneCount.get(), "done(false) must be called exactly once"); + verify(camelExchange).setException(resolvedEx); + } + + @Test + void handleException_withConduitSelector_noExceptionOnMessages_doesNotSetException() { + // When ConduitSelector is set and neither message carries an exception, the failover + // succeeded — do NOT fall back to the original transport exception, leave the exchange + // clean (failover already succeeded). + RuntimeException transportEx = new RuntimeException("connection refused"); + + ConduitSelector conduitSelector = mock(ConduitSelector.class); + Message outMessage = mock(Message.class); + when(cxfExchange.get(ConduitSelector.class)).thenReturn(conduitSelector); + when(cxfExchange.getOutMessage()).thenReturn(outMessage); + when(outMessage.getContent(Exception.class)).thenReturn(null); + when(cxfExchange.getInMessage()).thenReturn(null); + + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleException(ctx, transportEx); + + assertEquals(1, doneCount.get(), "done(false) must be called exactly once"); + // failover succeeded: setException must NOT be called (exchange should remain clean) + verify(camelExchange, never()).setException(any()); + } + + @Test + void handleException_bindingThrows_preservesRootCauseAndCallsDone() { + RuntimeException transportEx = new RuntimeException("connection refused"); + RuntimeException bindingEx = new RuntimeException("binding failure"); + doThrow(bindingEx).when(cxfBinding).populateExchangeFromCxfResponse(any(), any(), any()); + + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + // Simulate exchange already has the transport exception set + when(camelExchange.getException()).thenReturn(transportEx); + + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleException(ctx, transportEx); + + assertEquals(1, doneCount.get(), "done(false) must be called even when binding throws"); + // binding error should be suppressed on the root cause, not replace it + assertEquals(1, transportEx.getSuppressed().length); + assertSame(bindingEx, transportEx.getSuppressed()[0]); + } + + @Test + void handleException_bindingThrows_noRootCause_setsBindingException() { + RuntimeException bindingEx = new RuntimeException("binding failure"); + doThrow(bindingEx).when(cxfBinding).populateExchangeFromCxfResponse(any(), any(), any()); + + AtomicInteger doneCount = new AtomicInteger(); + AsyncCallback callback = sync -> doneCount.incrementAndGet(); + + // No prior exception on the exchange + when(camelExchange.getException()).thenReturn(null); + AtomicReference<Throwable> captured = new AtomicReference<>(); + doAnswer(inv -> { + captured.set((Throwable) inv.getArgument(0)); + return null; + }).when(camelExchange).setException(any(Throwable.class)); + + RuntimeException transportEx = new RuntimeException("connection refused"); + Map<String, Object> ctx = new HashMap<>(); + CxfClientCallback cb = new CxfClientCallback(callback, camelExchange, cxfExchange, boi, endpoint); + cb.handleException(ctx, transportEx); + + assertEquals(1, doneCount.get()); + // when there's no root cause, the binding exception should be set + assertSame(bindingEx, captured.get()); Review Comment: ⚠️ **Test does not cover the scenario it claims** The test name says "no root cause" and the comment says `// No prior exception on the exchange`, but in the production code path this test exercises (no `conduitSelector`), the `else` branch **always** runs `camelExchange.setException(ex)` before the binding `try` block: ```java } else { camelExchange.setException(ex); // ← always called; there IS a root cause after this } ``` After that call, `camelExchange.getException()` returns `transportEx` in production. The binding `catch` then hits `cur != null && cur != t → cur.addSuppressed(bindingEx)` — meaning `setException(bindingEx)` is never called, and `captured.get()` would hold `transportEx`, not `bindingEx`. The test only passes because `camelExchange.getException()` is stubbed with `thenReturn(null)` and does **not** update when `setException(transportEx)` is called — a mock state mismatch. The assertion `assertSame(bindingEx, captured.get())` would fail against a correctly wired mock. The actual "no root cause" scenario (where `setException` isn't called at all before the binding try) only occurs in the `conduitSelector` path when neither message carries an exception. That combination — `conduitSelector`, no resolved exception, binding throws — is not currently tested. **Fix option A** — wire the mock state correctly so the test reflects production: ```java AtomicReference<Throwable> captured = new AtomicReference<>(); doAnswer(inv -> { captured.set((Throwable) inv.getArgument(0)); return null; }) .when(camelExchange).setException(any(Throwable.class)); when(camelExchange.getException()).thenAnswer(inv -> captured.get()); // feeds back setException calls ``` With this, `captured.get()` will be `transportEx` (with `bindingEx` suppressed) — same as `handleException_bindingThrows_preservesRootCauseAndCallsDone` effectively tests. Keep or merge them and add the conduitSelector+no-resolved+binding-throws case separately. **Fix option B** — rename and adjust to test the conduitSelector path (the actual "no root cause" case): ```java void handleException_conduitSelector_noResolvedEx_bindingThrows_setsBindingException() { // conduitSelector set, neither message carries an exception (failover succeeded), // but then populateExchangeFromCxfResponse throws — binding error must become the exchange exception ... when(cxfExchange.get(ConduitSelector.class)).thenReturn(conduitSelector); when(outMessage.getContent(Exception.class)).thenReturn(null); when(cxfExchange.getInMessage()).thenReturn(null); doThrow(bindingEx).when(cxfBinding).populateExchangeFromCxfResponse(any(), any(), any()); // then verify: captured.get() == bindingEx (camelExchange.setException was never called before binding) } ``` -- 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]
