gnodet-bot commented on code in PR #28500:
URL: https://github.com/apache/camel/pull/28500#discussion_r4375431584
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/TextBodyFlow.java:
##########
@@ -149,11 +188,15 @@ sealed interface Body permits Text, Parsed {
record Parsed(String origin, String format) implements Body {
}
- /** Text: the payload as it was read or written. */
- record Text(String origin, String format, boolean file) implements Body {
+ /** Text: the payload as it was read or written; marshalled when a marshal
wrote it from parsed data. */
+ record Text(String origin, String format, boolean file, boolean
marshalled) implements Body {
Review Comment:
š” **Readability nit:** The `Text` record now has 4 positional boolean
parameters (`file`, `marshalled`). Call sites passing `true, false` or `false,
true` are easy to misread. A dedicated pattern such as a builder or named
constants could help, though with a `record` the friction may not be worth it.
Alternatively, using a small enum (`enum Encoding { RAW, MARSHALLED }`) instead
of `boolean marshalled` would make call sites self-documenting.
##########
core/camel-support/src/main/java/org/apache/camel/support/BodyFacts.java:
##########
@@ -277,6 +284,42 @@ private static String unmarshalFirst(Exchange exchange,
Object body) {
};
}
+ /**
+ * When a marshal wrote the body from a Map or a List, as the message
history says: the marshal is wanted, for a
+ * later step that needs text (a file write), and the field is read after
it, too late; an unmarshal would undo it.
+ * Null when the history does not say so, as for a marshal of text, where
an unmarshal was meant.
+ */
+ private static String marshalledData(Exchange exchange, Object body) {
+ List<MessageHistory> history =
exchange.getProperty(ExchangePropertyKey.MESSAGE_HISTORY, List.class);
+ if (history == null || history.isEmpty()) {
+ return null;
+ }
+ String now = ObjectHelper.classCanonicalName(body);
+ for (int i = history.size() - 1; i >= 0; i--) {
+ MessageHistory step = history.get(i);
+ String type = step.getBodyType();
+ if (type == null || type.equals(now)) {
+ continue;
+ }
+ // the step that set the body, and the body it was reached with
+ if (step.getNode() == null ||
!"marshal".equals(step.getNode().getShortName())
+ || !isDataType(exchange.getContext(), type)) {
+ return null;
+ }
+ return "the " + step(step.getNode()) + " wrote it from data (" +
StringHelper.afterLast(type, ".", type)
+ + "): read the field before the marshal, for example into a
variable with setVariable, or keep the data"
+ + " in the body and marshal into a variable (marshal with
variableReceive, and send that variable with"
+ + " variableSend)";
+ }
+ return null;
+ }
+
+ /** Whether a class, by its name, is a Map or a collection. */
+ private static boolean isDataType(CamelContext context, String type) {
+ Class<?> clazz = context != null ?
context.getClassResolver().resolveClass(type) : null;
Review Comment:
š” **Portability concern:** `context.getClassResolver().resolveClass(type)`
uses the default Camel class resolver, which delegates to the thread-context
classloader. This works fine in standard environments, but in OSGi (Karaf) and
Quarkus native, the thread-context classloader may not see user application
classes ā causing `resolveClass` to return `null` and the whole check to
silently return `false`.
If `type` is a FQCN of a Map/Collection subclass defined in a user bundle,
it will be invisible here. Consider a fallback:
```suggestion
Class<?> clazz = context != null ?
context.getClassResolver().resolveClass(type) : null;
if (clazz == null) {
try {
clazz =
Thread.currentThread().getContextClassLoader().loadClass(type);
} catch (ClassNotFoundException ignored) {
}
}
```
Or, since this check is advisory (it only affects the diagnostic message),
accepting that it degrades gracefully in those environments may be the
pragmatic call ā just worth a conscious decision.
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/TextBodyFlow.java:
##########
@@ -565,14 +657,317 @@ private static Text setBodyText(JsonNode value) {
* iterator).
*/
private static Parsed unmarshalled(JsonNode value, CatalogBodyTypes
bodyTypes) {
- if (value == null || !value.isObject() ||
!value.fieldNames().hasNext()) {
+ String format = dataFormatOf(value);
+ if (format == null) {
return null;
}
- String format = value.fieldNames().next();
CatalogBodyTypes.Answer out = bodyTypes.unmarshalOut(format,
value.get(format));
return out != null && out.data() ? new Parsed("unmarshal: " + format +
" turns the text into data", format) : null;
}
+ /** The text a marshal writes; marshalled when the body it writes is known
to be parsed data. */
+ private static Text marshalled(JsonNode value, Body body) {
+ String format = dataFormatOf(value);
+ return new Text(
+ "marshal" + (format != null ? ": " + format : "")
+ + " writes data as text (marshal turns data into text,
unmarshal turns text into data)",
+ format, false, body instanceof Parsed);
+ }
+
+ /** The data format of an unmarshal or a marshal: the key that is not one
of its options, such as json. */
+ private static String dataFormatOf(JsonNode value) {
+ if (value == null || !value.isObject()) {
+ return null;
+ }
+ for (var it = value.fieldNames(); it.hasNext();) {
+ String key = it.next();
+ if (!DATA_FORMAT_OPTIONS.contains(key)) {
+ return key;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * An unmarshal of data that is parsed already: the body after an
unmarshal, or a variable that holds data
+ * (variableSend). The data format reads text, and a Map is no text: "No
type converter available to convert from
+ * type: java.util.LinkedHashMap to the required type:
java.io.InputStream". Reports it, and answers whether it did.
+ */
+ private static boolean parsesData(String name, JsonNode value, Body body,
NodePath at, Walk w) {
+ String format = "unmarshal".equals(name) ? dataFormatOf(value) : null;
+ if (format == null) {
+ return false;
+ }
+ String variable = fixedName(value.get("variableSend"));
+ Body input = variable != null ? w.variables().get(variable) :
value.has("variableSend") ? null : body;
+ CatalogBodyTypes.Answer in = input instanceof Parsed ?
w.bodyTypes().unmarshalIn(format, value.get(format)) : null;
+ if (in == null || !in.onlyPayload()) {
+ return false;
+ }
+ String held = variable != null ? "the variable " + variable + " holds"
: "the body here is";
+ String fix = variable != null
+ ? "the variable is data already, so leave this unmarshal out,
and read its fields as they are, such as"
+ + " ${variable." + variable + "[name]}"
+ : "the body is data already, so leave this unmarshal out, and
read its fields as they are; to parse text"
+ + " in a variable, unmarshal it with variableSend and
variableReceive set to its name";
+ w.errors().add(Error.builder()
+ .keyword("type")
+ .instanceLocation(at)
+ .messageKey("type")
+ .format(new MessageFormat("{0}"))
+ .arguments((w.routeId() != null ? "route " + w.routeId() + ":
" : "") + "unmarshal: " + format
+ + " parses text (" + in.inWords() + "), but " + held
+ + " parsed data (a Map or a List), not text - " +
input.origin() + ": " + fix)
+ .build());
+ return true;
+ }
+
+ /**
+ * Remembers what a step puts in a variable: the result of a step with
variableReceive, which is what the step would
+ * leave in the body, and the body or a constant a setVariable sets. A
step that may set any variable (a bean, a
+ * call of a route the walk does not follow, a block with such a step)
makes the walk forget them all, so that it
+ * only reports a variable it is certain of.
+ */
+ private static void remember(String name, JsonNode value, Body body, Walk
w) {
+ Map<String, Body> variables = w.variables();
+ String uri = sendUri(name, value);
+ boolean callsARoute = uri != null && ROUTES.contains(scheme(uri))
+ && !("to".equals(name) && !value.has("variableReceive") &&
w.byFrom().containsKey(normalize(uri)));
+ if (SETS_VARIABLES.contains(name) || callsARoute ||
RouteGraph.SENDS.contains(name) && uri == null
+ || mayChangeVariables(value)) {
+ variables.clear();
+ }
+ String receive = value != null && value.isObject() ?
fixedName(value.get("variableReceive")) : null;
+ if (receive != null) {
+ put(variables, receive, received(name, value, uri, body, w));
+ } else if (value != null && value.isObject() &&
value.has("variableReceive")) {
+ // a variable named by an expression
+ variables.clear();
+ } else if ("setVariable".equals(name) ||
"removeVariable".equals(name)) {
+ JsonNode n = value != null && value.isObject() ? value.get("name")
: value;
+ String variable = fixedName(n);
+ if (variable == null) {
+ variables.clear();
+ } else if ("setVariable".equals(name)) {
+ put(variables, variable, setVariableBody(value, body));
+ } else {
+ variables.remove(variable);
+ }
+ }
+ }
+
+ /** Whether the steps of a block may set a variable: a step that does, or
a call of a route. */
+ private static boolean mayChangeVariables(JsonNode value) {
+ if (value == null || !value.isContainerNode()) {
+ return false;
+ }
+ String text = value.toString();
Review Comment:
š” **Minor performance note:** `value.toString()` on a Jackson `JsonNode`
serializes the entire subtree to a string on every call to
`mayChangeVariables`, which is invoked from `remember()` during the walk. For
large JSON bodies, this is O(nĀ²) in the number of steps Ć the size of the body.
Since this is build-time analysis (not runtime), the impact is bounded and
likely negligible in practice. Just noting it in case large generated Camel
YAML routes with many steps ever hit 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]
