From aed9a2f1f0ee7ed345234957cbc63fb26e45eb7d Mon Sep 17 00:00:00 2001 From: Krishna Kondaka Date: Wed, 4 Dec 2024 06:04:25 +0000 Subject: [PATCH] Addressed review comments Signed-off-by: Krishna Kondaka --- .../expression/ExpressionEvaluator.java | 2 +- .../expression/ExpressionEvaluatorTest.java | 33 ++++++- .../ParseTreeEvaluatorListener.java | 18 +++- ...ericExpressionEvaluator_ConditionalIT.java | 5 ++ .../ParseTreeEvaluatorListenerTest.java | 87 +++++++++++++++---- 5 files changed, 122 insertions(+), 23 deletions(-) diff --git a/data-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java b/data-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java index 113089636d..712a67948e 100644 --- a/data-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java +++ b/data-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java @@ -39,7 +39,7 @@ default Boolean evaluateConditional(final String statement, final Event context) } catch (ExpressionEvaluationException e) { return false; } - throw new ClassCastException("Unexpected expression return type of " + result.getClass()); + throw new ClassCastException("Unexpected expression return value of " + result); } Boolean isValidExpressionStatement(final String statement); diff --git a/data-prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java b/data-prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java index f4b266cb34..5b24f3a34c 100644 --- a/data-prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java +++ b/data-prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java @@ -19,7 +19,24 @@ public class ExpressionEvaluatorTest { private ExpressionEvaluator expressionEvaluator; class TestExpressionEvaluator implements ExpressionEvaluator { + private final boolean throwsExpressionEvaluationException; + private final boolean returnNull; + public TestExpressionEvaluator() { + throwsExpressionEvaluationException = false; + returnNull = false; + } + + public TestExpressionEvaluator(boolean throwsExpressionEvaluationException, boolean returnNull) { + this.throwsExpressionEvaluationException = throwsExpressionEvaluationException; + this.returnNull = returnNull; + } + public Object evaluate(final String statement, final Event event) { + if (throwsExpressionEvaluationException) { + throw new ExpressionEvaluationException("Expression Evaluation Exception", new RuntimeException("runtime exception")); + } else if (returnNull) { + return null; + } return event.get(statement, Object.class); } @@ -48,14 +65,24 @@ public List extractDynamicExpressionsFromFormatExpression(String format) public void testDefaultEvaluateConditional() { expressionEvaluator = new TestExpressionEvaluator(); assertThat(expressionEvaluator.evaluateConditional("/status", event("{\"status\":true}")), equalTo(true)); - } @Test public void testEvaluateReturningException() { expressionEvaluator = new TestExpressionEvaluator(); - assertThat(expressionEvaluator.evaluateConditional("/status > 300", event("{\"nostatus\":true}")), equalTo(false)); - + assertThat(expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")), equalTo(false)); + } + + @Test + public void testThrowExpressionEvaluationException() { + expressionEvaluator = new TestExpressionEvaluator(true, false); + assertThat(expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")), equalTo(false)); + } + + @Test + public void testExpressionEvaluationReturnsNull() { + expressionEvaluator = new TestExpressionEvaluator(false, true); + assertThat(expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")), equalTo(false)); } @Test diff --git a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java index f3c5286b09..509ad3c97c 100644 --- a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java +++ b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java @@ -112,6 +112,18 @@ public void visitErrorNode(ErrorNode node) { throw new RuntimeException("Hit error node in the parse tree: " + node.getText()); } + private boolean isBooleanOperator(Operator op) { + return (op instanceof GenericTypeOfOperator || + op instanceof GenericEqualOperator || + op instanceof NumericCompareOperator || + op instanceof OrOperator || + op instanceof AndOperator || + op instanceof GenericInSetOperator || + op instanceof NotOperator || + op instanceof GenericNotOperator || + op instanceof GenericRegexMatchOperator); + } + @Override public void exitEveryRule(ParserRuleContext ctx) { if (!operatorSymbolStack.isEmpty()) { @@ -123,8 +135,12 @@ public void exitEveryRule(ParserRuleContext ctx) { try { performSingleOperation(op, ctx); } catch (final Exception e) { - throw new ExpressionEvaluationException("Unable to evaluate the part of input statement: " + if (e instanceof IllegalArgumentException && isBooleanOperator(op)) { + operandStack.push(false); + } else { + throw new ExpressionEvaluationException("Unable to evaluate the part of input statement: " + getPartialStatementFromContext(ctx), e); + } } } } diff --git a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java b/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java index 86b3a51b47..cd1fac11b6 100644 --- a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java +++ b/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java @@ -151,6 +151,11 @@ private static Stream validExpressionArguments() { arguments("/status_code == 200", longEvent, true), arguments("/status_code != 300", event("{\"status_code\": 200}"), true), arguments("/status_code >= 300", event("{\"status_code_not_present\": 200}"), false), + arguments("/status_code != null and /status_code >= 300", event("{\"status_code_not_present\": 200}"), false), + arguments("not (/status_code >= 300)", event("{\"status_code_not_present\": 200}"), true), + arguments("(/status_code >= 300) or (/value == 10)", event("{\"status_code_not_present\": 200, \"value\" : 10}"), true), + arguments("(/status_code >= 300) and (/value == 15)", event("{\"status_code_not_present\": 200, \"value\" : 10}"), false), + arguments("(/value == 10) or ((/status_code >= 300) and (/novalue == 15))", event("{\"status_code_not_present\": 200, \"value\" : 10}"), true), arguments("/status_code == 200", event("{}"), false), arguments("/success == /status_code", event("{\"success\": true, \"status_code\": 200}"), false), arguments("/success != /status_code", event("{\"success\": true, \"status_code\": 200}"), true), diff --git a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java b/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java index f0e1fdb9f4..047286f74a 100644 --- a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java +++ b/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java @@ -11,6 +11,9 @@ import org.antlr.v4.runtime.tree.ParseTree; import org.antlr.v4.runtime.tree.ParseTreeWalker; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser; import org.apache.commons.lang3.RandomStringUtils; @@ -19,6 +22,7 @@ import java.util.List; import java.util.Map; import java.util.Random; +import java.util.stream.Stream; import java.util.function.Function; import static org.hamcrest.CoreMatchers.equalTo; @@ -46,6 +50,12 @@ class ParseTreeEvaluatorListenerTest { operatorConfiguration.greaterThanOperator(), operatorConfiguration.greaterThanOrEqualOperator(), operatorConfiguration.lessThanOperator(), operatorConfiguration.lessThanOrEqualOperator(), operatorConfiguration.regexEqualOperator(), operatorConfiguration.regexNotEqualOperator(), + operatorConfiguration.typeOfOperator(), + operatorConfiguration.concatOperator(), + operatorConfiguration.addOperator(), + operatorConfiguration.subtractOperator(), + operatorConfiguration.multiplyOperator(), + operatorConfiguration.divideOperator(), new NotOperator() ); private final OperatorProvider operatorProvider = new OperatorProvider(operators); @@ -184,10 +194,10 @@ void testSimpleRelationalOperatorExpressionWithInValidLiteralType() { final String lessThanStatement = "1 < true"; final String lessThanOrEqualStatement = "1 <= true"; final Event testEvent = createTestEvent(new HashMap<>()); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(greaterThanStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(greaterThanOrEqualStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(lessThanStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(lessThanOrEqualStatement, testEvent)); + assertThat(evaluateStatementOnEvent(greaterThanStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(greaterThanOrEqualStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(lessThanStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(lessThanOrEqualStatement, testEvent), is(false)); } @Test @@ -216,10 +226,10 @@ void testSimpleRelationalOperatorExpressionWithJsonPointerTypeInValidValueWithPo final String greaterThanOrEqualStatement = String.format(" /%s >= /%s", testKey, testKey); final String lessThanStatement = String.format(" /%s < %s", testKey, testValue); final String lessThanOrEqualStatement = String.format(" /%s <= /%s", testKey, testKey); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(greaterThanStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(greaterThanOrEqualStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(lessThanStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(lessThanOrEqualStatement, testEvent)); + assertThat(evaluateStatementOnEvent(greaterThanStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(greaterThanOrEqualStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(lessThanStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(lessThanOrEqualStatement, testEvent), is(false)); } @Test @@ -236,8 +246,49 @@ void testSimpleConditionalOperatorExpressionWithInValidLiteralType() { final String andStatement = "1 and false"; final String orStatement = "true or 0"; final Event testEvent = createTestEvent(new HashMap<>()); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(andStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(orStatement, testEvent)); + assertThat(evaluateStatementOnEvent(andStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(orStatement, testEvent), is(false)); + } + + @ParameterizedTest + @MethodSource("invalidExpressionArguments") + void testSimpleConditionalOperatorExpressionWithInValidExpression(final String statement) { + //final String statement = "((/field1 + 10) - (( 2 + /field2 ) + 5) - 10)"; + final Event testEvent = createTestEvent(new HashMap<>()); + assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(statement, testEvent)); + } + + private static Stream invalidExpressionArguments() { + return Stream.of( + Arguments.of("(/field1 + 10) + ( 2 + /field2 )"), + Arguments.of("(/field1 * 10) * ( 2 * /field2 )"), + Arguments.of("(/field1 / 10) + ( 2 / /field2 )"), + Arguments.of("(/field1 - 10) - ( 2 - /field2 )") + ); + } + + @ParameterizedTest + @MethodSource("booleanExpressionsGeneratingExceptions") + void testSimpleConditionalOperatorExpressionWithMissingField(final String statement, final boolean expectedResult) { + final String testKey = "testKey"; + final int testValue = 1; + final Map data = Map.of(testKey, testValue); + final Event testEvent = createTestEvent(data); + assertThat(evaluateStatementOnEvent(statement, testEvent), is(expectedResult)); + } + + private static Stream booleanExpressionsGeneratingExceptions() { + return Stream.of( + Arguments.of( "(/field1 > 1) or (/field2 > 2)", false), + Arguments.of( "(/field1 < 1) or (/field2 < 2)", false), + Arguments.of( "(/testKey < \"two\") or (2 == 2)", true), + Arguments.of( "(/testKey < \"two\") or (2)", false), + Arguments.of( "(/testKey) or (2 == 2)", false), + Arguments.of( "not (/testKey)", false), + Arguments.of( "(/testKey < \"two\") and (2)", false), + Arguments.of( "(/testKey) and (2 == 2)", false), + Arguments.of( "(/testKey < \"two\") and (2 == 2)", false) + ); } @Test @@ -260,8 +311,8 @@ void testSimpleConditionalOperatorExpressionWithJsonPointerTypeInValidValue() { final Event testEvent = createTestEvent(data); final String andStatement = String.format("/%s and false", testKey); final String orStatement = String.format("/%s or false", testKey); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(andStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(orStatement, testEvent)); + assertThat(evaluateStatementOnEvent(andStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(orStatement, testEvent), is(false)); } @Test @@ -275,7 +326,7 @@ void testSimpleNotOperatorExpressionWithValidLiteralType() { void testSimpleNotOperatorExpressionWithInValidLiteralType() { final String notStatement = "not 1"; final Event testEvent = createTestEvent(new HashMap<>()); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(notStatement, testEvent)); + assertThat(evaluateStatementOnEvent(notStatement, testEvent), is(false)); } @Test @@ -295,7 +346,7 @@ void testSimpleNotOperatorExpressionWithJsonPointerTypeInValidValue() { final Map data = Map.of(testKey, testValue); final String notStatement = String.format("not /%s", testKey); final Event testEvent = createTestEvent(data); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(notStatement, testEvent)); + assertThat(evaluateStatementOnEvent(notStatement, testEvent), is(false)); } @Test @@ -305,10 +356,10 @@ void testMultipleOperatorsExpressionNotPriorToRelational() { final String notPriorToGreaterThanOrEqualStatement = "not 1 >= 1"; final String notPriorToLessThanStatement = "not 2 < 1"; final String notPriorToLessThanOrEqualStatement = "not 1 <= 1"; - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(notPriorToGreaterThanStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(notPriorToGreaterThanOrEqualStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(notPriorToLessThanStatement, testEvent)); - assertThrows(ExpressionEvaluationException.class, () -> evaluateStatementOnEvent(notPriorToLessThanOrEqualStatement, testEvent)); + assertThat(evaluateStatementOnEvent(notPriorToGreaterThanStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(notPriorToGreaterThanOrEqualStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(notPriorToLessThanStatement, testEvent), is(false)); + assertThat(evaluateStatementOnEvent(notPriorToLessThanOrEqualStatement, testEvent), is(false)); } @Test