This is an automated email from the ASF dual-hosted git repository.
davsclaus pushed a commit to branch fix/CAMEL-25390
in repository https://gitbox.apache.org/repos/asf/camel.git
The following commit(s) were added to refs/heads/fix/CAMEL-25390 by this push:
new 6a1aefecd098 CAMEL-25390: Address review feedback
6a1aefecd098 is described below
commit 6a1aefecd0984d2086c9b723614f0da88e9ada32
Author: Claus Ibsen <[email protected]>
AuthorDate: Thu Oct 15 21:45:48 2026 +0200
CAMEL-25390: Address review feedback
An exception, an unknown action or a reported error now wins over a result
the action wrote before it failed: the file transport answers
{"status":"failed","error":...} with that result under "result", as the
WebSocket transport sends both. A result that reports a failure of its own
is
still written as it is, and the shared action file keeps its behaviour.
Several reported problems, such as two routes that fail to stop, are joined
in the error instead of the last one winning, and the reason is not-found
only when every problem is a not-found.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: Claus Ibsen <[email protected]>
---
.../apache/camel/catalog/docs/cli-connector.adoc | 4 +-
.../src/main/docs/cli-connector.adoc | 4 +-
.../camel/cli/connector/CliActionOutcome.java | 58 ++++++++++++++------
.../cli/connector/FileCliConnectorTransport.java | 24 +++++----
.../camel/cli/connector/CliActionOutcomeTest.java | 50 ++++++++++++++++++
.../connector/FileCliConnectorTransportTest.java | 61 +++++++++++++++++++---
6 files changed, 167 insertions(+), 34 deletions(-)
diff --git
a/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/cli-connector.adoc
b/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/cli-connector.adoc
index 7d6e67b68b40..8dc2e8170fdd 100644
---
a/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/cli-connector.adoc
+++
b/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/cli-connector.adoc
@@ -58,7 +58,9 @@ A tool writes an action to
`~/.camel/<pid>-action-<requestId>.json`, and the con
----
`reason` is `not-found` when what the action was asked to work on does not
exist, such as a route id that matches no
-route. A result can report a failure of its own, such as a message sent to a
route that throws an exception.
+route. When the action wrote a result before it failed, the answer has it
under `result`. Several problems, such as
+two routes that fail to stop, are joined with `;` in `error`. A result can
also report a failure of its own, such as a
+message sent to a route that throws an exception: it is written as it is.
Write the action file atomically (write a temporary file, then move it), so
the connector never reads half an action.
An action file that cannot be read yet is retried for two seconds. Deleting
the action file before the answer is
diff --git a/dsl/camel-cli-connector/src/main/docs/cli-connector.adoc
b/dsl/camel-cli-connector/src/main/docs/cli-connector.adoc
index 7d6e67b68b40..8dc2e8170fdd 100644
--- a/dsl/camel-cli-connector/src/main/docs/cli-connector.adoc
+++ b/dsl/camel-cli-connector/src/main/docs/cli-connector.adoc
@@ -58,7 +58,9 @@ A tool writes an action to
`~/.camel/<pid>-action-<requestId>.json`, and the con
----
`reason` is `not-found` when what the action was asked to work on does not
exist, such as a route id that matches no
-route. A result can report a failure of its own, such as a message sent to a
route that throws an exception.
+route. When the action wrote a result before it failed, the answer has it
under `result`. Several problems, such as
+two routes that fail to stop, are joined with `;` in `error`. A result can
also report a failure of its own, such as a
+message sent to a route that throws an exception: it is written as it is.
Write the action file atomically (write a temporary file, then move it), so
the connector never reads half an action.
An action file that cannot be read yet is retried for two seconds. Deleting
the action file before the answer is
diff --git
a/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/CliActionOutcome.java
b/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/CliActionOutcome.java
index 347ed190f6d7..febea69e1f86 100644
---
a/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/CliActionOutcome.java
+++
b/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/CliActionOutcome.java
@@ -16,6 +16,8 @@
*/
package org.apache.camel.cli.connector;
+import java.util.ArrayList;
+import java.util.List;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
@@ -39,19 +41,21 @@ final class CliActionOutcome {
private final String error;
private final String reason;
private final Throwable failure;
+ private final boolean failedResult;
- private CliActionOutcome(JsonObject result, String error, String reason,
Throwable failure) {
+ private CliActionOutcome(JsonObject result, String error, String reason,
Throwable failure, boolean failedResult) {
this.result = result;
this.error = error;
this.reason = reason;
this.failure = failure;
+ this.failedResult = failedResult;
}
/**
* An action that could not be dispatched, such as an action file that is
not valid JSON.
*/
static CliActionOutcome failed(String error) {
- return new CliActionOutcome(null, error, null, null);
+ return new CliActionOutcome(null, error, null, null, false);
}
/**
@@ -83,25 +87,25 @@ final class CliActionOutcome {
cause = cause.getCause();
}
String message = cause.getMessage() != null ? cause.getMessage() :
cause.getClass().getSimpleName();
- return new CliActionOutcome(output.result, message, null, cause);
+ return new CliActionOutcome(output.result, message, null, cause,
false);
}
if (Boolean.FALSE.equals(known)) {
String name = action.getString("action");
return new CliActionOutcome(
- output.result, name != null ? "Unknown action: " + name :
"Missing action", null,
- null);
+ output.result, name != null ? "Unknown action: " + name :
"Missing action", null, null, false);
}
- if (output.error != null) {
- return new CliActionOutcome(output.result, output.error,
output.reason, null);
+ String reported = output.error();
+ if (reported != null) {
+ return new CliActionOutcome(output.result, reported,
output.reason(), null, false);
}
if (output.result != null) {
// many actions report failures inside their result, as the file
transport had no other way to do it
String status = output.result.getString("status");
if ("error".equals(status) || "failed".equals(status)) {
- return new CliActionOutcome(output.result,
messageOf(output.result), null, null);
+ return new CliActionOutcome(output.result,
messageOf(output.result), null, null, true);
}
}
- return new CliActionOutcome(output.result, null, null, null);
+ return new CliActionOutcome(output.result, null, null, null, false);
}
private static String messageOf(JsonObject result) {
@@ -145,9 +149,17 @@ final class CliActionOutcome {
return failure;
}
+ /**
+ * Whether the error comes from the result itself, which reports a failure
of its own (status <tt>error</tt> or
+ * <tt>failed</tt>), rather than from an exception, an unknown action or a
reported error.
+ */
+ boolean failedResult() {
+ return failedResult;
+ }
+
/**
* The error as a reply for the file transport:
<tt>{"status":"failed","error":"..."}</tt>, the shape the CLI has
- * read since Camel 4.23.
+ * read since Camel 4.23, with the result the action wrote before it
failed, if any.
*/
JsonObject errorReply() {
JsonObject reply = new JsonObject();
@@ -156,13 +168,17 @@ final class CliActionOutcome {
if (reason != null) {
reply.put("reason", reason);
}
+ if (result != null) {
+ reply.put("result", result);
+ }
return reply;
}
private static final class Collector implements CliActionOutput {
private volatile JsonObject result;
- private volatile String error;
- private volatile String reason;
+ // an action can report several problems, such as one per route it
fails to stop
+ private final List<String> errors = new ArrayList<>();
+ private boolean onlyNotFound = true;
@Override
public void write(JsonObject result) {
@@ -170,14 +186,22 @@ final class CliActionOutcome {
}
@Override
- public void error(String message) {
- this.error = message;
+ public synchronized void error(String message) {
+ errors.add(message);
+ onlyNotFound = false;
}
@Override
- public void notFound(String message) {
- this.error = message;
- this.reason = NOT_FOUND;
+ public synchronized void notFound(String message) {
+ errors.add(message);
+ }
+
+ synchronized String error() {
+ return errors.isEmpty() ? null : String.join("; ", errors);
+ }
+
+ synchronized String reason() {
+ return !errors.isEmpty() && onlyNotFound ? NOT_FOUND : null;
}
}
}
diff --git
a/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/FileCliConnectorTransport.java
b/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/FileCliConnectorTransport.java
index 7804f1941f2d..4195b13a4723 100644
---
a/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/FileCliConnectorTransport.java
+++
b/dsl/camel-cli-connector/src/main/java/org/apache/camel/cli/connector/FileCliConnectorTransport.java
@@ -45,11 +45,12 @@ import org.slf4j.LoggerFactory;
* Deleting the <tt>{pid}</tt> lock file shuts down the integration.
* <p/>
* An action with a request id (<tt>{pid}-action-{requestId}.json</tt>) always
gets a reply in
- * <tt>{pid}-output-{requestId}.json</tt>: its result,
<tt>{"status":"failed","error":"..."}</tt> when it failed (with
- * <tt>"reason":"not-found"</tt> when what it was asked to work on does not
exist), or <tt>{"status":"success"}</tt>
- * when it has no result. An action in the shared <tt>{pid}-action.json</tt>
only gets its result, or the error when it
- * threw an exception, as a Camel CLI before 4.24 never reads the other
replies, and the next command could read them
- * instead of its own result.
+ * <tt>{pid}-output-{requestId}.json</tt>:
<tt>{"status":"failed","error":"..."}</tt> when it is unknown, throws or
+ * reports an error (with <tt>"reason":"not-found"</tt> when what it was asked
to work on does not exist, and the result
+ * it wrote before it failed under <tt>"result"</tt>), its result, or
<tt>{"status":"success"}</tt> when it has no
+ * result. A result that reports a failure of its own is written as it is. An
action in the shared
+ * <tt>{pid}-action.json</tt> only gets its result, or the error when it threw
an exception, as a Camel CLI before 4.24
+ * never reads the other replies, and the next command could read them instead
of its own result.
*/
public class FileCliConnectorTransport extends ServiceSupport implements
CliConnectorTransport {
@@ -265,15 +266,20 @@ public class FileCliConnectorTransport extends
ServiceSupport implements CliConn
}
/**
- * The reply to an action: its result, the error, or an ack. The shared
output file only gets the result, or the
+ * The reply to an action: the error, its result, or an ack. The shared
output file only gets the result, or the
* error of an exception (as before 4.24), so the next command does not
read a reply meant for another command.
*/
private static JsonObject reply(CliActionOutcome outcome, boolean
replyAlways) {
- if (outcome.result() != null) {
+ if (outcome.error() != null && !outcome.failedResult()) {
+ // an exception, an unknown action or a reported error wins over a
(partial) result
+ if (replyAlways || outcome.failure() != null) {
+ return outcome.errorReply();
+ }
return outcome.result();
}
- if (outcome.error() != null && (replyAlways || outcome.failure() !=
null)) {
- return outcome.errorReply();
+ if (outcome.result() != null) {
+ // also a result that reports a failure of its own: the CLI reads
it as it is
+ return outcome.result();
}
if (replyAlways) {
JsonObject ack = new JsonObject();
diff --git
a/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/CliActionOutcomeTest.java
b/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/CliActionOutcomeTest.java
index d49cc90ccffb..b373c5406897 100644
---
a/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/CliActionOutcomeTest.java
+++
b/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/CliActionOutcomeTest.java
@@ -77,6 +77,56 @@ class CliActionOutcomeTest {
assertThat(outcome.errorReply()).containsEntry("reason", "not-found");
}
+ @Test
+ void severalProblemsAreJoined() {
+ CliActionOutcome outcome = join(CliActionOutcome.dispatch((action,
output) -> {
+ output.error("Error stop route: a due to: x");
+ output.error("Error stop route: b due to: y");
+ return true;
+ }, action("route")));
+
+ assertThat(outcome.error()).isEqualTo("Error stop route: a due to: x;
Error stop route: b due to: y");
+ assertThat(outcome.reason()).isNull();
+ }
+
+ @Test
+ void notFoundOnlyWhenEveryProblemIsNotFound() {
+ CliActionOutcome mixed = join(CliActionOutcome.dispatch((action,
output) -> {
+ output.notFound("No route matching: a");
+ output.error("Error stop route: b due to: y");
+ return true;
+ }, action("route")));
+ assertThat(mixed.reason()).isNull();
+
+ CliActionOutcome notFound = join(CliActionOutcome.dispatch((action,
output) -> {
+ output.notFound("No route matching: a");
+ output.notFound("No route matching: b");
+ return true;
+ }, action("route")));
+ assertThat(notFound.reason()).isEqualTo(CliActionOutcome.NOT_FOUND);
+ assertThat(notFound.error()).isEqualTo("No route matching: a; No route
matching: b");
+ }
+
+ @Test
+ void exceptionAfterAPartialResultKeepsBoth() {
+ CliActionOutcome outcome = join(CliActionOutcome.dispatch((action,
output) -> {
+ output.write(result("{\"status\":\"success\",\"value\":1}"));
+ throw new IllegalStateException("Boom");
+ }, action("partial")));
+
+ assertThat(outcome.error()).isEqualTo("Boom");
+ assertThat(outcome.failedResult()).isFalse();
+ assertThat(outcome.errorReply()).containsEntry("status",
"failed").containsEntry("error", "Boom");
+ JsonObject partial = outcome.errorReply().getMap("result");
+ assertThat(partial.getInteger("value")).isEqualTo(1);
+ }
+
+ @Test
+ void failedResultIsMarked() {
+
assertThat(outcome(result("{\"status\":\"error\",\"message\":\"x\"}")).failedResult()).isTrue();
+
assertThat(outcome(result("{\"status\":\"success\"}")).failedResult()).isFalse();
+ }
+
@Test
void cancellingTheOutcomeCancelsTheAction() {
CompletableFuture<Boolean> running = new CompletableFuture<>();
diff --git
a/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/FileCliConnectorTransportTest.java
b/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/FileCliConnectorTransportTest.java
index 43fa587d3510..a76432df4f4b 100644
---
a/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/FileCliConnectorTransportTest.java
+++
b/dsl/camel-cli-connector/src/test/java/org/apache/camel/cli/connector/FileCliConnectorTransportTest.java
@@ -131,6 +131,34 @@ class FileCliConnectorTransportTest {
.containsEntry("reason", "not-found");
}
+ @Test
+ void exceptionWinsOverAPartialResult() {
+ writeAction(action("partial"), "r1");
+
+ JsonObject reply = awaitJson(file(PID + "-output-r1.json"));
+ assertThat(reply).containsEntry("status",
"failed").containsEntry("error", "Boom");
+ JsonObject partial = reply.getMap("result");
+ assertThat(partial).containsEntry("value", "42");
+ }
+
+ @Test
+ void reportedErrorWinsOverAResult() {
+ writeAction(action("partial-error"), "r1");
+
+ JsonObject reply = awaitJson(file(PID + "-output-r1.json"));
+ assertThat(reply).containsEntry("status",
"failed").containsEntry("error", "Half done");
+ JsonObject partial = reply.getMap("result");
+ assertThat(partial).containsEntry("value", "42");
+ }
+
+ @Test
+ void resultWithAFailureOfItsOwnIsWrittenAsItIs() {
+ writeAction(action("failed-result"), "r1");
+
+ assertThat(awaitJson(file(PID + "-output-r1.json")).toJson())
+ .isEqualTo("{\"status\":\"error\",\"message\":\"No such
datasource\"}");
+ }
+
@Test
void invalidActionGetsAnErrorOnceItIsNotBeingWritten() throws Exception {
File action = writeText("{\"action\":\"res", "r1");
@@ -170,6 +198,11 @@ class FileCliConnectorTransportTest {
assertThat(awaitJson(output)).containsEntry("status",
"failed").containsEntry("error", "Boom");
Files.writeString(output.toPath(), "");
+ // as before 4.24: a reported error is not written, but the result is
+ writeAction(action("partial-error"), null);
+
assertThat(awaitJson(output).toJson()).isEqualTo("{\"status\":\"success\",\"value\":\"42\"}");
+ Files.writeString(output.toPath(), "");
+
// no reply that a Camel CLI before 4.24 would not read, and the next
command could read instead of its own
for (String name : new String[] { "nothing", "error", "missing",
"does-not-exist" }) {
writeAction(action(name), null);
@@ -197,13 +230,22 @@ class FileCliConnectorTransportTest {
return false;
}
switch (name) {
- case "result" -> {
- JsonObject result = new JsonObject();
- result.put("status", "success");
- result.put("value", "42");
- output.write(result);
- }
+ case "result" -> output.write(result());
case "boom" -> throw new IllegalStateException("Boom");
+ case "partial" -> {
+ output.write(result());
+ throw new IllegalStateException("Boom");
+ }
+ case "partial-error" -> {
+ output.write(result());
+ output.error("Half done");
+ }
+ case "failed-result" -> {
+ JsonObject failed = new JsonObject();
+ failed.put("status", "error");
+ failed.put("message", "No such datasource");
+ output.write(failed);
+ }
case "error" -> output.error("Broken");
case "missing" -> output.notFound("No route matching: nope");
case "nothing" -> {
@@ -216,6 +258,13 @@ class FileCliConnectorTransportTest {
return true;
}
+ private static JsonObject result() {
+ JsonObject result = new JsonObject();
+ result.put("status", "success");
+ result.put("value", "42");
+ return result;
+ }
+
private File file(String name) {
return new File(dir, name);
}
