gnodet-bot commented on code in PR #26513:
URL: https://github.com/apache/camel/pull/26513#discussion_r4026076479
##########
core/camel-core-model/src/main/java/org/apache/camel/model/BeanModelHelper.java:
##########
@@ -129,7 +142,7 @@ public static Object newInstance(BeanFactoryDefinition def,
CamelContext context
}
// do not set properties when using #type as it uses an existing
shared bean
- boolean setProps = !type.startsWith("#type");
+ boolean setProps = type == null || !type.startsWith("#type");
Review Comment:
⚠️ **`bind()` gap — builder-without-type silently broken in route-template
local beans**
This `setProps` change fixes `newInstance()` (used by top-level
`<beans><bean>` and YAML `beans:`). But `bind()` — used by route-template and
kamelet local beans — was **not changed**. Its dispatch chain is:
```java
} else if (def.getScript() != null && def.getScriptLanguage() != null) {
// script path — works, already uses Object.class fallback
} else if (def.getBeanClass() != null || def.getType() != null) {
// class/type path
} else {
throw new IllegalArgumentException(
"Route template local bean: " + def.getName() + " has invalid type
syntax: " + def.getType() ...);
}
```
When `builderClass` is set but `type=null, beanClass=null`, none of the
`else if` branches match, so `bind()` throws the confusing IAE "invalid type
syntax: null". This means **a `builderClass` bean without `type` inside a
`<routeTemplate>` or kamelet YAML still fails at runtime** — the fix is
incomplete for that axis.
The PR description says the change covers both `newInstance()` and the
route-template path, but no `builderClass` branch was added to `bind()`.
`camel-app17.xml` tests the builder path at the top-level `<beans>` level only
(goes through `newInstance()`); the routeTemplate validator YAML file only
tests `builderClass` as a top-level `beans:` bean (schema validation only, no
runtime `bind()` call).
`bind()` needs the same `else if (def.getBuilderClass() != null)` branch
that `newInstance()` already has.
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-deserializers/src/main/java/org/apache/camel/dsl/yaml/deserializers/BeansDeserializer.java:
##########
@@ -69,14 +69,18 @@ public Object construct(Node node) {
}
ObjectHelper.notNull(bean.getName(), "The bean name must be set");
- ObjectHelper.notNull(bean.getType(), "The bean type must be set");
- if (!bean.getType().startsWith("#class:")) {
- bean.setType("#class:" + bean.getType());
- }
- if (bean.getScriptLanguage() != null || bean.getScript() != null) {
+ boolean script = bean.getScriptLanguage() != null ||
bean.getScript() != null;
Review Comment:
🔍 **`||` here vs `&&` in `newInstance()` — asymmetry worth aligning**
`BeansDeserializer` uses `||` to detect a script bean:
```java
boolean script = bean.getScriptLanguage() != null || bean.getScript() !=
null;
```
`newInstance()` uses `&&`:
```java
boolean script = def.getScriptLanguage() != null && def.getScript() != null;
```
The end result is the same because the `notNull` guards on lines 74-75
enforce both fields — a user who supplies only one will get an error either
way. But the intent reads better with `&&` in both places: "a bean is
considered scripted only when **both** language and script body are provided".
The `||` form with the guards is logically equivalent but inverts the meaning
of the variable then immediately corrects it with two nullchecks.
Consider aligning to `&&` for consistency:
```suggestion
boolean script = bean.getScriptLanguage() != null &&
bean.getScript() != null;
```
##########
components/camel-spring-parent/camel-spring-xml/src/test/resources/org/apache/camel/spring/routebuilder/SpringTemplatedRouteScriptBeanNoTypeTest.xml:
##########
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ 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.
+
+-->
+<beans xmlns="http://www.springframework.org/schema/beans";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="
+ http://camel.apache.org/schema/spring
http://camel.apache.org/schema/spring/camel-spring.xsd
+ http://www.springframework.org/schema/beans
http://www.springframework.org/schema/beans/spring-beans.xsd";>
+
+ <camelContext id="foo" xmlns="http://camel.apache.org/schema/spring";>
+ <routeTemplate id="myTemplate" description="blah blah">
+ <templateParameter name="foo"/>
+ <templateParameter name="bar"/>
+ <route>
+ <from uri="direct:{{foo}}"/>
+ <to uri="bean:{{myScriptBean}}"/>
+ <to uri="bean:{{myBean}}"/>
+ <to uri="mock:{{bar}}"/>
+ </route>
+ </routeTemplate>
+ <templatedRoute routeTemplateRef="myTemplate" routeId="my-route">
+ <parameter name="foo" value="fooVal"/>
+ <parameter name="bar" value="barVal"/>
+ <bean name="myBean"
type="#class:org.apache.camel.spring.routebuilder.SpringTemplatedRouteScriptBeanNoTypeTest$MySpecialBean">
+ <properties>
+ <property key="name" value="John"/>
+ </properties>
+ </bean>
+ <!-- a bean created by a script needs no type (class name); the
XSD must allow it -->
+ <bean name="myScriptBean" scriptLanguage="bean">
+
<script>org.apache.camel.spring.routebuilder.SpringTemplatedRouteScriptBeanNoTypeTest$MyScriptBean?method=create</script>
+ </bean>
+ </templatedRoute>
+ <route>
+ <from uri="direct:a"/>
+ <to uri="log:foo"/>
+ </route>
+ </camelContext>
+
+</beans>
Review Comment:
nit: Missing newline at end of file (`\ No newline at end of file` in the
diff). All other XML fixtures in this module have a trailing newline.
--
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]
