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 3 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
30 changes: 28 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 @@ -10,18 +10,33 @@
import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import javax.inject.Named;
import java.io.Serializable;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

@Named
class ParseTreeCoercionService {
private static final Map<Class<? extends Serializable>, Function<Object, Object>> literalTypes;

static {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be moved to a constructor argument and generated in a bean configuration function? Static variable can make classes difficult to unit test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Works for me.

literalTypes = new HashMap<>();
literalTypes.put(String.class, Function.identity());
Copy link
Member

Choose a reason for hiding this comment

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

Are all of these keys needed? Could we use something like

if (literalTypes.hasKey(value.getClass()))
    return literalTypes..get(value.getClass()).apply(value);
else
    return value;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that will not work for Long type which we want to throw exception. Also, for other custom types, it will just return literal objects which I am not sure if we want to support. e.g.

/code1 == /code2              | {"code1": StatusCode, "code2": StatusCode}

literalTypes.put(Boolean.class, Function.identity());
literalTypes.put(Integer.class, Function.identity());
literalTypes.put(Float.class, Function.identity());
literalTypes.put(Double.class, v -> ((Double) v).floatValue());
}

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 +57,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 (literalTypes.containsKey(value.getClass())) {
return literalTypes.get(value.getClass()).apply(value);
} else {
throw new ExpressionCoercionException("Unsupported type for value " + value);
}
}
}
83 changes: 68 additions & 15 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 Down Expand Up @@ -97,36 +96,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 +144,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 +222,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));
}
}
13 changes: 1 addition & 12 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 @@ -115,7 +115,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 +126,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