Fix null pointer exception during exception evaluation #5217

kkondaka · 2024-11-28T01:17:24Z

Description

Fix null pointer exception during exception evaluation.
Modified to check for null objects before evaluating expressions.
Modified to catch exception in conditional expression evaluation and return false instead of throwing the exception

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

[ X] New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
[X ] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable · 2024-12-02T18:52:15Z

...rc/main/java/org/opensearch/dataprepper/plugins/processor/mutateevent/AddEntryProcessor.java

@@ -102,7 +102,6 @@ public Collection<Record<Event>> doExecute(final Collection<Record<Event>> recor
                                .addArgument(entry.getValueExpression())
                                .addArgument(entry.getFormat())
                                .addArgument(entry.getValue())
-                                .setCause(e)


We should include the cause whenever we catch Exception. Please add a new catch for the known exception(s) when these errors arise.

e.g.

catch (InvalidExpressionException e) { // log without exception } catch (Exception e) { // log with the exception }

dlvenable · 2024-12-02T18:54:02Z

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

+        try {
+            result = evaluate(statement, context);
+            if (result == null) {
+                return false;


I'm not sure that this is correct. This indicates a bug with our exception handling because any boolean expression should return non-null. Maybe we need to handle this by throwing a more specific exception. Or perhaps we log this case.

I think we cover all the cases in the code below. I think we should still consider this an error. If we hit this point we are experiencing indeterminate behavior which is probably best considered as an error state.

dlvenable · 2024-12-02T18:55:09Z

...epper-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());


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.

dlvenable · 2024-12-02T18:55:56Z

...est/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java

@@ -119,7 +118,7 @@ void testGenericExpressionEvaluatorWithMultipleThreads(final String expression,
    void testGenericExpressionEvaluatorThrows(final String expression, final Event event) {
        final GenericExpressionEvaluator evaluator = applicationContext.getBean(GenericExpressionEvaluator.class);

-        assertThrows(RuntimeException.class, () -> evaluator.evaluateConditional(expression, event));
+        assertThat(evaluator.evaluateConditional(expression, event), equalTo(false));


This behavior does not seem correct.

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable · 2024-12-04T23:12:21Z

...pression/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) {


I don't think this will scale. We should add this as a method in Operator that needs to be implemented. This way, each implementation is sure to have some value for it.

dlvenable · 2024-12-04T23:15:23Z

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

+        try {
+            result = evaluate(statement, context);
+            if (result == null) {
+                return false;


I think we cover all the cases in the code below. I think we should still consider this an error. If we hit this point we are experiencing indeterminate behavior which is probably best considered as an error state.

…parsing exceptions Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable · 2024-12-12T16:49:46Z

...test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ArithmeticIT.java

+                Arguments.of("/status - ", event("{\"status\": 200, \"message\":\"msg\"}")),
+                Arguments.of("/status / ", event("{\"status\": 200, \"message\":\"msg\"}")),
+                Arguments.of(" * /status ", event("{\"status\": 200, \"message\":\"msg\"}"))
+                //Arguments.of("-/message ", event("{\"status\": 200, \"message\":\"msg\"}"))


Let's remove this commented out code.

dlvenable · 2024-12-12T16:49:51Z

...test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ArithmeticIT.java

-                Arguments.of("/status - ", event("{\"status\": 200, \"message\":\"msg\"}")),
-                Arguments.of("/status / ", event("{\"status\": 200, \"message\":\"msg\"}")),
-                Arguments.of(" * /status ", event("{\"status\": 200, \"message\":\"msg\"}")),
+                //Arguments.of("/status - ", event("{\"status\": 200, \"message\":\"msg\"}")),


Let's remove this commented out code.

dlvenable · 2024-12-12T16:52:53Z

...pper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionParsingException.java

+ * @since 2.11
+ * Wrapper exception for any exception thrown while evaluating a statement
+ */
+public class ExpressionParsingException extends RuntimeException {


I like having this specific exception. But, I think we should have a common parent, that parent should probably remain ExpressionEvaluationException.

Suggested change

public class ExpressionParsingException extends RuntimeException {

public class ExpressionParsingException extends ExpressionEvaluationException {

We could possibly add other sub-classes to ExpressionEvaluationException for other cases in the future as well.

dlvenable · 2024-12-12T16:55:09Z

...pression/src/main/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator.java

            return evaluator.evaluate(parseTree, context);
        }
        catch (final Exception exception) {
-            throw new ExpressionEvaluationException("Unable to evaluate statement \"" + statement + "\"", exception);
+            if (parsedSuccessfully) {


You can avoid the boolean. This should also be easier to maintain as we clearly distinguish the steps.

final ParseTree parseTree = parser.parse(statement); try { parseTree = parser.parse(statement); } catch (final Exception exception) { throw new ExpressionParsingException("Unable to parse statement \"" + statement + "\"", exception); } try { return evaluator.evaluate(parseTree, context); } catch (final Exception exception) { throw new ExpressionEvaluationException("Unable to evaluate statement \"" + statement + "\"", exception); }

Yeah, I thought about this but felt one try catch may be better. Anyways, changed to this.

dlvenable · 2024-12-12T17:00:22Z

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

            if (result instanceof Boolean) {
                return (Boolean) result;
            }
+            throw new ClassCastException("Unexpected expression return value of " + result);


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.

Signed-off-by: Krishna Kondaka <[email protected]>

…oject#5217) Fix null pointer exception during exception evaluation Signed-off-by: Krishna Kondaka <[email protected]>

Fix null pointer exception during exception evaluation

1ef89f3

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable requested changes Dec 2, 2024

View reviewed changes

Addressed review comments

aed9a2f

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable requested changes Dec 4, 2024

View reviewed changes

Addressed review comments. Introduced ExpressionParsingException for …

63de35a

…parsing exceptions Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable requested changes Dec 12, 2024

View reviewed changes

Addressed review comments

90a14c1

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable approved these changes Dec 12, 2024

View reviewed changes

dlvenable marked this pull request as ready for review December 12, 2024 22:25

dlvenable requested review from sb2k16, chenqi0805, engechas, graytaylor0, dinujoh, KarstenSchnitter and oeyh as code owners December 12, 2024 22:25

graytaylor0 approved these changes Dec 13, 2024

View reviewed changes

dlvenable merged commit 3a684f0 into opensearch-project:main Dec 13, 2024
43 of 47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix null pointer exception during exception evaluation #5217

Fix null pointer exception during exception evaluation #5217

kkondaka commented Nov 28, 2024

dlvenable Dec 2, 2024

dlvenable Dec 2, 2024

dlvenable Dec 4, 2024

dlvenable Dec 2, 2024

dlvenable Dec 2, 2024

dlvenable Dec 4, 2024

dlvenable Dec 4, 2024

dlvenable Dec 12, 2024

dlvenable Dec 12, 2024

dlvenable Dec 12, 2024

dlvenable Dec 12, 2024

kkondaka Dec 12, 2024

dlvenable Dec 12, 2024

	public class ExpressionParsingException extends RuntimeException {
	public class ExpressionParsingException extends ExpressionEvaluationException {

Fix null pointer exception during exception evaluation #5217

Fix null pointer exception during exception evaluation #5217

Conversation

kkondaka commented Nov 28, 2024

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment