davsclaus commented on code in PR #25554:
URL: https://github.com/apache/camel/pull/25554#discussion_r3845794699
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/InternalRouteStartupManager.java:
##########
@@ -426,7 +426,9 @@ private void doStartOrResumeRouteConsumers(
route.getProperties().remove("route.start.exception");
} catch (Exception e) {
route.getProperties().put("route.start.exception", e);
- throw e;
+ throw new FailedToStartRouteException(
+ routeService.getId(),
routeService.getLocation(),
+ extractUsefulMessage(e), e);
Review Comment:
This (and the identical block at the second catch site a bit further down)
now wraps *every* exception from consumer/route-service start, not just the
null-message case CAMEL-24404 describes. See the design-question discussion in
the overall review summary — the three `SupervisingRouteController` tests broke
precisely because this is broader than the reported bug needs.
##########
core/camel-core/src/test/java/org/apache/camel/impl/engine/InternalRouteStartupManagerConsumerStartTest.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * 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.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.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Verifies that {@link InternalRouteStartupManager} wraps consumer startup
failures in a
+ * {@link FailedToStartRouteException} with a meaningful (non-null) message,
even when the root cause carries no message
+ * (e.g. a bare {@link NullPointerException}).
+ *
+ * <p>
+ * Before the fix, the two {@code throw e} sites in {@code
doStartOrResumeRouteConsumers()} re-threw the raw exception
+ * without any wrapping, causing a bare NPE to escape directly to the caller
instead of a proper
+ * {@link FailedToStartRouteException}.
+ *
+ * <p>
+ * Reproducer: use a consumer whose {@code start()} throws a message-less
{@link NullPointerException}, matching the
+ * real-world scenario where e.g. {@code FileConsumer.doStart()} throws NPE
and it propagates through
+ * {@code BaseService.start()} to {@code
InternalRouteStartupManager.doStartOrResumeRouteConsumers()} line 429.
+ */
+public class InternalRouteStartupManagerConsumerStartTest {
Review Comment:
This class and its two `@Test` methods (here, and
`testConsumerStartWalksCauseChainForMessage` at line 99) are declared `public`.
Per this repo's convention, new test classes/methods shouldn't use `public` —
package-private is expected (the sibling PR #25553 had this same feedback
applied in a follow-up commit). The other `public` methods further down in this
file are fine since they override public interface methods
(`RouteBuilder.configure()`, `Endpoint.createConsumer()`, etc.) and must keep
that visibility.
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/InternalRouteStartupManager.java:
##########
@@ -496,4 +500,25 @@ int incrementRouteStartupOrder() {
return defaultRouteStartupOrder++;
}
+ /**
+ * 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) {
+ String msg = current.getLocalizedMessage();
+ if (msg != null && !msg.isBlank()) {
+ return msg;
+ }
+ current = current.getCause();
+ }
+ return e.getClass().getSimpleName();
+ }
Review Comment:
Two things here:
1. This method is an exact duplicate (including the javadoc) of
`RouteService.extractUsefulMessage(Throwable)` in
`core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/RouteService.java:152-162`,
added by PR #25205. Both classes are in the same package
(`org.apache.camel.impl.engine`), and `RouteService`'s version is already
package-private (`static`, not `private static`), so this could just call
`RouteService.extractUsefulMessage(e)` directly instead of redefining it.
2. Minor edge case: the guard `msg != null && !msg.isBlank()` only catches
an *absent* message. If some exception in the chain was constructed with the
literal string `"null"` as its message (e.g. from `String.valueOf(nullRef)`),
this returns that literal text verbatim — I checked, `extractUsefulMessage(new
RuntimeException("null"))` returns `"null"` — which reproduces the exact
`"...because: null"` symptom this fix exists to eliminate. Narrow case, just
flagging it.
--
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]
