Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: coercion on json pointer values #1178

Merged
merged 5 commits into from
Mar 11, 2022
Merged

FIX: coercion on json pointer values #1178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a200fc
FIX: coercion on jsonpointer values
chenqi0805 Mar 11, 2022
7bd9c85
MAINT: compactify type checking
chenqi0805 Mar 11, 2022
56c265d
STY: unused import
chenqi0805 Mar 11, 2022
6091ea5
ENH: use bean injection instead of static variable
chenqi0805 Mar 11, 2022
61a8715
STY: unused import
chenqi0805 Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .../main/java/org/opensearch/dataprepper/expression/LiteralTypeConversionsConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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());
}};
}
}
26 changes: 24 additions & 2 deletions ...ression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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);
}
}
}
88 changes: 72 additions & 16 deletions ...ion/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could testValue type be changed to Event and create the events in the provideUnSupportedJsonPointerValues method? I think it would make the test more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the same method is used for both jsonpointer and escape jsonpointer test case in which keys are separate. So providing the value only will probably make it more compact and focused on the difference (testValue). Thought?

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));
Expand All @@ -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<>());
Expand Down Expand Up @@ -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));
}
}
17 changes: 4 additions & 13 deletions ...n/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
Expand All @@ -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";
Expand Down