Repository navigation
Evaluate profile activation conditions lazily - #13139
Conversation
The condition parser evaluated every operand as it parsed it, so both
branches of if(..) and the right-hand side of && and || were always
computed. A branch that is not valid for the current input then fails
the whole condition. The if(..) example from the condition
documentation, substring(${java.version}, 0, indexOf(${java.version},
'-')), throws StringIndexOutOfBoundsException on any java.version
without a '-', and a guard such as length(x) >= 3 cannot protect a
substring(x, 0, 3) that follows it with &&.
Skip the unselected if(..) branch and the right operand of && / || when
the left one already decides the result. Skipped operands are still
checked for balanced parentheses, so malformed expressions keep failing.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Review
Core short-circuit logic is sound and correctly fixes the StringIndexOutOfBoundsException documented in the PR description. The skipOperand depth tracking handles nested parentheses correctly, and the error paths (false &&, true || (false) are properly exercised by the new assertThrows cases. Two design concerns and one test gap below.
Metadata suggestions (for maintainers)
- Label:
bug - Milestone: 4.x
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| 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)) { |
There was a problem hiding this comment.
"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:
- Minimal (in-PR): Extract the string to a constant to make the coupling visible:
(No behaviour change — but at least the magic string is a named constant in the companion commit, or aSuggested change
if ("if".equals(functionName) && (index == 1 || index == 2) && toBoolean(args.get(0)) != (index == 1)) { if ("if".equals(functionName) && (index == 1 || index == 2) && toBoolean(args.get(0)) != (index == 1)) { private static final String IF_FUNCTION = "if") - Proper long-term: Introduce a
LazyExpressionFunctionsub-interface whoseapplyreceives aSupplier<Object>or an unevaluated index range, so the parser can dispatch lazily without knowing function names.
There was a problem hiding this comment.
You're right that this line is the only place where the parser, which otherwise only sees the injected function map, depends on a function name. I took the minimal path in 92a381c: the literal is now private static final String IF_FUNCTION = "if", documented as the function whose unselected branch is skipped, so the coupling is visible from the top of the class. Spotless wrapped the condition over three lines, and ConditionParserTest plus ConditionProfileActivatorTest still pass (76 tests, 3 skipped as before).
I stayed away from a LazyExpressionFunction sub-interface in this PR because it changes the public ExpressionFunction contract, and that feels like a call for the maintainers rather than for a bug fix. If you'd rather have it here, I'm glad to rework the change along those lines.
Replace the "if" literal in parseArgumentList with a named constant, so the one place where the parser depends on a function name is visible, and cover the two-argument if(false, 'a') call next to if(true, 'a').
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review (commits 72e1ffb + 92a381c)
Both findings from the previous review are addressed:
-
"if"coupling — commit 92a381c extracts the literal toprivate static final String IF_FUNCTION = "if", documented as the one place where the parser depends on a function name. The coupling is now explicit and visible. TheLazyExpressionFunctionsub-interface approach is a legitimate design evolution, but deferring it is the right call for a targeted bug fix. -
Missing
if(false, 'a')test — commit 92a381c addsassertThrows(RuntimeException.class, () -> parser.parse("if(false, 'a')"))immediately after the symmetricif(true, 'a')case. The path is correctly exercised: condition=false at index 1 triggers the skip branch,if_()receives[false, null](size 2) and rejects the two-argument call.
skipOperand logic is correct: the terminator check fires on the token before the depth decrement, so a closing ) at depth 1 is consumed (depth → 0) and the next token triggers the break — the stream is left in the right state for the caller in all chained operator cases. The nested short-circuit test case (false && (substring... || true) && true) exercises this path and passes.
No new issues found. Ready to merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
3 participants
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I tried the "Conditional logic" example from the
<condition>documentation inmaven.mdo:With a
java.versionthat has no-(the test context uses1.8.0_292, and21.0.12behaves the same), the condition fails withStringIndexOutOfBoundsException: Range [0, -1) out of bounds for length 9, and the profile is reported as an error instead of being evaluated.ConditionParsercomputes every operand while it parses, so both branches ofif(..)are always evaluated, even the one that is discarded.&&and||have the same problem:length(${p}) >= 3 && substring(${p}, 0, 3) == 'abc'throws whenpis shorter than three characters, because the guard can't stop the right-hand side from running.This change skips the branch that isn't selected in
if(..), and skips the right operand of&&or||once the left operand has already decided the result. Skipping walks the tokens up to the next terminator outside parentheses (,or), plus&&/||depending on the operator). It still rejects a missing operand or unbalanced parentheses, so the existingtestParenthesesMismatchcases keep failing the way they did before. For operands that do get evaluated, nothing changes:ifstill goes through the registered function, which checks the argument count, and a non-boolean on either side of&&or||still fails as before.I added
testIfFunctionOnlyEvaluatesSelectedBranch, which uses the documented example verbatim, andtestLogicalOperatorsShortCircuit. Without the change toConditionParser, both fail with the exceptions above. With it,ConditionParserTestandConditionProfileActivatorTestpass (76 tests), andmvn verifyonimpl/maven-implpasses: 690 tests, checkstyle and spotless included. I ran that with Maven 3.8.4 and-Denforcer.skip -Drat.skip, which were the only toolchain constraints on that machine. I did not run the core ITs.Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5.