Skip to content

Commit d47fb03

Browse files
authored
Fixes a bug with expressions when functions are combined with and/or operations. (#6669)
The function composition change (#6628) converted function from a lexer rule to a parser rule, which meant function arguments became full parser sub-expressions (including conditionalExpression). The ParseTreeEvaluatorListener uses LPAREN/RPAREN tokens on the operatorSymbolStack as barriers to prevent operators from being evaluated inside nested scopes - this is how parenthesesExpression works correctly. However, the function composition code explicitly skipped LPAREN/RPAREN tokens inside functions, removing this barrier. When a multi-argument function appeared as the right operand of and/or, the AND operator sitting on the stack would fire prematurely when the walker exited the inner conditionalExpression of a function argument, consuming the function's arguments as boolean operands, silently pushing false, and leaving the function with only one argument instead of two. The fix simply stops skipping LPAREN/RPAREN inside functions, letting them flow through to the normal handling where they act as operator stack barriers. Signed-off-by: David Venable <dlv@amazon.com>
1 parent e2bb830 commit d47fb03

3 files changed

Lines changed: 60 additions & 5 deletions

File tree

data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,11 @@ public void visitTerminal(TerminalNode node) {
140140
}
141141

142142
if (isInsideFunction()) {
143-
// Inside a function context, LPAREN/RPAREN/COMMA are structural - skip them
144-
if (nodeType == DataPrepperExpressionParser.LPAREN ||
145-
nodeType == DataPrepperExpressionParser.RPAREN ||
146-
nodeType == DataPrepperExpressionParser.COMMA) {
143+
// Inside a function context, COMMA is structural - skip it.
144+
// LPAREN/RPAREN are NOT skipped so they act as a barrier on the
145+
// operator stack, preventing outer operators from being evaluated
146+
// inside function argument sub-expressions.
147+
if (nodeType == DataPrepperExpressionParser.COMMA) {
147148
return;
148149
}
149150

data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,8 @@ private static Stream<Arguments> validExpressionArguments() {
255255
arguments("substringAfter(/path, \"/\") == \"app/src/main.py\"", event("{\"path\": \"/app/src/main.py\"}"), true),
256256
arguments("substringBefore(\"key=a=b\", \"=\") == \"key\"", event("{}"), true),
257257
arguments("substringAfterLast(\"/app/src/main.py\", \"/\") == \"main.py\"", event("{}"), true),
258-
arguments("substringBeforeLast(\"app.src.main\", \".\") == \"app.src\"", event("{}"), true)
258+
arguments("substringBeforeLast(\"app.src.main\", \".\") == \"app.src\"", event("{}"), true),
259+
arguments("/value == \"value-a\" and contains(/string, \"x/y/\")", event("{\"value\": \"value-a\", \"string\": \"prefix/x/y/postfix\"}"), true)
259260
);
260261
}
261262

data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,4 +529,57 @@ void testFunctionResultUsedInArithmeticExpression() {
529529
final String statement = "length(/field) + 3";
530530
assertThat(evaluateStatementOnEvent(statement, testEvent), equalTo(8));
531531
}
532+
533+
@Test
534+
void testFunctionWithMultipleArgsAsRightOperandOfAnd() {
535+
final Event testEvent = createTestEvent(Map.of("value", "value-a", "message", "hello world"));
536+
when(expressionFunctionProvider.provideFunction(eq("contains"), argThat(args ->
537+
args.size() == 2 && args.get(0) instanceof EventKey && args.get(1) instanceof String
538+
), any(Event.class), any(Function.class))).thenReturn(true);
539+
final String statement = "/value == \"value-a\" and contains(/message, \"hello\")";
540+
assertThat(evaluateStatementOnEvent(statement, testEvent), is(true));
541+
}
542+
543+
@Test
544+
void testFunctionWithMultipleArgsAsRightOperandOfOr() {
545+
final Event testEvent = createTestEvent(Map.of("value", "value-b", "message", "hello world"));
546+
when(expressionFunctionProvider.provideFunction(eq("contains"), argThat(args ->
547+
args.size() == 2 && args.get(0) instanceof EventKey && args.get(1) instanceof String
548+
), any(Event.class), any(Function.class))).thenReturn(true);
549+
final String statement = "/value == \"value-a\" or contains(/message, \"hello\")";
550+
assertThat(evaluateStatementOnEvent(statement, testEvent), is(true));
551+
}
552+
553+
@Test
554+
void testFunctionWithMultipleArgsAsLeftOperandOfAnd() {
555+
final Event testEvent = createTestEvent(Map.of("value", "value-a", "message", "hello world"));
556+
when(expressionFunctionProvider.provideFunction(eq("contains"), argThat(args ->
557+
args.size() == 2 && args.get(0) instanceof EventKey && args.get(1) instanceof String
558+
), any(Event.class), any(Function.class))).thenReturn(true);
559+
final String statement = "contains(/message, \"hello\") and /value == \"value-a\"";
560+
assertThat(evaluateStatementOnEvent(statement, testEvent), is(true));
561+
}
562+
563+
@Test
564+
void testFunctionWithMultipleArgsAndStringContainingSlashes() {
565+
final Event testEvent = createTestEvent(Map.of("value", "value-a", "path", "prefix/x/y/postfix"));
566+
when(expressionFunctionProvider.provideFunction(eq("contains"), argThat(args ->
567+
args.size() == 2 && args.get(0) instanceof EventKey && "x/y/".equals(args.get(1))
568+
), any(Event.class), any(Function.class))).thenReturn(true);
569+
final String statement = "/value == \"value-a\" and contains(/path, \"x/y/\")";
570+
assertThat(evaluateStatementOnEvent(statement, testEvent), is(true));
571+
}
572+
573+
@Test
574+
void testFunctionOnBothSidesOfAnd() {
575+
final Event testEvent = createTestEvent(Map.of("msg", "hello", "path", "/a/b"));
576+
when(expressionFunctionProvider.provideFunction(eq("contains"), argThat(args ->
577+
args.size() == 2 && args.get(0) instanceof EventKey && "hello".equals(args.get(1))
578+
), any(Event.class), any(Function.class))).thenReturn(true);
579+
when(expressionFunctionProvider.provideFunction(eq("startsWith"), argThat(args ->
580+
args.size() == 2 && args.get(0) instanceof EventKey && "/a".equals(args.get(1))
581+
), any(Event.class), any(Function.class))).thenReturn(true);
582+
final String statement = "contains(/msg, \"hello\") and startsWith(/path, \"/a\")";
583+
assertThat(evaluateStatementOnEvent(statement, testEvent), is(true));
584+
}
532585
}

0 commit comments

Comments
 (0)