davsclaus commented on code in PR #25205:
URL: https://github.com/apache/camel/pull/25205#discussion_r3709707687
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/RouteService.java:
##########
@@ -136,11 +136,32 @@ public void setUp() throws FailedToStartRouteException {
try {
doSetup();
} catch (Exception e) {
- throw new FailedToStartRouteException(getId(), getLocation(),
e.getLocalizedMessage(), e);
+ throw new FailedToStartRouteException(getId(), getLocation(),
extractUsefulMessage(e), e);
}
}
}
+ /**
+ * Extracts a non-null, non-empty error message from the exception or its
cause chain.
+ * <p/>
+ * {@link Throwable#getLocalizedMessage()} can return {@code null} for
exceptions such as
+ * {@link NullPointerException} that carry no message, which would cause
{@link FailedToStartRouteException} to
+ * throw {@link NullPointerException} from its own constructor (via {@code
Objects.requireNonNull}) instead of
+ * wrapping the original failure. This helper walks the cause chain to
find the first meaningful message and falls
+ * back to the simple class name so the caller always receives a non-null
string.
+ */
+ private static String extractUsefulMessage(Throwable e) {
+ Throwable current = e;
+ while (current != null) {
Review Comment:
Consider making this `static` package-private (`static String
extractUsefulMessage(...)`) instead of `private` so that
`DefaultSupervisingRouteController` (line 521) — which has the same null-safety
bug with `e.getMessage()` — can reuse it. Alternatively, fix both call sites
independently in a follow-up.
##########
core/camel-core/src/test/java/org/apache/camel/impl/engine/RouteServiceWarmUpNullMessageTest.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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.impl.engine;
+
+import java.util.Map;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.Consumer;
+import org.apache.camel.Endpoint;
+import org.apache.camel.Exchange;
+import org.apache.camel.FailedToStartRouteException;
+import org.apache.camel.Processor;
+import org.apache.camel.Producer;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.apache.camel.support.DefaultComponent;
+import org.apache.camel.support.DefaultConsumer;
+import org.apache.camel.support.DefaultEndpoint;
+import org.apache.camel.support.DefaultProducer;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Verifies that {@link RouteService#warmUp()} and {@link
RouteService#setUp()} wrap startup failures in a
+ * {@link FailedToStartRouteException} whose message is always meaningful —
even when the root cause exception carries a
+ * {@code null} message (e.g. a bare {@link NullPointerException}).
+ *
+ * <p>
+ * Before the fix, {@code RouteService} passed {@code e.getLocalizedMessage()}
directly to the
+ * {@link FailedToStartRouteException} constructor, which calls {@code
Objects.requireNonNull} on that argument. A
+ * message-less exception therefore caused a secondary {@link
NullPointerException} to be thrown from inside the
+ * exception constructor rather than a proper {@link
FailedToStartRouteException}.
+ *
+ * <p>
+ * The tests trigger the failure during endpoint initialisation (inside {@code
doSetup()}), which is the code path
+ * covered by the {@code RouteService} fix.
+ */
+class RouteServiceWarmUpNullMessageTest {
+
+ /**
+ * When the endpoint throws a message-less {@link NullPointerException}
during route setup, the result must be a
+ * {@link FailedToStartRouteException}, not a raw NPE.
+ */
+ @Test
+ void testSetUpNullMessageExceptionProducesFailedToStartRouteException() {
+ CamelContext context = new DefaultCamelContext();
+ context.addComponent("fail", new NullMessageFailComponent());
+
+ assertThatThrownBy(() -> {
+ context.addRoutes(new RouteBuilder() {
+ @Override
+ public void configure() {
+
from("fail:trigger").routeId("test-route").to("direct:out");
+ }
+ });
+ context.start();
+ }).isInstanceOf(FailedToStartRouteException.class);
+ }
+
+ /**
+ * The {@link FailedToStartRouteException} message must contain the route
id and must not use the literal string
+ * "null" as the failure description.
+ */
+ @Test
+ void testFailedToStartMessageIsNonNullAndMeaningful() {
+ CamelContext context = new DefaultCamelContext();
+ context.addComponent("fail", new NullMessageFailComponent());
+
+ FailedToStartRouteException caught = null;
+ try {
+ context.addRoutes(new RouteBuilder() {
+ @Override
+ public void configure() {
Review Comment:
Suggestion: since test 1 shows `FailedToStartRouteException` is thrown
directly (not wrapped), the cause-chain walking here is unnecessary. This test
could use `assertThatThrownBy` consistently with test 1:
```java
assertThatThrownBy(() -> {
context.addRoutes(...);
context.start();
}).isInstanceOf(FailedToStartRouteException.class)
.satisfies(thrown -> assertThat(thrown.getMessage())
.contains("meaningful-route")
.doesNotContain("because: null"));
```
Same applies to test 3. Not blocking — just a readability suggestion.
##########
core/camel-core/src/test/java/org/apache/camel/impl/engine/RouteServiceWarmUpNullMessageTest.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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.impl.engine;
+
+import java.util.Map;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.Consumer;
+import org.apache.camel.Endpoint;
+import org.apache.camel.Exchange;
+import org.apache.camel.FailedToStartRouteException;
+import org.apache.camel.Processor;
+import org.apache.camel.Producer;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.apache.camel.support.DefaultComponent;
+import org.apache.camel.support.DefaultConsumer;
+import org.apache.camel.support.DefaultEndpoint;
+import org.apache.camel.support.DefaultProducer;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Verifies that {@link RouteService#warmUp()} and {@link
RouteService#setUp()} wrap startup failures in a
+ * {@link FailedToStartRouteException} whose message is always meaningful —
even when the root cause exception carries a
+ * {@code null} message (e.g. a bare {@link NullPointerException}).
+ *
+ * <p>
+ * Before the fix, {@code RouteService} passed {@code e.getLocalizedMessage()}
directly to the
+ * {@link FailedToStartRouteException} constructor, which calls {@code
Objects.requireNonNull} on that argument. A
+ * message-less exception therefore caused a secondary {@link
NullPointerException} to be thrown from inside the
+ * exception constructor rather than a proper {@link
FailedToStartRouteException}.
+ *
+ * <p>
+ * The tests trigger the failure during endpoint initialisation (inside {@code
doSetup()}), which is the code path
+ * covered by the {@code RouteService} fix.
+ */
+class RouteServiceWarmUpNullMessageTest {
+
+ /**
+ * When the endpoint throws a message-less {@link NullPointerException}
during route setup, the result must be a
+ * {@link FailedToStartRouteException}, not a raw NPE.
+ */
+ @Test
+ void testSetUpNullMessageExceptionProducesFailedToStartRouteException() {
+ CamelContext context = new DefaultCamelContext();
+ context.addComponent("fail", new NullMessageFailComponent());
+
+ assertThatThrownBy(() -> {
+ context.addRoutes(new RouteBuilder() {
+ @Override
+ public void configure() {
+
from("fail:trigger").routeId("test-route").to("direct:out");
+ }
+ });
+ context.start();
+ }).isInstanceOf(FailedToStartRouteException.class);
+ }
+
+ /**
+ * The {@link FailedToStartRouteException} message must contain the route
id and must not use the literal string
Review Comment:
Minor: unlike tests 2 and 3, this test does not call `context.stop()` after
the assertion. If `context.start()` partially initialises resources before
failing, they would leak during the test run. Consider wrapping with
`try/finally` like the other tests, or using `assertThatThrownBy` inside a try
block with cleanup.
--
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]
