Skip to content

CAMEL-22884: Validate method attribute placement in when clause #21969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gnodet wants to merge 2 commits into
base: main
Choose a base branch
from
+77 −0
Open

CAMEL-22884: Validate method attribute placement in when clause #21969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d1bc14
CAMEL-22884: Validate method attribute placement in when clause
gnodet Mar 13, 2026
577b5cb
CAMEL-22884: Address review comments - use text blocks and AssertJ
gnodet Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions core/camel-core-model/src/main/java/org/apache/camel/model/BasicOutputExpressionNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,32 @@ public void setOutputs(List<ProcessorDefinition<?>> outputs) {
public void addOutput(ProcessorDefinition<?> output) {
this.outputs.add(output);
}

@Override
public void setExpression(ExpressionDefinition expression) {
// Detect when an expression element (e.g. <method>) appears after processing steps
// inside a when/filter clause in XML/YAML DSL. This is almost certainly a user mistake
// where they intended to use <bean> (processor) instead of <method> (expression/predicate).
// We skip this check when the existing expression wraps an ExpressionClause (Java DSL),
// because preCreateProcessor() legitimately re-sets the expression after resolving it.
if (expression != null && getExpression() != null && !outputs.isEmpty()) {
ExpressionDefinition existing = getExpression();
boolean isExpressionClause
= existing.getExpressionValue() instanceof org.apache.camel.builder.ExpressionClause
|| existing.getPredicate() instanceof org.apache.camel.builder.ExpressionClause;
if (!isExpressionClause) {
String lang = expression.getLanguage() != null
? expression.getLanguage() : expression.getClass().getSimpleName();
throw new IllegalArgumentException(
"The " + getShortName() + " already has a predicate (" + existing
+ ") and " + outputs.size() + " output(s). "
+ "The expression '" + lang
+ "' is being parsed as an expression/predicate but appears after processing steps. "
+ "If you intended to call a bean method as a processing step, use <bean> instead of <method>. "
+ "An expression element must be the first child of <" + getShortName()
+ ">.");
}
}
super.setExpression(expression);
}
}
49 changes: 49 additions & 0 deletions core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
Expand Down Expand Up @@ -449,6 +450,54 @@ public void testParseError() throws Exception {
assertTrue(e.getMessage().startsWith("Unexpected attribute '{}ref'"));
}

@Test
public void testMethodAfterStepsInWhenClauseShouldFail() throws Exception {
String routesXml = """
<routes xmlns="http://camel.apache.org/schema/xml-io">
<route>
<from uri="direct:start"/>
<choice>
<when>
<simple>${header.foo} == 'bar'</simple>
<log message="Condition met"/>
<method ref="someBean" method="processFooBar"/>
</when>
<otherwise>
<to uri="mock:other"/>
</otherwise>
</choice>
</route>
</routes>
""";
Exception e = assertThrows(Exception.class, () -> {
new ModelParser(new StringReader(routesXml)).parseRoutesDefinition();
});
assertThat(e).hasStackTraceContaining("already has a predicate");
}

@Test
public void testMethodAsPredicateInWhenClauseShouldWork() throws Exception {
String routesXml = """
<routes xmlns="http://camel.apache.org/schema/xml-io">
<route>
<from uri="direct:start"/>
<choice>
<when>
<method ref="someBean" method="isFoo"/>
<to uri="mock:foo"/>
Copy link
Contributor

@davsclaus davsclaus Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be good to add a here as the exception message tells

Copy link
Contributor Author

@gnodet gnodet Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea — adding a <bean id="someBean" type="..."/> would make the test XML more realistic, even though this is purely a parser-level test (no runtime resolution). I'll update both test methods.

Copy link
Contributor Author

@gnodet gnodet Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on second thought — <bean> declarations live at the <camel> root level, not inside <routes>. This test uses parseRoutesDefinition() which expects <routes> as root. Restructuring to use a <camel> wrapper would require a different parse method and adds complexity for a purely parser-level test where bean refs aren't resolved. Happy to change it if you still think it's worth it, but I'd lean toward keeping it simple since the test is only validating XML structure parsing, not runtime behavior.

</when>
<otherwise>
<to uri="mock:other"/>
</otherwise>
</choice>
</route>
</routes>
""";
RoutesDefinition routes = new ModelParser(new StringReader(routesXml)).parseRoutesDefinition().orElse(null);
assertNotNull(routes);
assertEquals(1, routes.getRoutes().size());
}

private Path getResourceFolder() {
final URL resource = getClass().getClassLoader().getResource("barInterceptorRoute.xml");
assert resource != null : "Cannot find barInterceptorRoute.xml";
Expand Down
Loading