Addressed review comments

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
opensearch-project · Dec 12, 2024 · 90a14c1 · 90a14c1
1 parent 63de35a
commit 90a14c1
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 15 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
@@ -34,6 +34,8 @@ default Boolean evaluateConditional(final String statement, final Event context)
                 return (Boolean) result;
             }
             throw new ClassCastException("Unexpected expression return value of " + result);
+        } catch (ExpressionParsingException e) {
+            throw e;
         } catch (ExpressionEvaluationException e) {
             return false;
         }

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
@@ -9,7 +9,7 @@
  * @since 2.11
  * Wrapper exception for any exception thrown while evaluating a statement
  */
-public class ExpressionParsingException extends RuntimeException {
+public class ExpressionParsingException extends ExpressionEvaluationException {
     public ExpressionParsingException(final String message, final Throwable cause) {
         super(message, cause);
     }

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,18 +36,16 @@ public GenericExpressionEvaluator(final Parser<ParseTree> parser, final Evaluato
      */
     @Override
     public Object evaluate(final String statement, final Event context) {
-        boolean parsedSuccessfully = false;
+        ParseTree parseTree = null;
         try {
-            final ParseTree parseTree = parser.parse(statement);
-            parsedSuccessfully = true;
-            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) {
-            if (parsedSuccessfully) {
-                throw new ExpressionEvaluationException("Unable to evaluate statement \"" + statement + "\"", exception);
-            } else {
-                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);
         }
     }
 

diff --git a/...t/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ArithmeticIT.java b/...t/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ArithmeticIT.java
@@ -235,7 +235,6 @@ private static Stream<Arguments> exceptionExpressionSyntaxArguments() {
                 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\"}"))
         );
     }
 
@@ -252,9 +251,6 @@ private static Stream<Arguments> exceptionExpressionArguments() {
                 Arguments.of("/message * /status", event("{\"status\": 200, \"message\":\"msg\"}")),
                 Arguments.of("/message + /status", event("{\"status\": 200, \"message\":\"msg\"}")),
                 Arguments.of("/status - /message", 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\"}")),
                 Arguments.of("/message - /status", event("{\"status\": 200, \"message\":\"msg\"}")),
                 Arguments.of("-/message ", event("{\"status\": 200, \"message\":\"msg\"}"))
         );