mayurbm commented on code in PR #25205:
URL: https://github.com/apache/camel/pull/25205#discussion_r3710441655
##########
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:
Done — removed the `private` modifier so `extractUsefulMessage` is now
package-private `static`. This makes it accessible to
`DefaultSupervisingRouteController` (and any other class in the same package)
for reuse at the identical null-safety call site on line 521.
##########
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:
Fixed — test 1 is now wrapped in a `try/finally` block that calls
`context.stop()` in the `finally` clause, matching the cleanup pattern already
used in tests 2 and 3.
##########
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:
Done — both tests 2 and 3 now use `assertThatThrownBy` with fluent assertion
chaining (`.isInstanceOf()`, `.hasMessageContaining()`,
`.hasMessageNotContaining()`), consistent with test 1. The manual `try/catch`
unwrapping and intermediate `caught` variable are removed. Each test is still
wrapped in `try/finally` for `context.stop()` 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]
