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

Fix: IllegalArgument Exception in String converter #3172

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Fix: IllegalArgument Exception in String converter #3172

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3b5f91
Fix: IllegalArgument Exception in String converter
asifsmohammed Aug 15, 2023
460251e
Added tags_on_failure
asifsmohammed Aug 18, 2023
72d4584
Addressed feedback
asifsmohammed Aug 21, 2023
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
18 changes: 16 additions & 2 deletions data-prepper-api/src/main/java/org/opensearch/dataprepper/typeconverter/StringConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,17 @@

package org.opensearch.dataprepper.typeconverter;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Objects;

import static org.opensearch.dataprepper.logging.DataPrepperMarkers.EVENT;

public class StringConverter implements TypeConverter<String> {
public String convert(Object source) throws IllegalArgumentException {
private static final Logger LOG = LoggerFactory.getLogger(StringConverter.class);

public String convert(final Object source) {
if (source instanceof Long) {
return Long.toString(((Number)source).longValue());
}
Expand All @@ -25,6 +34,11 @@ public String convert(Object source) throws IllegalArgumentException {
if (source instanceof String) {
return (String)source;
}
throw new IllegalArgumentException("Unsupported type conversion");
LOG.error(EVENT, "Unable to convert {} to String", source);
if (Objects.nonNull(source)) {
return source.toString();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is really a good behavior. It could result in strange output. Say a list is provided. The user will get a string that looks like ListString@89879 or something like that.

I think this should continue to throw the exception.

We can handle the exception in the ConvertEntryTypeProcessor class to make it consistent across all types.

Perhaps by dropping the data?

} else {
return "";
Copy link
Member

Choose a reason for hiding this comment

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

In the case of null, we can probably handle this in the ConvertEntryTypeProcessor class. Any null value can be converted to null for the target.

Also, it should not be an empty string. It should be null.

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 can return null string in case of null value instead of empty string. convert method is also used in translate processor.

}
}
}
25 changes: 20 additions & 5 deletions ...pper-api/src/test/java/org/opensearch/dataprepper/typeconverter/StringConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@

import org.junit.jupiter.api.Test;

import java.util.Collections;
import java.util.List;
import java.util.Map;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class StringConverterTests {
@Test
Expand Down Expand Up @@ -64,9 +63,25 @@ void testStringToStringConversion() {
assertThat(converter.convert(strConstant), equalTo(strConstant));
}
@Test
void testInvalidStringConversion() {
void testNullValueStringConversion() {
StringConverter converter = new StringConverter();
final Map<Object, Object> map = Collections.emptyMap();
assertThrows(IllegalArgumentException.class, () -> converter.convert(map));
final String expectedString = "";
assertThat(converter.convert(null), equalTo(expectedString));
}

@Test
void testMapToStringConversion() {
StringConverter converter = new StringConverter();
final Map<Object, Object> map = Map.of("testKey", "testValue");
final String expectedString = "{testKey=testValue}";
assertThat(converter.convert(map), equalTo(expectedString));
}

@Test
void testListToStringConversion() {
StringConverter converter = new StringConverter();
final List<Object> list = List.of("listItem1", "listItem2");
final String expectedString = "[listItem1, listItem2]";
assertThat(converter.convert(list), equalTo(expectedString));
}
}
Loading