gnodet-bot commented on code in PR #13139:
URL: https://github.com/apache/maven/pull/13139#discussion_r4015181143
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/profile/ConditionParser.java:
##########
@@ -379,15 +419,24 @@ private Object parseVariableOrUnknownFunction() {
/**
* Parses a list of arguments for a function call.
+ * For the {@code if} function, only the branch selected by the condition
is evaluated,
+ * the other one is skipped and passed as {@code null}.
*
+ * @param functionName the name of the called function
* @return a list of parsed arguments
* @throws RuntimeException if there's a mismatch in parentheses
*/
- private List<Object> parseArgumentList() {
+ private List<Object> parseArgumentList(String functionName) {
List<Object> args = new ArrayList<>();
current++; // Skip the opening parenthesis
while (current < tokens.size() && !tokens.get(current).equals(")")) {
- args.add(parseLogicalOr());
+ int index = args.size();
+ if ("if".equals(functionName) && (index == 1 || index == 2) &&
toBoolean(args.get(0)) != (index == 1)) {
Review Comment:
⚠️ **Design coupling: `"if"` hardcoded in a function-agnostic parser**
`ConditionParser` receives its function set as an injected `Map<String,
ExpressionFunction>` — the parser is supposed to be agnostic about which
functions exist. This line breaks that contract: the parser now needs to know
the *name* `"if"` to apply lazy evaluation.
If a second function needing lazy evaluation is added (e.g., `"unless"`,
`"cond"`, a short-circuit `"and"`), another magic-string guard has to be added
here.
Two paths forward:
1. **Minimal (in-PR):** Extract the string to a constant to make the
coupling visible:
```suggestion
if ("if".equals(functionName) && (index == 1 || index == 2) &&
toBoolean(args.get(0)) != (index == 1)) {
```
*(No behaviour change — but at least the magic string is a named constant
in the companion commit, or a `private static final String IF_FUNCTION = "if"`)*
2. **Proper long-term:** Introduce a `LazyExpressionFunction` sub-interface
whose `apply` receives a `Supplier<Object>` or an unevaluated index range, so
the parser can dispatch lazily without knowing function names.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/model/profile/ConditionParserTest.java:
##########
@@ -182,6 +182,40 @@ void testIfFunction() {
assertEquals("short", parser.parse("if(length('hi') > 3, 'long',
'short')"));
}
+ /**
+ * Only the selected branch of {@code if(..)} is evaluated. The other one
may not be valid for
+ * the current input, as in the example from the {@code condition}
documentation: with a
+ * {@code java.version} that has no {@code -}, the unselected {@code
substring(..)} would get an
+ * end index of -1.
+ */
+ @Test
+ void testIfFunctionOnlyEvaluatesSelectedBranch() {
+ assertEquals(
+ "1.8.0_292",
+ parser.parse("if(contains(${java.version}, '-'), "
+ + "substring(${java.version}, 0,
indexOf(${java.version}, '-')), ${java.version})"));
+ assertEquals(
+ "21",
+ parser.parse("if(contains('21-ea', '-'), substring('21-ea', 0,
indexOf('21-ea', '-')), '21-ea')"));
+ assertThrows(RuntimeException.class, () -> parser.parse("if(true,
'a')"));
+ assertThrows(RuntimeException.class, () -> parser.parse("if(false,
substring('a', 0, 5), 'b'"));
Review Comment:
🔍 **Missing symmetric test case: `if(false, 'a')` (2-arg, false branch)**
The test at line 200 covers `if(true, 'a')` — the 2-arg call where the
*true* branch would be selected but only one argument was supplied. The
symmetric case `if(false, 'a')` — where the *false* branch would be taken — is
not covered. In the new code, when `condition=false` and `index==1`, the guard
`toBoolean(args.get(0)) != (index == 1)` becomes `false != true` → `true`, so
the branch is **skipped** and `null` is appended. The call to `if_()` then
receives `[false, null]` (size 2), which correctly throws — but this path is
not exercised by the test suite.
Suggested addition:
```suggestion
assertThrows(RuntimeException.class, () -> parser.parse("if(true,
'a')"));
assertThrows(RuntimeException.class, () -> parser.parse("if(false,
'a')"));
assertThrows(RuntimeException.class, () -> parser.parse("if(false,
substring('a', 0, 5), 'b'"));
```
--
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]
