Make expressions evaluating on keys with List or Map values not throw…

… an exception (#4142) Signed-off-by: Taylor Gray <[email protected]>
opensearch-project · Feb 19, 2024 · 4c0e944 · 4c0e944
1 parent b77815c
commit 4c0e944
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 30 deletions.
diff --git a/data-prepper-expression/build.gradle b/data-prepper-expression/build.gradle
@@ -42,7 +42,7 @@ jacocoTestCoverageVerification {
     violationRules {
         rule { //in addition to core projects rule - this one checks for 100% code coverage for this project
             limit {
-                minimum = 1.0 //keep this at 100%
+                minimum = 1.0 // keep at 100%
             }
         }
     }

diff --git a/data-prepper-expression/src/main/antlr/DataPrepperExpression.g4 b/data-prepper-expression/src/main/antlr/DataPrepperExpression.g4
@@ -175,7 +175,6 @@ literal
     | Null
     ;
 
-
 Integer
     : ZERO
     | NonZeroDigit Digit*

diff --git a/.../main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java b/.../main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java
@@ -9,6 +9,8 @@
 
 import javax.inject.Named;
 import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.function.Function;
 
@@ -22,6 +24,8 @@ public Map<Class<? extends Serializable>, Function<Object, Object>> literalTypeC
             Integer.class, Function.identity(),
             Float.class, Function.identity(),
             Long.class, Function.identity(),
+            ArrayList.class, Function.identity(),
+            LinkedHashMap.class, Function.identity(),
             Double.class, o -> ((Double) o).floatValue()
         );
     }

diff --git a/.../java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java b/.../java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java
@@ -210,7 +210,11 @@ private static Stream<Arguments> validExpressionArguments() {
                 Arguments.of("cidrContains(/sourceIp,\"192.0.2.0/24\",\"192.1.1.0/24\")", event("{\"sourceIp\": \"192.2.2.3\"}"), false),
                 Arguments.of("cidrContains(/sourceIp,\"2001:0db8::/32\")", event("{\"sourceIp\": \"2001:0db8:aaaa:bbbb::\"}"), true),
                 Arguments.of("cidrContains(/sourceIp,\"2001:0db8::/32\",\"2001:aaaa::/32\")", event("{\"sourceIp\": \"2001:0db8:aaaa:bbbb::\"}"), true),
-                Arguments.of("cidrContains(/sourceIp,\"2001:0db8::/32\",\"2001:aaaa::/32\")", event("{\"sourceIp\": \"2001:abcd:aaaa:bbbb::\"}"), false)
+                Arguments.of("cidrContains(/sourceIp,\"2001:0db8::/32\",\"2001:aaaa::/32\")", event("{\"sourceIp\": \"2001:abcd:aaaa:bbbb::\"}"), false),
+                Arguments.of("/sourceIp != null", event("{\"sourceIp\": [10, 20]}"), true),
+                Arguments.of("/sourceIp == null", event("{\"sourceIp\": [\"test\", \"test_two\"]}"), false),
+                Arguments.of("/sourceIp == null", event("{\"sourceIp\": {\"test\": \"test_two\"}}"), false),
+                Arguments.of("/sourceIp != null", event("{\"sourceIp\": {\"test\": \"test_two\"}}"), true)
         );
     }
 

diff --git a/...ion/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java b/...ion/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java
@@ -5,11 +5,11 @@
 
 package org.opensearch.dataprepper.expression;
 
-import org.opensearch.dataprepper.model.event.Event;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.antlr.v4.runtime.Token;
 import org.antlr.v4.runtime.tree.TerminalNode;
+import org.apache.commons.lang3.RandomStringUtils;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -19,12 +19,13 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;
 import org.opensearch.dataprepper.expression.util.TestObject;
-import org.apache.commons.lang3.RandomStringUtils;
+import org.opensearch.dataprepper.model.event.Event;
 
 import java.util.HashMap;
-import java.util.Map;
 import java.util.List;
+import java.util.Map;
 import java.util.Random;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Function;
 import java.util.stream.Stream;
 
@@ -35,8 +36,8 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -153,20 +154,6 @@ void testCoerceTerminalNodeJsonPointerTypeSupportedValues(final Object testValue
         }
     }
 
-    @ParameterizedTest
-    @MethodSource("provideUnSupportedJsonPointerValues")
-    void testCoerceTerminalNodeJsonPointerTypeUnSupportedValues(final Object testValue) {
-        final String testKey1 = "key1";
-        final String testKey2 = "key2";
-        final String testJsonPointerKey = String.format("/%s/%s", testKey1, testKey2);
-        final Event testEvent = testValue == null ? createTestEvent(new HashMap<>()) :
-                createTestEvent(Map.of(testKey1, Map.of(testKey2, testValue)));
-        when(token.getType()).thenReturn(DataPrepperExpressionParser.JsonPointer);
-        when(terminalNode.getSymbol()).thenReturn(token);
-        when(terminalNode.getText()).thenReturn(testJsonPointerKey);
-        assertThrows(ExpressionCoercionException.class, () -> objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent));
-    }
-
     @ParameterizedTest
     @MethodSource("provideKeys")
     void testCoerceTerminalNodeEscapeJsonPointerTypeWithSpecialCharacters(final String testKey, final String testEscapeJsonPointer)
@@ -200,19 +187,32 @@ void testCoerceTerminalNodeEscapeJsonPointerTypeSupportedValues(final Object tes
         }
     }
 
-    @ParameterizedTest
-    @MethodSource("provideUnSupportedJsonPointerValues")
-    void testCoerceTerminalNodeEscapeJsonPointerTypeUnSupportedValues(final Object testValue) {
+    @Test
+    void testCoerceTerminalNodeEscapeJsonPointerTypeUnSupportedValues() {
         final String testKey = "testKey";
+        final AtomicBoolean testValue = new AtomicBoolean();
         final String testEscapeJsonPointerKey = String.format("\"/%s\"", testKey);
-        final Event testEvent = testValue == null ? createTestEvent(new HashMap<>()) :
-                createTestEvent(Map.of(testKey, testValue));
+        final Event testEvent = createInvalidTestEvent(Map.of(testKey, testValue));
         when(token.getType()).thenReturn(DataPrepperExpressionParser.EscapedJsonPointer);
         when(terminalNode.getSymbol()).thenReturn(token);
         when(terminalNode.getText()).thenReturn(testEscapeJsonPointerKey);
         assertThrows(ExpressionCoercionException.class, () -> objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent));
     }
 
+    @Test
+    void testCoerceTerminalNodeJsonPointerTypeUnSupportedValues() {
+        final String testKey1 = "key1";
+        final String testKey2 = "key2";
+        final AtomicBoolean testValue = new AtomicBoolean();
+        final String testJsonPointerKey = String.format("/%s/%s", testKey1, testKey2);
+
+        final Event testEvent = createInvalidTestEvent(Map.of(testKey1, testValue));
+        when(token.getType()).thenReturn(DataPrepperExpressionParser.JsonPointer);
+        when(terminalNode.getSymbol()).thenReturn(token);
+        when(terminalNode.getText()).thenReturn(testJsonPointerKey);
+        assertThrows(ExpressionCoercionException.class, () -> objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent));
+    }
+
     @Test
     void testCoerceTerminalNodeUnsupportedType() {
         final Event testEvent = createTestEvent(new HashMap<>());
@@ -333,6 +333,12 @@ private Event createTestEvent(final Object data) {
         return event;
     }
 
+    private Event createInvalidTestEvent(final Object data) {
+        final Event event = mock(Event.class);
+        lenient().when(event.get(anyString(), any())).thenReturn(new AtomicBoolean());
+        return event;
+    }
+
     private static Stream<Arguments> provideKeys() {
         return Stream.of(
                 Arguments.of("test key", "\"/test key\""),
@@ -350,11 +356,9 @@ private static Stream<Arguments> provideSupportedJsonPointerValues() {
                 Arguments.of("test value"),
                 Arguments.of(1.1f),
                 Arguments.of(1.1),
+                Arguments.of(List.of("test_value")),
+                Arguments.of(Map.of("test_key", "test_value")),
                 Arguments.of((Object) null)
         );
     }
-
-    private static Stream<Arguments> provideUnSupportedJsonPointerValues() {
-        return Stream.of(Arguments.of(new HashMap<>()));
-    }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -175,7 +175,6 @@ literal @@
         | Null
         ;
     Integer
         : ZERO
         | NonZeroDigit Digit*
@@ Expand Down @@