davsclaus commented on code in PR #26813:
URL: https://github.com/apache/camel/pull/26813#discussion_r4091285414
##########
core/camel-core-model/src/main/java/org/apache/camel/model/BasicExpressionNode.java:
##########
@@ -36,7 +36,7 @@ public abstract class BasicExpressionNode<T extends
BasicExpressionNode<T>> exte
@Metadata(required = true,
description = "Expression used as the predicate to evaluate
whether this when should trigger and route the message or not.")
- @XmlElementRef
+ @XmlElementRef(required = false)
Review Comment:
This is the change I would most like to see reverted, and the scope split
removes it.
Relaxing `@XmlElementRef` to `required = false` turns `<xs:choice>` into
`<xs:choice minOccurs="0"/>` in both `camel-xml-io.xsd` and `camel-spring.xsd`.
`WhenDefinition` is the only subclass, so the blast radius is contained — but
the effect is that a `<when>` with no predicate stops being a schema error and
becomes a route-build error instead, and every XML author and IDE loses that
validation at edit time. That is a real regression in the editing experience
for a twenty-year-old element.
A separate EIP with its own branch element carrying a `value` attribute
leaves `WhenDefinition.expression` required and this line untouched.
##########
core/camel-core-model/src/main/java/org/apache/camel/model/ChoiceDefinition.java:
##########
@@ -45,10 +47,14 @@
description = "Routes messages to different steps based on a series
of conditions (predicates),"
+ " similar to if-elseif-else in Java. Each condition
is evaluated in order until one matches.")
@XmlRootElement(name = "choice")
-@XmlType(propOrder = { "whenClauses", "otherwise" })
+@XmlType(propOrder = { "selector", "whenClauses", "otherwise" })
@XmlAccessorType(XmlAccessType.FIELD)
public class ChoiceDefinition extends NoOutputDefinition<ChoiceDefinition> {
+ @XmlElement
+ @Metadata(description = "Expression evaluated once per entry into this
choice. Its String result is matched against literal when values. Cannot be
combined with precondition mode or predicate branches.")
+ @DslArg
+ private ExpressionSubElementDefinition selector;
Review Comment:
**Blocking — please take this out of the PR.** This field, the
`when(String)` method, the `@XmlType(propOrder)` reordering and the
`preCreateProcessor()` null-skip together change the model of a foundational
EIP that has been stable for twenty years, along with its generated
`choice.json`, its XSD block and its YAML schema.
`camel-semantic` does not need it — `choice().when(semantic("ref:isSpam"))`
already works against unmodified Choice, and a category question routes with
`setHeader` plus `simple`.
When the value-dispatch EIP is proposed on its own ticket, a standalone
definition is entirely achievable: `OtherwiseDefinition` is already a
standalone `@XmlRootElement` that `ChoiceDefinition` references through a plain
`@XmlElement` field, so a new parent can reuse it unchanged, and the
registration points in `ProcessorReifier` and `DefaultManagementObjectStrategy`
are additive branches. Please do not reach for a shared base class — inserting
an abstract parent between `ChoiceDefinition` and `NoOutputDefinition` changes
choice's `<xs:extension base=...>` in the generated XSD, which is itself a
change to Choice.
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/ChoiceProcessor.java:
##########
@@ -71,7 +97,7 @@ public boolean process(final Exchange exchange, final
AsyncCallback callback) {
// as we should only pick one processor
boolean matches = false;
try {
- matches = filter.matches(exchange);
+ matches = (selector == null || values.get(i).equals(selected))
&& filter.matches(exchange);
Review Comment:
Two things here, both resolved by the split.
**Choice's hot path.** `ChoiceProcessor.process()` is one of the
most-executed methods in Camel. Adding a selector branch and a per-iteration
`selector == null` guard to it, for a feature most routes never use, is exactly
the kind of change a foundational EIP should not have to absorb.
**It is also the slower design.** To reach branch N this builds a
`FilterProcessor` per branch wrapping an `exchange -> true` predicate, so it
costs N calls to `filter.matches()` plus N calls to
`MessageHelper.resetStreamCache()` on the way — and those resets are pure
waste, since a predicate that ignores the exchange never reads the body. A
dedicated processor for the follow-up EIP can be a `Map<String, Processor>`
lookup: O(1), no dummy predicates, no resets.
One detail worth carrying forward when you build that processor: reusing
`FilterProcessor` here is what keeps `getFilteredCount()` populated for the JMX
`extendedInformation()` table, so a standalone processor will need to track
per-branch counts itself.
##########
core/camel-core-reifier/src/main/java/org/apache/camel/reifier/ChoiceReifier.java:
##########
@@ -49,6 +52,25 @@ public ChoiceReifier(Route route, ProcessorDefinition<?>
definition) {
@Override
public Processor createProcessor() throws Exception {
final boolean isPrecondition = Boolean.TRUE ==
parseBoolean(definition.getPrecondition());
+ boolean selecting = definition.getSelector() != null;
+ if (selecting && isPrecondition) {
+ throw new IllegalArgumentException("Choice selector cannot be
combined with precondition mode");
+ }
+ Set<String> uniqueValues = new HashSet<>();
+ for (WhenDefinition when : definition.getWhenClauses()) {
+ if (selecting) {
+ if (when.getValue() == null || when.getExpression() != null) {
+ throw new IllegalArgumentException("Choice selector
requires literal when values without predicates");
+ }
+ if (!uniqueValues.add(parseString(when.getValue()))) {
+ throw new IllegalArgumentException("Choice selector has
duplicate when values");
+ }
+ } else if (when.getValue() != null || when.getExpression() ==
null) {
+ throw new IllegalArgumentException("Choice without a selector
requires when predicates without values");
+ }
+ }
+ Expression selector = selecting ?
createExpression(definition.getSelector().getExpressionType()) : null;
Review Comment:
This file comes out with the scope split, but the point is worth carrying to
the follow-up EIP: `definition.getSelector().getExpressionType()` is
dereferenced without a null check, so a `<selector/>` element with no nested
expression NPEs during route build instead of producing one of the clear
`IllegalArgumentException`s this method raises for every other malformed
combination just above.
Given how carefully the surrounding validation reports the
selector/predicate/precondition conflicts, a matching message would be
consistent — something along the lines of "selector requires an expression".
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/SemanticQuestions.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.HashMap;
+import java.util.Map;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.spi.Resource;
+
+/** Context-local named questions, replaced atomically per source when a route
resource is reloaded. */
+public final class SemanticQuestions {
+ private final Map<String, Map<String, SemanticQuestion>> sources = new
HashMap<>();
+ private final Map<String, Resource> resources = new HashMap<>();
+ private volatile Map<String, SemanticQuestion> questions = Map.of();
+
+ public static SemanticQuestions get(CamelContext context) {
+ synchronized (context) {
Review Comment:
Locking on the `CamelContext` instance itself. It is a shared, publicly
reachable object, so any other code that ever locks on the same monitor can
deadlock against this, and the lock is held far wider than the map access it is
protecting.
I grepped `core/` and `components/` excluding tests: there is **no other
non-test code in the codebase that synchronizes on a `CamelContext`**, so this
is a new pattern rather than an existing convention. SonarCloud flags it as
java:S2445 ("blocks should be synchronized on private final fields").
A private static lock, or a compute-if-absent style access on the context
plugin map, would scope this correctly. The same applies to `synchronized
(context.getRegistry())` in `SemanticLanguage.adapter()` — likewise with no
precedent elsewhere in the tree.
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-deserializers/pom.xml:
##########
@@ -38,6 +38,10 @@
</properties>
<dependencies>
+ <dependency>
+ <groupId>org.apache.camel</groupId>
+ <artifactId>camel-semantic</artifactId>
Review Comment:
**Blocking (the dependency-direction question you raised).** A compile-scope
dependency here makes `camel-semantic` — and its `camel-core-languages`
dependency — transitive for *every* YAML DSL user: JBang, Spring Boot, Quarkus,
Kamelets. Combined with the unconditional
`SemanticDefinitionDeserializer.configure(...)` call in
`YamlRoutesBuilderLoader.preConfigureNode`, every YAML resource pays a semantic
preparse pass whether or not it declares `semantic:`.
The downstream consequence you asked about is real: `camel-quarkus-yaml-dsl`
would pull in an artifact with no corresponding Quarkus extension, and the
camel-spring-boot starter dependency graph changes for all YAML users.
Suggested direction: resolve the `semantic` node lazily through
`FactoryFinder`/SPI so this module keeps no compile-scope dependency on an AI
component, and express the resource-wide preparse hook as an interface in
`camel-yaml-dsl-common` that `camel-semantic` implements. That keeps the
layering pointing the usual way — components depend on the DSL, not the reverse.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/language/semantic/SemanticLanguageConfigurer.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.language.semantic;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.spi.PropertyConfigurer;
+
+/** Preserves adapter references so the language can validate their type and
manage only instances it creates. */
+public class SemanticLanguageConfigurer implements PropertyConfigurer {
+ @Override
+ public boolean configureRaw(CamelContext context, Object target, String
name, Object value, boolean ignoreCase) {
+ if (ignoreCase ? "adapter".equalsIgnoreCase(name) :
"adapter".equals(name)) {
+ if (!(value instanceof String)) {
+ throw new IllegalArgumentException("Semantic adapter selection
must be a bean reference or class name");
+ }
+ ((SemanticLanguage) target).setAdapter((String) value);
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public boolean configure(CamelContext context, Object target, String name,
Object value, boolean ignoreCase) {
Review Comment:
`configure()` returns `false` unconditionally, so `defaultState` — and
`adapter` on any path that does not go through `configureRaw` — always falls
back to reflection binding in `PropertyBindingSupport`. Generated configurers
exist precisely to keep configuration reflection-free for GraalVM native and
`camel-main` fast startup; this hand-written one gives that up for the whole
language.
Two related points:
- No other language in the repo ships a `PropertyConfigurer` at all — I
could not find a single `*-language` entry under any
`META-INF/services/org/apache/camel/configurer/` directory. This is a new
pattern for languages, not an existing one being followed.
- The two service files are hand-maintained under `src/main/resources`,
whereas every other configurer service file in the tree lives under
`src/generated/resources` and is owned by the package plugin.
If the `configureRaw` hook is dropped per the `PropertyConfigurer` comment,
this class and both service files disappear with it. If it is kept,
`configure()` should at least bind `defaultState` properly rather than forcing
reflection.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/language/semantic/SemanticLanguage.java:
##########
@@ -0,0 +1,307 @@
+/*
+ * 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.language.semantic;
+
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.CamelContextAware;
+import org.apache.camel.Exchange;
+import org.apache.camel.Expression;
+import org.apache.camel.Predicate;
+import org.apache.camel.RuntimeCamelException;
+import org.apache.camel.semantic.SemanticAdapter;
+import org.apache.camel.semantic.SemanticQuestion;
+import org.apache.camel.semantic.SemanticQuestions;
+import org.apache.camel.semantic.SemanticResult;
+import org.apache.camel.spi.Metadata;
+import org.apache.camel.spi.annotations.Language;
+import org.apache.camel.support.ExpressionAdapter;
+import org.apache.camel.support.LanguageSupport;
+import org.apache.camel.support.service.ServiceHelper;
+import org.apache.camel.support.service.ServiceSupport;
+
+/** Evaluates a named, provider-independent question against selected message
state. */
+@Language(value = "semantic", modelName = "language")
+@Metadata(title = "Semantic", description = "Evaluate named semantic questions
through a provider adapter",
+ label = "language,ai", firstVersion = "4.23.0")
+public class SemanticLanguage extends LanguageSupport {
+ public static final String RESULT = "CamelSemanticResult";
+ public static final String ADAPTER_NAME = "camelSemanticAdapter";
+ public static final String ADAPTER_RESOURCE =
"META-INF/services/org.apache.camel.semantic.SemanticAdapter";
+
+ private String adapter;
+ private String defaultState = "${body}";
+ private SemanticAdapter selectedAdapter;
+
+ public String getAdapter() {
+ return adapter;
+ }
+
+ /**
+ * Adapter registry reference (#bean:name) or implementation class (plain
FQCN or #class:FQCN). Absent selects the
+ * sole advertised adapter.
+ */
+ public void setAdapter(String adapter) {
+ this.adapter = adapter;
+ }
+
+ public String getDefaultState() {
+ return defaultState;
+ }
+
+ /** Default Simple state selector for questions without their own
selector. Defaults to the body. */
+ public void setDefaultState(String defaultState) {
+ this.defaultState = defaultState;
+ }
+
+ @Override
+ public Expression createExpression(String expression) {
+ return createEvaluation(expression, false);
+ }
+
+ @Override
+ public Predicate createPredicate(String expression) {
+ return createEvaluation(expression, true);
+ }
+
+ public boolean validateExpression(String expression) {
+ if (expression == null || !expression.startsWith("ref:") ||
expression.substring(4).isBlank()) {
+ throw new IllegalArgumentException("Semantic expression must
reference a named question using ref:name");
+ }
+ return true;
+ }
+
+ public boolean validatePredicate(String expression) {
+ return validateExpression(expression);
+ }
+
+ private Evaluation createEvaluation(String expression, boolean predicate) {
+ validateExpression(expression);
+ Evaluation evaluation = new Evaluation(expression.substring(4),
predicate);
+ if (getCamelContext() != null) {
+ evaluation.init(getCamelContext());
+ }
+ return evaluation;
+ }
+
+ private synchronized SemanticAdapter adapter() {
+ if (selectedAdapter != null) {
+ return selectedAdapter;
+ }
+ CamelContext context = getCamelContext();
+ String configured = adapter == null ? null :
context.resolvePropertyPlaceholders(adapter);
+ if (configured != null && configured.startsWith("#") &&
!configured.startsWith("#class:")) {
+ String name = configured.startsWith("#bean:") ?
configured.substring(6) : configured.substring(1);
+ if (name.contains(":")) {
+ throw new IllegalArgumentException("Semantic adapter reference
must use #bean:name, #name or #class:FQCN");
+ }
+ Object bean = context.getRegistry().lookupByName(name);
+ if (!(bean instanceof SemanticAdapter found)) {
+ throw new IllegalArgumentException("Semantic adapter bean is
missing or does not implement SemanticAdapter");
+ }
+ selectedAdapter = found;
+ return found;
+ }
+ ManagedAdapter owned = null;
+ try {
+ String className = configured;
+ if (className == null) {
+ Set<String> candidates = new TreeSet<>();
+ Enumeration<URL> resources =
context.getClassResolver().loadAllResourcesAsURL(ADAPTER_RESOURCE);
Review Comment:
This hand-rolls service-file discovery: `loadAllResourcesAsURL(...)`, then
reading each URL and splitting lines on `#` to strip comments. The resource
path chosen (`META-INF/services/org.apache.camel.semantic.SemanticAdapter`) is
exactly the JDK `ServiceLoader` convention, and Camel already has
`FactoryFinder` for the same job with class-resolver and injector integration.
Two concrete consequences of doing it by hand:
- GraalVM native will not register the resource automatically the way it
does for `ServiceLoader`/`FactoryFinder` providers, so adapter discovery is
likely to fail in native mode without an explicit resource registration.
- The manual comment-stripping and trimming reimplements parsing rules
`ServiceLoader` already specifies.
If the reason for reading names rather than loading instances is to
type-check before construction, `FactoryFinder.findClass(...)` gives you the
`Class` without instantiating it.
##########
core/camel-api/src/main/java/org/apache/camel/spi/PropertyConfigurer.java:
##########
@@ -37,6 +37,24 @@
*/
public interface PropertyConfigurer {
+ /**
+ * Optionally binds a value before resolving bean or class references.
Property placeholders are resolved before
+ * this call when placeholder resolution was requested by the caller. This
allows an option that owns reference
+ * resolution and lifecycle to retain the reference text. Returning false
leaves the normal binding behavior
+ * unchanged.
+ *
+ * @param camelContext the Camel context
+ * @param target the target instance
+ * @param name the property name
+ * @param value the value before reference resolution
+ * @param ignoreCase whether to ignore case for matching the property
name
+ * @return true if the property was configured
+ * @since 4.23
+ */
+ default boolean configureRaw(CamelContext camelContext, Object target,
String name, Object value, boolean ignoreCase) {
Review Comment:
**Blocking (the SPI question you raised).** I think this addition can be
avoided entirely, which would be the better outcome.
`PropertyBindingSupport.resolveValue` delegates string values to
`resolveBean`, and `resolveBean` only transforms a string **that starts with
`#`** — a plain FQCN or a plain registry bean name passes through binding
completely untouched. So `setAdapter("com.foo.MyAdapter")` and
`setAdapter("myAdapterBean")` already reach the language as raw text, with no
instantiation and no SPI hook at all.
That means this hook is needed *only* to preserve the `#bean:` and `#class:`
prefixed spellings documented in `semantic-language.adoc`. If the language
accepts just the unprefixed spellings, you get the same
type-check-before-instantiation contract and can drop:
- this `camel-api` SPI method and its `PropertyBindingSupport` call site,
- the hand-written `SemanticLanguageConfigurer`,
- the two hand-maintained configurer service files under
`src/main/resources`.
Weighed against that, the cost of keeping it is a permanent public SPI
method on `camel-api` invoked on every `doSetPropertyValue` for every
configurer in the framework, in order to serve one option of one preview
language. I do not think that trade holds up — but flagging it for the PMC
rather than deciding unilaterally, since you asked for agreement.
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-deserializers/src/main/java/org/apache/camel/dsl/yaml/deserializers/SemanticDefinitionDeserializer.java:
##########
@@ -0,0 +1,188 @@
+/*
+ * 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.dsl.yaml.deserializers;
+
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.dsl.yaml.common.YamlDeserializationContext;
+import org.apache.camel.dsl.yaml.common.YamlDeserializerSupport;
+import org.apache.camel.dsl.yaml.common.exception.YamlDeserializationException;
+import org.apache.camel.semantic.SemanticQuestion;
+import org.apache.camel.semantic.SemanticQuestions;
+import org.apache.camel.spi.CamelContextCustomizer;
+import org.apache.camel.spi.annotations.YamlIn;
+import org.apache.camel.spi.annotations.YamlProperty;
+import org.apache.camel.spi.annotations.YamlType;
+import org.snakeyaml.engine.v2.api.ConstructNode;
+import org.snakeyaml.engine.v2.nodes.Node;
+import org.snakeyaml.engine.v2.nodes.NodeTuple;
+import org.snakeyaml.engine.v2.nodes.SequenceNode;
+
+/** Named semantic declarations are installed in a resource-wide pass before
route references are resolved. */
+@YamlIn
+@YamlType(nodes = "semantic", properties = {
+ @YamlProperty(name = "question",
+ type =
"map:org.apache.camel.dsl.yaml.deserializers.SemanticDefinitionDeserializer$QuestionSchema",
+ required = true)
+})
+public class SemanticDefinitionDeserializer extends YamlDeserializerSupport
implements ConstructNode {
+ private static final Set<String> FIELDS
+ = Set.of("type", "instructions", "state", "criteria", "threshold",
"uncertainty", "uncertaintyPolicy");
+
+ @Override
+ public Object construct(Node node) {
+ read(node);
+ // Registration happens once for the entire resource, including
declarations after routes.
+ return (CamelContextCustomizer) context -> {
+ };
+ }
+
+ public static void configure(CamelContext context,
YamlDeserializationContext dc, Node root) {
+ if (!(root instanceof SequenceNode sequence)) {
+ return;
+ }
+ Map<String, SemanticQuestion> definitions = new LinkedHashMap<>();
+ for (Node node : sequence.getValue()) {
+ for (NodeTuple tuple : asMappingNode(node).getValue()) {
+ if ("semantic".equals(asText(tuple.getKeyNode()))) {
+ read(tuple.getValueNode()).forEach((name, question) -> {
+ if (definitions.putIfAbsent(name, question) != null) {
+ throw new IllegalArgumentException("Duplicate
semantic question: " + name);
+ }
+ });
+ }
+ }
+ }
+ SemanticQuestions.get(context).replace(dc.getResource(), definitions);
+ }
+
+ private static Map<String, SemanticQuestion> read(Node node) {
+ Map<String, Node> semantic = fields(node);
+ if (!semantic.keySet().equals(Set.of("question"))) {
+ throw new IllegalArgumentException("Semantic declaration requires
only question");
+ }
+ Map<String, SemanticQuestion> result = new LinkedHashMap<>();
+ fields(semantic.get("question")).forEach((name, definition) -> {
+ Map<String, Node> values = fields(definition);
+ if (!FIELDS.containsAll(values.keySet())) {
+ throw new IllegalArgumentException("Unknown property in
semantic question: " + name);
+ }
+ String typeName = asText(values.get("type"));
+ if (typeName == null) {
+ throw new IllegalArgumentException("Semantic question type is
required: " + name);
+ }
+ SemanticQuestion.Type type =
SemanticQuestion.Type.valueOf(typeName.toUpperCase(Locale.ROOT));
Review Comment:
The located-diagnostic fix from the previous round reached `number(...)` but
not the enum parsing in the same file. `type: foo` throws a bare
`IllegalArgumentException: No enum constant
org.apache.camel.semantic.SemanticQuestion.Type.FOO` with no question name,
resource or line/column — exactly the failure mode that was just fixed for
`threshold`.
Same class of gap at lines 109-110:
`UncertaintyPolicy.valueOf(asText(values.get("uncertaintyPolicy")).replace(...))`
**NPEs** when the key is present but the value is empty, since `asText`
returns null before `.replace` is called.
Also unlocated in this file: `"Duplicate semantic question: "`, `"Unknown
property in semantic question: "`, `"Semantic question type is required: "` and
`"Semantic declaration requires only question"`. Schema validation catches most
of these when enabled, but the parameterised tests added in this PR
deliberately load without schema validation, so the path is reachable in
practice.
Wrapping these the same way `number(...)` now does —
`YamlDeserializationException` with the offending `Node` — would make the whole
file consistent.
##########
core/camel-management/src/main/java/org/apache/camel/management/mbean/ManagedChoice.java:
##########
@@ -76,8 +76,9 @@ public TabularData extendedInformation() {
FilterProcessor filter = filters.get(i);
CompositeType ct = CamelOpenMBeanTypes.choiceCompositeType();
- String predicate = when.getExpression().getExpression();
- String language = when.getExpression().getLanguage();
+ String predicate = when.getValue() != null ? when.getValue() :
when.getExpression().getExpression();
+ String language = getDefinition().getSelector() != null
+ ?
getDefinition().getSelector().getExpressionType().getLanguage() :
when.getExpression().getLanguage();
Review Comment:
Same null dereference as in the reifier:
`getDefinition().getSelector().getExpressionType().getLanguage()` assumes the
selector always carries an expression type. Here it is worse than a build-time
failure, because it surfaces while a JMX client is reading
`extendedInformation()` — a monitoring call failing with an NPE is hard to
diagnose.
This file reverts entirely with the scope split; the null-safety point
applies to the new MBean that the follow-up EIP will need.
--
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]
