From 02a9050ec90e3be4f84d09a4971b0f811bae2d2e Mon Sep 17 00:00:00 2001 From: Qi Chen <19492223+chenqi0805@users.noreply.github.com> Date: Fri, 11 Mar 2022 17:13:20 -0600 Subject: [PATCH] FIX: coercion on json pointer values (#1178) * FIX: coercion on jsonpointer values Signed-off-by: Chen <19492223+chenqi0805@users.noreply.github.com> * MAINT: compactify type checking Signed-off-by: Chen <19492223+chenqi0805@users.noreply.github.com> * STY: unused import Signed-off-by: Chen <19492223+chenqi0805@users.noreply.github.com> * ENH: use bean injection instead of static variable Signed-off-by: Chen <19492223+chenqi0805@users.noreply.github.com> * STY: unused import Signed-off-by: Chen <19492223+chenqi0805@users.noreply.github.com> --- .../LiteralTypeConversionsConfiguration.java | 28 ++++++ .../expression/ParseTreeCoercionService.java | 26 +++++- .../ParseTreeCoercionServiceTest.java | 88 +++++++++++++++---- .../ParseTreeEvaluatorListenerTest.java | 17 +--- 4 files changed, 128 insertions(+), 31 deletions(-) create mode 100644 data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java diff --git a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java new file mode 100644 index 0000000000..9fc3dbc51f --- /dev/null +++ b/data-prepper-expression/src/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, Function> literalTypeConversions() { + return new HashMap, Function>() {{ + 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/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java b/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java index d75f7d9d9c..fb8f56230d 100644 --- a/data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java +++ b/data-prepper-expression/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, Function> literalTypeConversions; + + @Inject + public ParseTreeCoercionService(final Map, Function> 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 coerce(final Object obj, Class 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/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java b/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java index 41cbacafb6..9075b99dd8 100644 --- a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java +++ b/data-prepper-expression/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 provideKeys() { Arguments.of("test~0key", "\"/test~00key\"") ); } + + private static Stream 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 provideUnSupportedJsonPointerValues() { + return Stream.of(Arguments.of(Long.MAX_VALUE)); + } } \ No newline at end of file diff --git a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java b/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java index 8b5a3e6303..2effeced0b 100644 --- a/data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java +++ b/data-prepper-expression/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> 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 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";