Fix null pointer exception during exception evaluation (opensearch-pr…

…oject#5217) Fix null pointer exception during exception evaluation Signed-off-by: Krishna Kondaka <[email protected]>
sb2k16 · Dec 13, 2024 · 320a3d3 · 320a3d3
1 parent 047bdeb
commit 320a3d3
Show file tree

Hide file tree

Showing 27 changed files with 337 additions and 50 deletions.
diff --git a/...-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java b/...-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java
@@ -27,11 +27,17 @@ public interface ExpressionEvaluator {
     Object evaluate(final String statement, final Event context);
 
     default Boolean evaluateConditional(final String statement, final Event context) {
-        final Object result = evaluate(statement, context);
-        if (result instanceof Boolean) {
-            return (Boolean) result;
-        } else {
-            throw new ClassCastException("Unexpected expression return type of " + result.getClass());
+        Object result;
+        try {
+            result = evaluate(statement, context);
+            if (result instanceof Boolean) {
+                return (Boolean) result;
+            }
+            throw new ClassCastException("Unexpected expression return value of " + result);
+        } catch (ExpressionParsingException e) {
+            throw e;
+        } catch (ExpressionEvaluationException e) {
+            return false;
         }
     }
 
@@ -42,4 +48,4 @@ default Boolean evaluateConditional(final String statement, final Event context)
     List<String> extractDynamicKeysFromFormatExpression(final String format);
 
     List<String> extractDynamicExpressionsFromFormatExpression(final String format);
-}
+}
diff --git a/...r-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionParsingException.java b/...r-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionParsingException.java
@@ -0,0 +1,17 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.dataprepper.expression;
+
+/**
+ * @since 2.11
+ * Wrapper exception for any exception thrown while evaluating a statement
+ */
+public class ExpressionParsingException extends ExpressionEvaluationException {
+    public ExpressionParsingException(final String message, final Throwable cause) {
+        super(message, cause);
+    }
+}
+
diff --git a/...pper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java b/...pper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java
@@ -19,7 +19,29 @@
 public class ExpressionEvaluatorTest {
     private ExpressionEvaluator expressionEvaluator;
     class TestExpressionEvaluator implements ExpressionEvaluator {
+        private final boolean throwsExpressionEvaluationException;
+        private final boolean throwsExpressionParsingException;
+        private final boolean returnNull;
+        public TestExpressionEvaluator() {
+            throwsExpressionEvaluationException = false;
+            throwsExpressionParsingException = false;
+            returnNull = false;
+        }
+
+        public TestExpressionEvaluator(boolean throwsExpressionEvaluationException, boolean throwsExpressionParsingException, boolean returnNull) {
+            this.throwsExpressionEvaluationException = throwsExpressionEvaluationException;
+            this.throwsExpressionParsingException = throwsExpressionParsingException;
+            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 (throwsExpressionParsingException) {
+                throw new ExpressionParsingException("Expression Parsing Exception", new RuntimeException("runtime exception"));
+            } else if (returnNull) {
+                return null;
+            }
             return event.get(statement, Object.class);
         }
 
@@ -48,7 +70,30 @@ 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();
+        assertThrows(ClassCastException.class, () -> expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")));
+    }
+
+    @Test
+    public void testThrowExpressionEvaluationException() {
+        expressionEvaluator = new TestExpressionEvaluator(true, false, false);
+        assertThat(expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")), equalTo(false));
+    }
+
+    @Test
+    public void testThrowExpressionParsingException() {
+        expressionEvaluator = new TestExpressionEvaluator(false, true, false);
+        assertThrows(ExpressionParsingException.class, () -> expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")));
+    }
+
+    @Test
+    public void testExpressionEvaluationReturnsNull() {
+        expressionEvaluator = new TestExpressionEvaluator(false, false, true);
+        assertThrows(ClassCastException.class, () -> expressionEvaluator.evaluateConditional("/status", event("{\"nostatus\":true}")));
     }
 
     @Test

diff --git a/...i/src/test/java/org/opensearch/dataprepper/expression/ExpressionParsingExceptionTest.java b/...i/src/test/java/org/opensearch/dataprepper/expression/ExpressionParsingExceptionTest.java
@@ -0,0 +1,25 @@
+/*
+ *
+ *  * Copyright OpenSearch Contributors
+ *  * SPDX-License-Identifier: Apache-2.0
+ *
+ */
+
+package org.opensearch.dataprepper.expression;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.UUID;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+class ExpressionParsingExceptionTest {
+
+    @Test
+    void testExpressionParsingException() {
+        final ExpressionParsingException exception = new ExpressionParsingException(UUID.randomUUID().toString(), null);
+        assertThat(exception instanceof RuntimeException, is(true));
+    }
+}
+
diff --git a/...per-expression/src/main/java/org/opensearch/dataprepper/expression/AddBinaryOperator.java b/...per-expression/src/main/java/org/opensearch/dataprepper/expression/AddBinaryOperator.java
@@ -18,6 +18,12 @@ class AddBinaryOperator implements Operator<Object> {
     private final String displayName;
     private final Map<Class<? extends Number>, Map<Class<? extends Number>, BiFunction<Object, Object, Number>>> operandsToOperationMap;
 
+
+    @Override
+    public boolean isBooleanOperator() {
+        return false;
+    }
+
     public AddBinaryOperator(final int symbol,
             final Map<Class<? extends Number>, Map<Class<? extends Number>, BiFunction<Object, Object, Number>>> operandsToOperationMap) {
         this.symbol = symbol;
@@ -41,6 +47,7 @@ public Object evaluate(final Object ... args) {
         checkArgument(args.length == 2, displayName + " requires operands length needs to be 2.");
         final Object leftValue = args[0];
         final Object rightValue = args[1];
+        checkArgument(leftValue != null && rightValue != null, displayName + " requires operands length needs to be non-null.");
         final Class<?> leftValueClass = leftValue.getClass();
         final Class<?> rightValueClass = rightValue.getClass();
         if (leftValue instanceof String && rightValue instanceof String) {

diff --git a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/AndOperator.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/AndOperator.java
@@ -18,6 +18,11 @@ class AndOperator implements Operator<Boolean> {
     private static final String DISPLAY_NAME = DataPrepperExpressionParser.VOCABULARY
             .getDisplayName(DataPrepperExpressionParser.AND);
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public boolean shouldEvaluate(final RuleContext ctx) {
         return ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_conditionalExpression;

diff --git a/...ression/src/main/java/org/opensearch/dataprepper/expression/ArithmeticBinaryOperator.java b/...ression/src/main/java/org/opensearch/dataprepper/expression/ArithmeticBinaryOperator.java
@@ -18,6 +18,11 @@ class ArithmeticBinaryOperator implements Operator<Number> {
     private final String displayName;
     private final Map<Class<? extends Number>, Map<Class<? extends Number>, BiFunction<Object, Object, Number>>> operandsToOperationMap;
 
+    @Override
+    public boolean isBooleanOperator() {
+        return false;
+    }
+
     public ArithmeticBinaryOperator(final int symbol,
             final Map<Class<? extends Number>, Map<Class<? extends Number>, BiFunction<Object, Object, Number>>> operandsToOperationMap) {
         this.symbol = symbol;
@@ -40,6 +45,7 @@ public Number evaluate(final Object ... args) {
         checkArgument(args.length == 2, displayName + " requires operands length needs to be 2.");
         final Object leftValue = args[0];
         final Object rightValue = args[1];
+        checkArgument(leftValue != null && rightValue != null, displayName + " requires operands length needs to be non-null.");
         final Class<?> leftValueClass = leftValue.getClass();
         final Class<?> rightValueClass = rightValue.getClass();
         if (!operandsToOperationMap.containsKey(leftValueClass)) {

diff --git a/...ssion/src/main/java/org/opensearch/dataprepper/expression/ArithmeticSubtractOperator.java b/...ssion/src/main/java/org/opensearch/dataprepper/expression/ArithmeticSubtractOperator.java
@@ -29,6 +29,11 @@ public ArithmeticSubtractOperator(final int symbol,
         this.strategy = strategy;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return false;
+    }
+
     @Override
     public int getNumberOfOperands(final RuleContext ctx) {
         if (ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_arithmeticExpression)
@@ -59,6 +64,7 @@ public Number evaluate(final Object ... args) {
         checkArgument(args.length == 2, displayName + " requires operands length needs to be 2.");
         final Object leftValue = args[0];
         final Object rightValue = args[1];
+        checkArgument(leftValue != null && rightValue != null, displayName + " requires operands length needs to be non-null.");
         final Class<?> leftValueClass = leftValue.getClass();
         final Class<?> rightValueClass = rightValue.getClass();
         if (!operandsToOperationMap.containsKey(leftValueClass)) {

diff --git a/...-expression/src/main/java/org/opensearch/dataprepper/expression/GenericEqualOperator.java b/...-expression/src/main/java/org/opensearch/dataprepper/expression/GenericEqualOperator.java
@@ -20,6 +20,11 @@ public GenericEqualOperator(
         this.equalStrategy = equalStrategy;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     protected Boolean checkedEvaluate(final Object lhs, final Object rhs) {
         if (lhs == null || rhs == null) {

diff --git a/...ssion/src/main/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator.java b/...ssion/src/main/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator.java
@@ -36,11 +36,15 @@ public GenericExpressionEvaluator(final Parser<ParseTree> parser, final Evaluato
      */
     @Override
     public Object evaluate(final String statement, final Event context) {
+        ParseTree parseTree = null;
         try {
-            final ParseTree parseTree = parser.parse(statement);
-            return evaluator.evaluate(parseTree, context);
+            parseTree = parser.parse(statement);
+        } catch (final Exception exception) {
+            throw new ExpressionParsingException("Unable to parse statement \"" + statement + "\"", exception);
         }
-        catch (final Exception exception) {
+        try {
+            return evaluator.evaluate(parseTree, context);
+        } catch (final Exception exception) {
             throw new ExpressionEvaluationException("Unable to evaluate statement \"" + statement + "\"", exception);
         }
     }

diff --git a/...-expression/src/main/java/org/opensearch/dataprepper/expression/GenericInSetOperator.java b/...-expression/src/main/java/org/opensearch/dataprepper/expression/GenericInSetOperator.java
@@ -24,6 +24,11 @@ public GenericInSetOperator(final int symbol, BiPredicate<Object, Object> operat
         this.operation = operation;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public boolean shouldEvaluate(final RuleContext ctx) {
         return ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_setOperatorExpression;

diff --git a/...er-expression/src/main/java/org/opensearch/dataprepper/expression/GenericNotOperator.java b/...er-expression/src/main/java/org/opensearch/dataprepper/expression/GenericNotOperator.java
@@ -16,6 +16,11 @@ public GenericNotOperator(final int symbol, final int shouldEvaluateRuleIndex, f
         this.innerOperator = innerOperator;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public int getNumberOfOperands(final RuleContext ctx) {
         return innerOperator.getNumberOfOperands(ctx);

diff --git a/...ession/src/main/java/org/opensearch/dataprepper/expression/GenericRegexMatchOperator.java b/...ession/src/main/java/org/opensearch/dataprepper/expression/GenericRegexMatchOperator.java
@@ -24,6 +24,11 @@ public GenericRegexMatchOperator(final int symbol, BiPredicate<Object, Object> o
         this.operation = operation;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public boolean shouldEvaluate(final RuleContext ctx) {
         return ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_regexOperatorExpression;

diff --git a/...expression/src/main/java/org/opensearch/dataprepper/expression/GenericTypeOfOperator.java b/...expression/src/main/java/org/opensearch/dataprepper/expression/GenericTypeOfOperator.java
@@ -23,6 +23,11 @@ public GenericTypeOfOperator(final int symbol, BiPredicate<Object, Object> opera
         this.operation = operation;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public boolean shouldEvaluate(final RuleContext ctx) {
         return ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_typeOfOperatorExpression;

diff --git a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/NotOperator.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/NotOperator.java
@@ -18,6 +18,11 @@ class NotOperator implements Operator<Boolean> {
     private static final String DISPLAY_NAME = DataPrepperExpressionParser.VOCABULARY
             .getDisplayName(DataPrepperExpressionParser.NOT);
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public int getNumberOfOperands(final RuleContext ctx) {
         return 1;

diff --git a/...xpression/src/main/java/org/opensearch/dataprepper/expression/NumericCompareOperator.java b/...xpression/src/main/java/org/opensearch/dataprepper/expression/NumericCompareOperator.java
@@ -25,6 +25,11 @@ public NumericCompareOperator(final int symbol,
         this.operandsToOperationMap = operandsToOperationMap;
     }
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public boolean shouldEvaluate(final RuleContext ctx) {
         return ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_relationalOperatorExpression;
@@ -40,6 +45,7 @@ public Boolean evaluate(final Object ... args) {
         checkArgument(args.length == 2, displayName + " requires operands length needs to be 2.");
         final Object leftValue = args[0];
         final Object rightValue = args[1];
+        checkArgument(leftValue != null && rightValue != null, displayName + " requires operands length needs to be non-null.");
         final Class<?> leftValueClass = leftValue.getClass();
         final Class<?> rightValueClass = rightValue.getClass();
         if (!operandsToOperationMap.containsKey(leftValueClass)) {

diff --git a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/Operator.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/Operator.java
@@ -12,6 +12,8 @@ default int getNumberOfOperands(final RuleContext ctx) {
         return 2;
     }
 
+    boolean isBooleanOperator();
+
     boolean shouldEvaluate(final RuleContext ctx);
 
     int getSymbol();

diff --git a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/OrOperator.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/OrOperator.java
@@ -18,6 +18,11 @@ class OrOperator implements Operator<Boolean> {
     private static final String DISPLAY_NAME = DataPrepperExpressionParser.VOCABULARY
             .getDisplayName(DataPrepperExpressionParser.OR);
 
+    @Override
+    public boolean isBooleanOperator() {
+        return true;
+    }
+
     @Override
     public boolean shouldEvaluate(final RuleContext ctx) {
         return ctx.getRuleIndex() == DataPrepperExpressionParser.RULE_conditionalExpression;

diff --git a/...er-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluator.java b/...er-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluator.java
@@ -37,7 +37,7 @@ public Object evaluate(ParseTree parseTree, Event event) {
             walker.walk(listener, parseTree);
             return listener.getResult();
         } catch (final Exception e) {
-            LOG.error("Unable to evaluate event", e);
+            LOG.error(e.getMessage());
             throw new ExpressionEvaluationException(e.getMessage(), e);
         }
     }

diff --git a/...ssion/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java b/...ssion/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListener.java
@@ -123,8 +123,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 && op.isBooleanOperator()) {
+                            operandStack.push(false);
+                        } else {
+                            throw new ExpressionEvaluationException("Unable to evaluate the part of input statement: "
                                 + getPartialStatementFromContext(ctx), e);
+                        }
                     }
                 }
             }

diff --git a/...n/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluatorTest.java b/...n/src/test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluatorTest.java
@@ -85,7 +85,7 @@ void testGivenParserThrowsExceptionThenExceptionThrown() {
 
         doThrow(new RuntimeException()).when(parser).parse(eq(statement));
 
-        assertThrows(ExpressionEvaluationException.class, () -> statementEvaluator.evaluate(statement, null));
+        assertThrows(ExpressionParsingException.class, () -> statementEvaluator.evaluate(statement, null));
 
         verify(parser).parse(eq(statement));
         verify(evaluator, times(0)).evaluate(any(), any());
@@ -100,7 +100,7 @@ void testGivenEvaluatorThrowsExceptionThenExceptionThrown() {
         doReturn(parseTree).when(parser).parse(eq(statement));
         doThrow(new RuntimeException()).when(evaluator).evaluate(eq(parseTree), eq(event));
 
-        assertThrows(ExpressionEvaluationException.class, () -> statementEvaluator.evaluateConditional(statement, event));
+        assertThat(statementEvaluator.evaluateConditional(statement, event), equalTo(false));
 
         verify(parser).parse(eq(statement));
         verify(evaluator).evaluate(eq(parseTree), eq(event));