Skip to content

Commit

Permalink
Addressed review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Krishna Kondaka <[email protected]>
  • Loading branch information
@kkondaka
kkondaka committed Dec 4, 2024
1 parent 1ef89f3 commit aed9a2f
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 23 deletions.
2 changes: 1 addition & 1 deletion ...-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
33 changes: 30 additions & 3 deletions ...pper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -48,14 +65,24 @@ public List<String> 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
Expand Down
18 changes: 17 additions & 1 deletion ...ssion/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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);
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions .../java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ private static Stream<Arguments> 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),
Expand Down
87 changes: 69 additions & 18 deletions ...n/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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<Arguments> 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<String, Object> data = Map.of(testKey, testValue);
final Event testEvent = createTestEvent(data);
assertThat(evaluateStatementOnEvent(statement, testEvent), is(expectedResult));
}

private static Stream<Arguments> 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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -295,7 +346,7 @@ void testSimpleNotOperatorExpressionWithJsonPointerTypeInValidValue() {
final Map<String, Integer> 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
Expand All @@ -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
Expand Down

0 comments on commit aed9a2f

Please sign in to comment.