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 1 commit
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
27 changes: 25 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,28 @@
import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import javax.inject.Named;
import java.io.Serializable;
import java.util.Arrays;
import java.util.List;

@Named
class ParseTreeCoercionService {
private final List<? extends Class<? extends Serializable>> literalTypes = Arrays.asList(
Copy link
Member

Choose a reason for hiding this comment

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

This can be static.

String.class,
Boolean.class,
Integer.class,
Float.class
);

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 +52,17 @@ 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) {
throw new ExpressionCoercionException("Unsupported type for value " + value);
Copy link
Member

Choose a reason for hiding this comment

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

Why does a null value throw?

I see that there is no clear default value to use in this case. Maybe just return a new Object or have a NullValue class which represents any null values.

Copy link
Collaborator Author

@chenqi0805 chenqi0805 Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right now there is no null value support in the grammar literal types so it is impossible for cx to check jsonpointer value is null in conditional expression. We have two options for null appearing in jsonPointer value:

  • throw Exception
  • return null and let each operator complain about the arg type.

I think both should work and I do not favor one over the other. @sbayer55 thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

So in the current design /message == 200 would have to fail if /message is null? I think this is clearly a false expression. I think if we returned a special NullValue class the rest of the code would work as expected. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this code (currently in PR), I think the equals operator would work correctly if the ParseTreeCoercsionService provided a new Object().

https://github.com/opensearch-project/data-prepper/pull/1177/files#diff-083f9ffc9761303a542e0904c2de3bc5ba2ec7ef612278087bf6da29208ab961R34

We could even do something like this:

class NullValue {
  private static NULL_VALUE = new NullValue();

  private NullValue() { }

  public static nullValue() {
    return NULL_VALUE;
  }
}

And return:

return NullValue.nullValue()

} else if (literalTypes.contains(value.getClass())) {
return value;
} else if (value instanceof Double) {
Copy link
Member

Choose a reason for hiding this comment

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

This approach would be easier to extend if we have other possible types.

private static final Map<Class, Function<Object, Object>> literalTypes;

static {
  literalTypes = new HashMap<>();
  literalTypes.put(String.class, Function.identity());
  literalTypes.put(Boolean.class, Function.identity());
  literalTypes.put(Integer.class, Function.identity());
  literalTypes.put(Float.class, Function.identity());
  literalTypes.put(Double.class, v -> ((Double) value).floatValue());
}

And here:

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

But, this is all internal and later refactoring is light. So I don't have a strong opinion on it.

return ((Double) value).floatValue();
} else {
throw new ExpressionCoercionException("Unsupported type for value " + value);
}
}
}
81 changes: 67 additions & 14 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,42 @@ 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)));
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 +143,37 @@ 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 = 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 +220,21 @@ 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)
);
}

private static Stream<Arguments> provideUnSupportedJsonPointerValues() {
return Stream.of(
Arguments.of(Long.MAX_VALUE),
Arguments.of((Object) null)
);
}
}
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