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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 0 additions & 11 deletions data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEvent.java
View file
Open in desktop
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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the motivation for not allowing these characters initially? I'm wondering what the consequences of this change would be

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what the reasoning was

Copy link
Member Author

@graytaylor0 graytaylor0 Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is the PR where I added this custom code (https://github.com/opensearch-project/data-prepper/pull/725/files). But the regex pattern that was used to validate before (which disallowed starting or ending with these characters) was added by @cmanning09

}

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
5 changes: 2 additions & 3 deletions data-prepper-api/src/test/java/org/opensearch/dataprepper/model/event/JacksonEventTest.java
View file
Open in desktop
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
55 changes: 29 additions & 26 deletions .../main/java/org/opensearch/dataprepper/plugins/processor/parsejson/ParseJsonProcessor.java
View file
Open in desktop
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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: seems like this block is double indented which is showing more of a diff than there actually is

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