FIX: coercion on json pointer values (#1178)

* FIX: coercion on jsonpointer values Signed-off-by: Chen <[email protected]> * MAINT: compactify type checking Signed-off-by: Chen <[email protected]> * STY: unused import Signed-off-by: Chen <[email protected]> * ENH: use bean injection instead of static variable Signed-off-by: Chen <[email protected]> * STY: unused import Signed-off-by: Chen <[email protected]>
opensearch-project · Mar 11, 2022 · 02a9050 · 02a9050
1 parent 5ebe995
commit 02a9050
Show file tree

Hide file tree

Showing 4 changed files with 128 additions and 31 deletions.
diff --git a/.../main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java b/.../main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java
@@ -0,0 +1,28 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.dataprepper.expression;
+
+import org.springframework.context.annotation.Bean;
+
+import javax.inject.Named;
+import java.io.Serializable;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Function;
+
+@Named
+class LiteralTypeConversionsConfiguration {
+    @Bean
+    public Map<Class<? extends Serializable>, Function<Object, Object>> literalTypeConversions() {
+        return new HashMap<Class<? extends Serializable>, Function<Object, Object>>() {{
+            put(String.class, Function.identity());
+            put(Boolean.class, Function.identity());
+            put(Integer.class, Function.identity());
+            put(Float.class, Function.identity());
+            put(Double.class, o -> ((Double) o).floatValue());
+        }};
+    }
+}
diff --git a/...ression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java b/...ression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java
@@ -9,19 +9,30 @@
 import org.antlr.v4.runtime.tree.TerminalNode;
 import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;
 
+import javax.inject.Inject;
 import javax.inject.Named;
+import java.io.Serializable;
+import java.util.Map;
+import java.util.function.Function;
 
 @Named
 class ParseTreeCoercionService {
+    private final Map<Class<? extends Serializable>, Function<Object, Object>> literalTypeConversions;
+
+    @Inject
+    public ParseTreeCoercionService(final Map<Class<? extends Serializable>, Function<Object, Object>> literalTypeConversions) {
+        this.literalTypeConversions = literalTypeConversions;
+    }
+
     public Object coercePrimaryTerminalNode(final TerminalNode node, final Event event) {
         final int nodeType = node.getSymbol().getType();
         final String nodeStringValue = node.getText();
         switch (nodeType) {
             case DataPrepperExpressionParser.EscapedJsonPointer:
                 final String jsonPointerWithoutQuotes = nodeStringValue.substring(1, nodeStringValue.length() - 1);
-                return event.get(jsonPointerWithoutQuotes, Object.class);
+                return resolveJsonPointerValue(jsonPointerWithoutQuotes, event);
             case DataPrepperExpressionParser.JsonPointer:
-                return event.get(nodeStringValue, Object.class);
+                return resolveJsonPointerValue(nodeStringValue, event);
             case DataPrepperExpressionParser.String:
                 return nodeStringValue;
             case DataPrepperExpressionParser.Integer:
@@ -42,4 +53,15 @@ public <T> T coerce(final Object obj, Class<T> clazz) throws ExpressionCoercionE
         }
         throw new ExpressionCoercionException("Unable to cast " + obj.getClass().getName() + " into " + clazz.getName());
     }
+
+    private Object resolveJsonPointerValue(final String jsonPointer, final Event event) {
+        final Object value = event.get(jsonPointer, Object.class);
+        if (value == null) {
+            return null;
+        } else if (literalTypeConversions.containsKey(value.getClass())) {
+            return literalTypeConversions.get(value.getClass()).apply(value);
+        } else {
+            throw new ExpressionCoercionException("Unsupported type for value " + value);
+        }
+    }
 }
diff --git a/...ion/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java b/...ion/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java
@@ -28,7 +28,6 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
@@ -47,7 +46,10 @@ class ParseTreeCoercionServiceTest {
     @Mock
     private Token token;
 
-    private final ParseTreeCoercionService objectUnderTest = new ParseTreeCoercionService();
+    private final LiteralTypeConversionsConfiguration literalTypeConversionsConfiguration =
+            new LiteralTypeConversionsConfiguration();
+    private final ParseTreeCoercionService objectUnderTest = new ParseTreeCoercionService(
+            literalTypeConversionsConfiguration.literalTypeConversions());
 
     @Test
     void testCoerceTerminalNodeStringType() {
@@ -97,36 +99,43 @@ void testCoerceTerminalNodeBooleanType() {
         assertThat(result, equalTo(testBoolean));
     }
 
-    @Test
-    void testCoerceTerminalNodeJsonPointerType() {
+    @ParameterizedTest
+    @MethodSource("provideSupportedJsonPointerValues")
+    void testCoerceTerminalNodeJsonPointerTypeSupportedValues(final Object testValue) {
         final String testKey1 = "key1";
         final String testKey2 = "key2";
-        final String testValue = "value";
         final String testJsonPointerKey = String.format("/%s/%s", testKey1, testKey2);
-        final Event testEvent = createTestEvent(Map.of(testKey1, Map.of(testKey2, testValue)));
+        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);
         final Object result = objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent);
-        assertThat(result, instanceOf(String.class));
-        assertThat(result, equalTo(testValue));
+        if (testValue instanceof Double) {
+            assertThat(result, instanceOf(Float.class));
+            assertThat(result, equalTo(((Double) testValue).floatValue()));
+        } else {
+            assertThat(result, equalTo(testValue));
+        }
     }
 
-    @Test
-    void testCoerceTerminalNodeJsonPointerTypeMissingKey() {
-        final String testMissingKey = "missingKey";
-        final String testJsonPointerKey = "/" + testMissingKey;
-        final Event testEvent = createTestEvent(new HashMap<>());
+    @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);
-        final Object result = objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent);
-        assertThat(result, nullValue());
+        assertThrows(ExpressionCoercionException.class, () -> objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent));
     }
 
     @ParameterizedTest
     @MethodSource("provideKeys")
-    void testCoerceTerminalNodeEscapeJsonPointerType(final String testKey, final String testEscapeJsonPointer)
+    void testCoerceTerminalNodeEscapeJsonPointerTypeWithSpecialCharacters(final String testKey, final String testEscapeJsonPointer)
             throws ExpressionCoercionException {
         final String testValue = "test value";
         final Event testEvent = createTestEvent(Map.of(testKey, testValue));
@@ -138,6 +147,38 @@ void testCoerceTerminalNodeEscapeJsonPointerType(final String testKey, final Str
         assertThat(result, equalTo(testValue));
     }
 
+    @ParameterizedTest
+    @MethodSource("provideSupportedJsonPointerValues")
+    void testCoerceTerminalNodeEscapeJsonPointerTypeSupportedValues(final Object testValue) {
+        final String testKey = "testKey";
+        final String testEscapeJsonPointerKey = String.format("\"/%s\"", testKey);
+        final Event testEvent = testValue == null ? createTestEvent(new HashMap<>()) :
+                createTestEvent(Map.of(testKey, testValue));
+        when(token.getType()).thenReturn(DataPrepperExpressionParser.EscapedJsonPointer);
+        when(terminalNode.getSymbol()).thenReturn(token);
+        when(terminalNode.getText()).thenReturn(testEscapeJsonPointerKey);
+        final Object result = objectUnderTest.coercePrimaryTerminalNode(terminalNode, testEvent);
+        if (testValue instanceof Double) {
+            assertThat(result, instanceOf(Float.class));
+            assertThat(result, equalTo(((Double) testValue).floatValue()));
+        } else {
+            assertThat(result, equalTo(testValue));
+        }
+    }
+
+    @ParameterizedTest
+    @MethodSource("provideUnSupportedJsonPointerValues")
+    void testCoerceTerminalNodeEscapeJsonPointerTypeUnSupportedValues(final Object testValue) {
+        final String testKey = "testKey";
+        final String testEscapeJsonPointerKey = String.format("\"/%s\"", testKey);
+        final Event testEvent = testValue == null ? createTestEvent(new HashMap<>()) :
+                createTestEvent(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 testCoerceTerminalNodeUnsupportedType() {
         final Event testEvent = createTestEvent(new HashMap<>());
@@ -184,4 +225,19 @@ private static Stream<Arguments> provideKeys() {
                 Arguments.of("test~0key", "\"/test~00key\"")
         );
     }
+
+    private static Stream<Arguments> provideSupportedJsonPointerValues() {
+        return Stream.of(
+                Arguments.of(1000),
+                Arguments.of(true),
+                Arguments.of("test value"),
+                Arguments.of(1.1f),
+                Arguments.of(1.1),
+                Arguments.of((Object) null)
+        );
+    }
+
+    private static Stream<Arguments> provideUnSupportedJsonPointerValues() {
+        return Stream.of(Arguments.of(Long.MAX_VALUE));
+    }
 }
diff --git a/...n/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java b/...n/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java
@@ -30,7 +30,9 @@ class ParseTreeEvaluatorListenerTest {
     private final ParseTreeWalker walker = new ParseTreeWalker();
     private final ParseTreeParser parseTreeParser = constructParseTreeParser();
     private final OperatorFactory operatorFactory = new OperatorFactory();
-    private final ParseTreeCoercionService coercionService = new ParseTreeCoercionService();
+    private final LiteralTypeConversionsConfiguration literalTypeConversionsConfiguration = new LiteralTypeConversionsConfiguration();
+    private final ParseTreeCoercionService coercionService = new ParseTreeCoercionService(
+            literalTypeConversionsConfiguration.literalTypeConversions());
     private final List<Operator<?>> operators = Arrays.asList(
             new AndOperator(), new OrOperator(),
             operatorFactory.inSetOperator(), operatorFactory.notInSetOperator(),
@@ -115,7 +117,7 @@ void testSimpleEqualityOperatorExpressionWithLiteralType() {
     }
 
     @Test
-    void testSimpleEqualityOperatorExpressionWithJsonPointerTypeExistingKey() {
+    void testSimpleEqualityOperatorExpressionWithJsonPointerType() {
         final String testKey = "testKey";
         final Integer testValue = random.nextInt(1000);
         final Map<String, Integer> data = Map.of(testKey, testValue);
@@ -126,17 +128,6 @@ void testSimpleEqualityOperatorExpressionWithJsonPointerTypeExistingKey() {
         assertThat(evaluateStatementOnEvent(notEqualStatement, testEvent), is(true));
     }
 
-    @Test
-    void testSimpleEqualityOperatorExpressionWithJsonPointerTypeMissingKey() {
-        final String testMissingKey1 = "missingKey1";
-        final String testMissingKey2 = "missingKey2";
-        final String equalStatement = String.format("/%s == /%s", testMissingKey1, testMissingKey2);
-        final String notEqualStatement = String.format("/%s != 1", testMissingKey1);
-        final Event testEvent = createTestEvent(new HashMap<>());
-        assertThat(evaluateStatementOnEvent(equalStatement, testEvent), is(true));
-        assertThat(evaluateStatementOnEvent(notEqualStatement, testEvent), is(true));
-    }
-
     @Test
     void testSimpleEqualityOperatorExpressionWithEscapeJsonPointerType() {
         final String testKey = "testKey";