You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/03/13 07:36:32 UTC
[PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
gnodet opened a new pull request, #21969:
URL: https://github.com/apache/camel/pull/21969
## Summary
- Adds validation in `BasicOutputExpressionNode.setExpression()` to detect when an expression element (e.g. `<method>`) appears after processing steps inside a `<when>` clause in XML/YAML DSL
- Previously, a `<method>` element placed after `<log>` or other steps in a `<when>` clause would silently overwrite the predicate expression instead of being treated as a processor or raising an error
- The validation correctly skips the check for Java DSL where `ExpressionClause` is legitimately resolved to the actual expression during `preCreateProcessor()`
## Test plan
- Added `testMethodAfterStepsInWhenClauseShouldFail` to verify that placing `<method>` after processing steps in a `<when>` clause raises an error
- Added `testMethodAsPredicateInWhenClauseShouldWork` to verify that `<method>` as the first (and only expression) child of `<when>` still works correctly
- Verified all existing Choice/When tests pass (144 tests in camel-core, 21 tests in camel-xml-io)
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21969:
URL: https://github.com/apache/camel/pull/21969#issuecomment-4053345318
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `build-all`, `build-dependents`, `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #21969:
URL: https://github.com/apache/camel/pull/21969#discussion_r2935839628
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -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"/>
Review Comment:
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.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #21969:
URL: https://github.com/apache/camel/pull/21969#discussion_r2935840734
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -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"/>
Review Comment:
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.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #21969:
URL: https://github.com/apache/camel/pull/21969#discussion_r2930590008
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -449,6 +449,52 @@ 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>";
Review Comment:
Done — switched to text blocks in 577b5cb.
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -449,6 +449,52 @@ 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();
+ });
+ assertTrue(e.getMessage().contains("already has a predicate")
+ || e.getCause().getMessage().contains("already has a predicate"),
+ "Error message should indicate that the when clause already has a predicate: " + e.getMessage());
+ }
+
+ @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\"/>"
+ + " </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());
Review Comment:
Done — replaced with `assertThat(e).hasStackTraceContaining("already has a predicate")` in 577b5cb.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #21969:
URL: https://github.com/apache/camel/pull/21969#discussion_r2932649604
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -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"/>
Review Comment:
Would it be good to add a <bean> here as the exception message tells
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "apupier (via GitHub)" <gi...@apache.org>.
apupier commented on code in PR #21969:
URL: https://github.com/apache/camel/pull/21969#discussion_r2940663294
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -449,6 +449,52 @@ 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();
+ });
+ assertTrue(e.getMessage().contains("already has a predicate")
+ || e.getCause().getMessage().contains("already has a predicate"),
+ "Error message should indicate that the when clause already has a predicate: " + e.getMessage());
+ }
+
+ @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\"/>"
+ + " </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());
Review Comment:
this comment was for another piece of code
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-22884: Validate method attribute placement in when clause [camel]
Posted by "apupier (via GitHub)" <gi...@apache.org>.
apupier commented on code in PR #21969:
URL: https://github.com/apache/camel/pull/21969#discussion_r2929734920
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -449,6 +449,52 @@ 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();
+ });
+ assertTrue(e.getMessage().contains("already has a predicate")
+ || e.getCause().getMessage().contains("already has a predicate"),
+ "Error message should indicate that the when clause already has a predicate: " + e.getMessage());
+ }
+
+ @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\"/>"
+ + " </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());
Review Comment:
using AssertJ assertions will allow to write it in a single assertion and have a more precise error message out of the box
##########
core/camel-xml-io/src/test/java/org/apache/camel/xml/in/ModelParserTest.java:
##########
@@ -449,6 +449,52 @@ 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>";
Review Comment:
using String text block will simplify this part
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org