gnodet-bot commented on code in PR #27082:
URL: https://github.com/apache/camel/pull/27082#discussion_r4132812544
##########
dsl/camel-xml-io-dsl/src/main/java/org/apache/camel/dsl/xml/io/XmlRoutesBuilderLoader.java:
##########
@@ -95,19 +97,50 @@ public void preParseRoute(Resource resource) throws
Exception {
if (preparseDone.getOrDefault(resource.getLocation(), false)) {
return;
}
- XmlStreamInfo xmlInfo = xmlInfo(resource);
- if (xmlInfo.isValid()) {
- String root = xmlInfo.getRootElementName();
- if ("beans".equals(root) || "blueprint".equals(root) ||
"camel".equals(root)) {
- new XmlModelParser(resource, xmlInfo.getRootElementNamespace())
- .parseBeansDefinition()
- .ifPresent(bd -> {
- registerBeans(resource, bd);
- camelAppCache.put(resource.getLocation(), bd);
- });
+ try {
+ XmlStreamInfo xmlInfo = xmlInfo(resource);
+ if (xmlInfo.isValid()) {
+ String root = xmlInfo.getRootElementName();
+ SemanticDefinition semantic = null;
+ if ("beans".equals(root) || "blueprint".equals(root) ||
"camel".equals(root)) {
+ new XmlModelParser(resource,
xmlInfo.getRootElementNamespace())
+ .parseBeansDefinition()
+ .ifPresent(bd -> {
+ registerBeans(resource, bd);
+ camelAppCache.put(resource.getLocation(), bd);
+ });
+ BeansDefinition app =
camelAppCache.get(resource.getLocation());
+ if (app != null) {
+ semantic = app.getSemantic();
+ }
+ } else if ("routes".equals(root) || "route".equals(root)) {
+ RoutesDefinition routes = new
XmlModelParser(resource(resource), xmlInfo.getRootElementNamespace())
+ .parseRoutesDefinition().orElse(null);
+ if (routes != null) {
+ routesCache.put(resource.getLocation(), routes);
+ semantic = routes.getSemantic();
+ }
+ }
+ SemanticDefinition.configure(getCamelContext(), resource,
resource.getLocation(), semantic);
}
+ preparseDone.put(resource.getLocation(), true);
+ } catch (Exception e) {
+ // A failed batch also prevents builders for earlier resources
from clearing their cached input.
+ resourceCache.clear();
+ xmlInfoCache.clear();
Review Comment:
💡 **Aggressive cache clearing on error:** The catch block clears ALL caches
(including previously-parsed valid resources) on any exception during preparse.
This is intentional — the test
`failedResourceBatchDoesNotReuseEarlierCachedDeclarationsOnRetry` validates
that earlier resources in a failed batch don't retain stale cached input on
retry. The comment explains the rationale well.
However, this means a single invalid XML resource in a large batch forces
re-parsing of ALL resources on retry, not just the failed one. For correctness
this is the right tradeoff, but for large deployments with many XML resources,
a per-resource rollback (tracking which resources were successfully preparsed
and only clearing the failed one's caches + preventing semantic side-effects)
would be more efficient. No action needed now — just flagging the tradeoff.
##########
dsl/camel-xml-io-dsl/src/main/java/org/apache/camel/dsl/xml/io/XmlRoutesBuilderLoader.java:
##########
@@ -95,19 +97,50 @@ public void preParseRoute(Resource resource) throws
Exception {
if (preparseDone.getOrDefault(resource.getLocation(), false)) {
return;
}
- XmlStreamInfo xmlInfo = xmlInfo(resource);
- if (xmlInfo.isValid()) {
- String root = xmlInfo.getRootElementName();
- if ("beans".equals(root) || "blueprint".equals(root) ||
"camel".equals(root)) {
- new XmlModelParser(resource, xmlInfo.getRootElementNamespace())
- .parseBeansDefinition()
- .ifPresent(bd -> {
- registerBeans(resource, bd);
- camelAppCache.put(resource.getLocation(), bd);
- });
+ try {
+ XmlStreamInfo xmlInfo = xmlInfo(resource);
+ if (xmlInfo.isValid()) {
+ String root = xmlInfo.getRootElementName();
+ SemanticDefinition semantic = null;
+ if ("beans".equals(root) || "blueprint".equals(root) ||
"camel".equals(root)) {
+ new XmlModelParser(resource,
xmlInfo.getRootElementNamespace())
+ .parseBeansDefinition()
+ .ifPresent(bd -> {
+ registerBeans(resource, bd);
+ camelAppCache.put(resource.getLocation(), bd);
+ });
Review Comment:
💡 **preparse now fully parses `<routes>` documents:** Before this change,
preparse only handled `<beans>`/`<camel>` documents; `<routes>` documents were
parsed only in `configure()`. Now, `<routes>` documents are fully parsed during
preparse (to extract semantic declarations), with results cached in
`routesCache` and reused in `configure()`. This correctly avoids double-parsing
and ensures semantic questions from co-loaded resources exist before route
references initialize. Clean approach.
##########
core/camel-core-model/src/main/java/org/apache/camel/model/app/SemanticDefinition.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.model.app;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import jakarta.xml.bind.annotation.XmlAccessType;
+import jakarta.xml.bind.annotation.XmlAccessorType;
+import jakarta.xml.bind.annotation.XmlElement;
+import jakarta.xml.bind.annotation.XmlType;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.spi.Metadata;
+import org.apache.camel.spi.Resource;
+
+/** Named, provider-independent semantic question declarations. */
+@Metadata(label = "configuration")
+@XmlType(name = "semanticDefinition")
+@XmlAccessorType(XmlAccessType.FIELD)
+public class SemanticDefinition {
+ @XmlElement(name = "question")
+ @Metadata(description = "Named semantic questions shared by routes in this
Camel context.")
+ private List<SemanticQuestionDefinition> questions = new ArrayList<>();
+
+ public List<SemanticQuestionDefinition> getQuestions() {
+ return questions;
+ }
+
+ public void setQuestions(List<SemanticQuestionDefinition> questions) {
+ this.questions = questions;
+ }
+
+ /** Declare a named semantic question. */
+ public SemanticQuestionDefinition question(String name) {
+ SemanticQuestionDefinition question = new SemanticQuestionDefinition();
+ question.setName(name);
+ questions.add(question);
+ return question;
+ }
+
Review Comment:
💡 **Thread safety of lazy configurer discovery:** The `getContextPlugin` →
null-check → `addContextPlugin` sequence is a TOCTOU pattern. If two threads
both call `configure()` concurrently with a null plugin, both will discover the
configurer and call `addContextPlugin`. In practice, route loading is
single-threaded in Camel's lifecycle, so this is low risk — but worth a brief
comment documenting that assumption, or switching to `computeIfAbsent` on the
context plugin registry if that API exists.
##########
core/camel-core-model/src/main/java/org/apache/camel/model/app/SemanticQuestionDefinition.java:
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.model.app;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import jakarta.xml.bind.annotation.XmlAccessType;
+import jakarta.xml.bind.annotation.XmlAccessorType;
+import jakarta.xml.bind.annotation.XmlAttribute;
+import jakarta.xml.bind.annotation.XmlElement;
+import jakarta.xml.bind.annotation.XmlType;
+
+import org.apache.camel.model.PropertyDefinition;
+import org.apache.camel.spi.Metadata;
+
+/** A semantic question with the same fields and policies as a YAML
declaration. */
+@Metadata(label = "configuration")
+@XmlType(name = "semanticQuestionDefinition", propOrder = { "instructions",
"criteria", "levels" })
+@XmlAccessorType(XmlAccessType.FIELD)
+public class SemanticQuestionDefinition {
+ @XmlAttribute(required = true)
+ @Metadata(required = true, description = "The context-wide question name.")
+ private String name;
+ @XmlAttribute(required = true)
+ @Metadata(required = true, enums = "boolean,choice,score", description =
"The question type.")
+ private String type;
+ @XmlAttribute
+ @Metadata(description = "The Simple expression selecting the message
state.")
+ private String state;
+ @XmlAttribute
+ @Metadata(description = "The boolean decision threshold.")
+ private String threshold;
+ @XmlAttribute
+ @Metadata(description = "The boolean uncertainty band.")
+ private String uncertainty;
+ @XmlAttribute
+ @Metadata(enums = "fail,non-match", description = "The boolean uncertainty
policy.")
+ private String uncertaintyPolicy;
+ @XmlElement(required = true)
+ @Metadata(required = true, description = "Instructions describing the
judgment to make.")
+ private String instructions;
+ @XmlElement(name = "criterion")
+ @Metadata(description = "Named choice criteria, or optional true/false
boolean criteria.")
+ private List<PropertyDefinition> criteria = new ArrayList<>();
+ @XmlElement(name = "level")
Review Comment:
💡 **Mutable internal collections:** `getCriteria()` and `getLevels()` return
the internal `ArrayList` directly. This is consistent with Camel's model class
patterns (e.g., `RouteDefinition.getOutputs()`) where model objects are mutable
builders, but callers adding elements via the fluent API (`criterion()`,
`level()`) while another thread reads the list would cause a
`ConcurrentModificationException`. Since model objects are built in a single
thread during `configure()`, this follows existing Camel conventions — no
change needed.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/DefaultSemanticDefinitionConfigurer.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.semantic;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.model.PropertyDefinition;
+import org.apache.camel.model.app.SemanticDefinition;
+import org.apache.camel.model.app.SemanticDefinitionConfigurer;
+import org.apache.camel.model.app.SemanticQuestionDefinition;
+import org.apache.camel.spi.Resource;
+import org.apache.camel.util.StringHelper;
+
+/** Converts Java/XML declarations to the same immutable questions used by
YAML and programmatic registration. */
+public class DefaultSemanticDefinitionConfigurer implements
SemanticDefinitionConfigurer {
+ @Override
+ public void configure(CamelContext context, Resource resource, String
source, SemanticDefinition definition) {
+ Map<String, SemanticQuestion> questions = new LinkedHashMap<>();
+ if (definition != null) {
+ for (SemanticQuestionDefinition question :
definition.getQuestions()) {
+ String name = question.getName();
+ if (name == null || name.isBlank()) {
+ throw new IllegalArgumentException("Semantic question
requires a nonblank name");
Review Comment:
💡 **Threshold parsing:** `Double.parseDouble()` on the threshold/uncertainty
strings will throw `NumberFormatException` for non-numeric input like `"abc"`,
which would bubble up as-is rather than as a clean `IllegalArgumentException`.
The `SemanticQuestion` constructor does validate `Double.isFinite()` (catching
`NaN` from `Double.parseDouble("NaN")`), and the test suite covers
`threshold="NaN"` → `"within [0,1]"`. However, truly non-numeric strings would
produce a less informative `NumberFormatException`. Consider wrapping with a
try-catch:
```suggestion
definition.getThreshold() == null ? 0.5 :
parseDouble(definition.getThreshold(), "threshold"),
definition.getUncertainty() == null ? 0 :
parseDouble(definition.getUncertainty(), "uncertainty"),
```
With a helper:
```java
private static double parseDouble(String value, String field) {
try {
return Double.parseDouble(value);
} catch (NumberFormatException e) {
throw new IllegalArgumentException(field + " must be a valid number:
" + value, e);
}
}
```
This would produce `"Invalid semantic question 'q': threshold must be a
valid number: abc"` instead of `NumberFormatException: For input string: "abc"`.
--
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]
