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.

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

Remove validation that made keys starting or ending with . - or _ invalid, catch all exceptions in the parse json processor #2945

Merged
merged 2 commits into from
Jun 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -373,19 +373,9 @@ private String trimKey(final String key) {
}

private boolean isValidKey(final String key) {
char previous = ' ';
char next = ' ';
for (int i = 0; i < key.length(); i++) {
char c = key.charAt(i);

if (i < key.length() - 1) {
next = key.charAt(i + 1);
}

if ((i == 0 || i == key.length() - 1 || previous == '/' || next == '/') && (c == '_' || c == '.' || c == '-')) {
return false;
}

if (!(c >= 48 && c <= 57
|| c >= 65 && c <= 90
|| c >= 97 && c <= 122
Expand All @@ -397,7 +387,6 @@ private boolean isValidKey(final String key) {

return false;
}
previous = c;
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,8 @@ public void testIsValueAList_withNull() {
}

@ParameterizedTest
@ValueSource(strings = {"", "withSpecialChars*$%", "-withPrefixDash", "\\-withEscapeChars", "\\\\/withMultipleEscapeChars",
"withDashSuffix-", "withDashSuffix-/nestedKey", "withDashPrefix/-nestedKey", "_withUnderscorePrefix", "withUnderscoreSuffix_",
".withDotPrefix", "withDotSuffix.", "with,Comma", "with:Colon", "with[Bracket", "with|Brace"})
@ValueSource(strings = {"", "withSpecialChars*$%", "\\-withEscapeChars", "\\\\/withMultipleEscapeChars",
"with,Comma", "with:Colon", "with[Bracket", "with|Brace"})
void testKey_withInvalidKey_throwsIllegalArgumentException(final String invalidKey) {
assertThrowsForKeyCheck(IllegalArgumentException.class, invalidKey);
}
Expand Down
1 change: 0 additions & 1 deletion data-prepper-plugins/opensearch-source/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,3 @@ The default behavior is to process all indices.
#### <a name="index_configuration">Index Configuration</a>

* `index_name_regex`: A regex pattern to represent the index names for filtering

Original file line number Diff line number Diff line change
Expand Up @@ -64,34 +64,37 @@ public Collection<Record<Event>> doExecute(final Collection<Record<Event>> recor
final boolean doUsePointer = Objects.nonNull(pointer);

for (final Record<Event> record : records) {
final Event event = record.getData();

if (Objects.nonNull(parseWhen) && !expressionEvaluator.evaluateConditional(parseWhen, event)) {
continue;
}

final String message = event.get(source, String.class);
if (Objects.isNull(message)) {
continue;
}

try {
final TypeReference<HashMap<String, Object>> hashMapTypeReference = new TypeReference<HashMap<String, Object>>() {};
Map<String, Object> parsedJson = objectMapper.readValue(message, hashMapTypeReference);

if (doUsePointer) {
parsedJson = parseUsingPointer(event, parsedJson, pointer, doWriteToRoot);
}

if (doWriteToRoot) {
writeToRoot(event, parsedJson);
} else {
event.put(destination, parsedJson);
final Event event = record.getData();
try {
if (Objects.nonNull(parseWhen) && !expressionEvaluator.evaluateConditional(parseWhen, event)) {
continue;
}

final String message = event.get(source, String.class);
if (Objects.isNull(message)) {
continue;
}
final TypeReference<HashMap<String, Object>> hashMapTypeReference = new TypeReference<HashMap<String, Object>>() {
};
Map<String, Object> parsedJson = objectMapper.readValue(message, hashMapTypeReference);

if (doUsePointer) {
parsedJson = parseUsingPointer(event, parsedJson, pointer, doWriteToRoot);
}

if (doWriteToRoot) {
writeToRoot(event, parsedJson);
} else {
event.put(destination, parsedJson);
}
} catch (final JsonProcessingException jsonException) {
event.getMetadata().addTags(tagsOnFailure);
LOG.error(EVENT, "An exception occurred due to invalid JSON while reading event [{}]", event, jsonException);
} catch (final Exception e) {
event.getMetadata().addTags(tagsOnFailure);
LOG.error(EVENT, "An exception occurred while using the parse_json processor on Event [{}]", event, e);
}
} catch (final JsonProcessingException jsonException) {
event.getMetadata().addTags(tagsOnFailure);
LOG.error(EVENT, "An exception occurred due to invalid JSON while reading event [{}]", event, jsonException);
}
}
return records;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,25 @@ void test_tags_when_json_parse_fails() {
assertTrue(parsedEvent.getMetadata().hasTags(testTags));
}

@Test
void when_evaluate_conditional_throws_RuntimeException_events_are_not_dropped() {
final String source = "different_source";
final String destination = "destination_key";
when(processorConfig.getSource()).thenReturn(source);
when(processorConfig.getDestination()).thenReturn(destination);
final String whenCondition = UUID.randomUUID().toString();
when(processorConfig.getParseWhen()).thenReturn(whenCondition);
final Map<String, Object> data = Collections.singletonMap("key", "value");
final String serializedMessage = convertMapToJSONString(data);
final Record<Event> testEvent = createMessageEvent(serializedMessage);
when(expressionEvaluator.evaluateConditional(whenCondition, testEvent.getData())).thenThrow(RuntimeException.class);
parseJsonProcessor = createObjectUnderTest();

final Event parsedEvent = createAndParseMessageEvent(testEvent);

assertThat(parsedEvent.toMap(), equalTo(testEvent.getData().toMap()));
}

private String constructDeeplyNestedJsonPointer(final int numberOfLayers) {
String pointer = "/" + DEEPLY_NESTED_KEY_NAME;
for (int layer = 0; layer < numberOfLayers; layer++) {
Expand Down
Loading