gnodet-bot commented on code in PR #27956: URL: https://github.com/apache/camel/pull/27956#discussion_r4350511477
########## components/camel-milo/src/test/java/org/apache/camel/component/milo/client/internal/SubscriptionManagerExceptionPropagationTest.java: ########## @@ -0,0 +1,203 @@ +/* + * 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.milo.client.internal; + +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +import org.apache.camel.component.milo.client.MiloClientConfiguration; +import org.eclipse.milo.opcua.stack.core.types.builtin.DataValue; +import org.eclipse.milo.opcua.stack.core.types.builtin.ExpandedNodeId; +import org.eclipse.milo.opcua.stack.core.types.builtin.Variant; +import org.eclipse.milo.opcua.stack.core.types.structured.CallMethodResult; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Unit tests for {@link SubscriptionManager} exception propagation through write() and call(). + * + * <p> + * Regression tests for CAMEL-25694: {@code SubscriptionManager.write()} and {@code call()} previously used + * {@code handleAsync()} which converted exceptional completions into normal completions with null values. The fix + * replaces {@code handleAsync()} with {@code whenCompleteAsync()} to preserve the original exceptional completion. + * + * <p> + * The {@code whenCompleteAsync}-vs-{@code handleAsync} contract is tested via standalone helper methods (no real OPC UA + * connection needed). The disconnected-manager tests use a real {@link SubscriptionManager} to verify that + * {@code write()}, {@code call()}, and {@code readValues()} all return exceptional futures before any connection is + * established. + */ +class SubscriptionManagerExceptionPropagationTest { + + private ScheduledExecutorService executor; + + @BeforeEach + void setUp() { + executor = Executors.newSingleThreadScheduledExecutor(); + } + + @AfterEach + void tearDown() throws InterruptedException { + executor.shutdownNow(); + assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS), + "Executor must shut down cleanly within 5 seconds"); + } + + // ------------------------------------------------------------------------- + // Contract tests: whenCompleteAsync vs handleAsync + // + // These do NOT instantiate SubscriptionManager (which would trigger a + // connection attempt in its constructor). Instead they exercise the same + // CompletableFuture chain used by the production fix. + // ------------------------------------------------------------------------- + + /** Review Comment: ⚠️ **Two separate issues with the coverage:** 1. `testWhenCompleteAsyncPreservesExceptionalCompletion` and `testHandleAsyncSwallowsExceptionalCompletion` are pure `CompletableFuture` API contract tests — they don't call any `SubscriptionManager` method. They demonstrate *why* the fix works (the JDK contract), not *that* `SubscriptionManager` is correctly fixed. 2. `testWriteWhenDisconnectedReturnsExceptionalFuture` / `testCallWhenDisconnectedReturnsExceptionalFuture` / `testReadValuesWhenDisconnectedReturnsExceptionalFuture` test the `this.connected == null` guard (`newNotConnectedResult()`), not the `whenCompleteAsync` path. That path only executes when `this.connected != null` — i.e. on an active connection where the server-side operation fails. These tests verify pre-existing behavior, not the fix. The tests that would actually validate the fix are those that create a `SubscriptionManager` in the *connected* state with a `Connected` whose inner Milo client operation returns an exceptional future. ########## components/camel-milo/src/test/java/org/apache/camel/component/milo/client/MiloClientProducerWriteFailureTest.java: ########## @@ -0,0 +1,221 @@ +/* + * 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.milo.client; + +import java.lang.reflect.Field; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.Exchange; +import org.apache.camel.component.milo.MiloConstants; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.eclipse.milo.opcua.stack.core.types.builtin.ExpandedNodeId; +import org.eclipse.milo.opcua.stack.core.types.builtin.StatusCode; +import org.eclipse.milo.opcua.stack.core.types.enumerated.BrowseDirection; +import org.eclipse.milo.opcua.stack.core.types.structured.BrowseResult; +import org.eclipse.milo.opcua.stack.core.types.structured.CallMethodResult; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Review Comment: ⚠️ **Test doesn't exercise the fixed code path.** `StubMiloClientConnection` overrides `writeValue()` and `call()` to return pre-arranged futures directly, completely bypassing `SubscriptionManager`. In production, `MiloClientConnection.writeValue()` delegates to `this.manager.write()` (line 217 of `MiloClientConnection.java`) — exactly where the `handleAsync` → `whenCompleteAsync` fix lives. The stub short-circuits that delegation. This means the test would pass identically before and after this PR's fix. It tests that `MiloClientProducer.process()` propagates exceptions from the connection's `CompletableFuture` — which was never the bug. The bug was `SubscriptionManager` converting exceptions to `null` via `handleAsync`. To validate the actual fix, a test needs to invoke `SubscriptionManager.write()`/`call()` on a *connected* manager whose `Connected.write()`/`call()` returns a failing `CompletableFuture`, then assert the returned future is still exceptional. Without this, if someone reverts the `handleAsync` → `whenCompleteAsync` change, all tests still pass. ########## components/camel-milo/src/test/java/org/apache/camel/component/milo/client/internal/SubscriptionManagerExceptionPropagationTest.java: ########## @@ -0,0 +1,203 @@ +/* + * 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.milo.client.internal; + +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +import org.apache.camel.component.milo.client.MiloClientConfiguration; +import org.eclipse.milo.opcua.stack.core.types.builtin.DataValue; +import org.eclipse.milo.opcua.stack.core.types.builtin.ExpandedNodeId; +import org.eclipse.milo.opcua.stack.core.types.builtin.Variant; +import org.eclipse.milo.opcua.stack.core.types.structured.CallMethodResult; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Unit tests for {@link SubscriptionManager} exception propagation through write() and call(). + * + * <p> + * Regression tests for CAMEL-25694: {@code SubscriptionManager.write()} and {@code call()} previously used + * {@code handleAsync()} which converted exceptional completions into normal completions with null values. The fix + * replaces {@code handleAsync()} with {@code whenCompleteAsync()} to preserve the original exceptional completion. + * + * <p> + * The {@code whenCompleteAsync}-vs-{@code handleAsync} contract is tested via standalone helper methods (no real OPC UA + * connection needed). The disconnected-manager tests use a real {@link SubscriptionManager} to verify that + * {@code write()}, {@code call()}, and {@code readValues()} all return exceptional futures before any connection is + * established. + */ +class SubscriptionManagerExceptionPropagationTest { + + private ScheduledExecutorService executor; + + @BeforeEach + void setUp() { + executor = Executors.newSingleThreadScheduledExecutor(); + } + + @AfterEach + void tearDown() throws InterruptedException { + executor.shutdownNow(); + assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS), + "Executor must shut down cleanly within 5 seconds"); + } + + // ------------------------------------------------------------------------- + // Contract tests: whenCompleteAsync vs handleAsync + // + // These do NOT instantiate SubscriptionManager (which would trigger a + // connection attempt in its constructor). Instead they exercise the same + // CompletableFuture chain used by the production fix. + // ------------------------------------------------------------------------- + + /** + * Verifies that {@code whenCompleteAsync} (the fix) preserves an exceptional completion. + * + * <p> + * Before CAMEL-25694 was fixed, {@code SubscriptionManager.write()} used {@code handleAsync()}, which converted the + * exceptional completion into a normal null completion. This test verifies the correct behavior: a + * {@code whenCompleteAsync} side-effect chain over a failed future must stay failed. + */ + @Test + void testWhenCompleteAsyncPreservesExceptionalCompletion() throws Exception { + RuntimeException serviceError = new RuntimeException("Bad_Timeout OPC-UA service fault"); + CompletableFuture<Object> upstream = new CompletableFuture<>(); + upstream.completeExceptionally(serviceError); + + // Same chain as the fixed SubscriptionManager.write(): side-effect only, completion preserved + CompletableFuture<?> result = upstream.whenCompleteAsync((r, e) -> { + // side-effect (e.g. handleConnectionFailure); exception propagated by whenCompleteAsync + }, executor); + + ExecutionException ex = null; + try { + result.get(5, TimeUnit.SECONDS); + } catch (ExecutionException e) { + ex = e; + } catch (Exception e) { + fail("Unexpected exception type: " + e); + } + assertNotNull(ex, "Get on a failed whenCompleteAsync future must throw ExecutionException"); + assertInstanceOf(RuntimeException.class, ex.getCause()); + assertTrue(result.isCompletedExceptionally(), + "whenCompleteAsync must preserve the exceptional completion"); + } + Review Comment: **Low severity / documentation gap:** This test exercises `newNotConnectedResult()`, not the `whenCompleteAsync` path. Consider adding a comment making this explicit: ```suggestion /** * Verifies that a disconnected manager returns an exceptional future from write(), ensuring producers fail fast * when there is no connection. * * <p>Note: this tests the {@code this.connected == null} guard path ({@code newNotConnectedResult()}), * not the {@code whenCompleteAsync} exception-preservation fix (which requires a connected manager * with a failing upstream future). */ @Test void testWriteWhenDisconnectedReturnsExceptionalFuture() { ``` -- 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]
