Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null pointer exception during exception evaluation #5217

Merged
merged 4 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR raises the idea of other exceptions, this is a good candidate for a better exception.

Declare:

public class UnexpectedEvaluatedReturnTypeException extends ExpressionEvaluationException {

And then throw:

throw new UnexpectedEvaluatedReturnTypeException("Unexpected expression return value of " + result);

We don't have to do for this PR, but it would be a good improvement.

} catch (ExpressionParsingException e) {
throw e;
} catch (ExpressionEvaluationException e) {
return false;
}
}

Expand All @@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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));
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ default int getNumberOfOperands(final RuleContext ctx) {
return 2;
}

boolean isBooleanOperator();

boolean shouldEvaluate(final RuleContext ctx);

int getSymbol();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should log the stack trace when catching Exception.

Please add new catch statements for known exceptions (e.g. parse error) and log those without the stack trace.

throw new ExpressionEvaluationException(e.getMessage(), e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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));
Expand Down
Loading
Loading