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

Add jsonValue check on Enum deserializer, default to checking name() function of enum #5103

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Add jsonValue check on Enum deserializer, default to checking name() function of enum #5103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4153147
Ignore case sensitivity when deserializing enums
graytaylor0 Oct 24, 2024
6522706
Add jsonValue check on Enum deserializer, default to checking name() …
graytaylor0 Oct 24, 2024
7c61cf5
Address comments on test cases
graytaylor0 Oct 25, 2024
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
2 changes: 2 additions & 0 deletions .../src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.dataprepper.integration;

import org.junit.FixMethodOrder;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.opensearch.dataprepper.model.event.Event;
Expand All @@ -28,6 +29,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

@FixMethodOrder()
class PipelinesWithAcksIT {
private static final Logger LOG = LoggerFactory.getLogger(PipelinesWithAcksIT.class);
private static final String IN_MEMORY_IDENTIFIER = "PipelinesWithAcksIT";
Expand Down
4 changes: 3 additions & 1 deletion ...repper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ private void buildPipelineFromConfiguration(
Buffer pipelineDefinedBuffer = null;
final PluginSetting bufferPluginSetting = pipelineConfiguration.getBufferPluginSetting();
try {
pipelineDefinedBuffer = pluginFactory.loadPlugin(Buffer.class, bufferPluginSetting, source.getDecoder());
if (source != null) {
pipelineDefinedBuffer = pluginFactory.loadPlugin(Buffer.class, bufferPluginSetting, source.getDecoder());
}
} catch (Exception e) {
final PluginError pluginError = PluginError.builder()
.componentType(PipelineModel.BUFFER_PLUGIN_TYPE)
Expand Down
7 changes: 1 addition & 6 deletions ...r-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,11 @@ void parseConfiguration_with_invalid_root_source_pipeline_creates_empty_pipeline
final Map<String, Pipeline> connectedPipelines = pipelineTransformer.transformConfiguration();
assertThat(connectedPipelines.size(), equalTo(0));
verify(dataPrepperConfiguration).getPipelineExtensions();
assertThat(pluginErrorCollector.getPluginErrors().size(), equalTo(2));
assertThat(pluginErrorCollector.getPluginErrors().size(), equalTo(1));
final PluginError sourcePluginError = pluginErrorCollector.getPluginErrors().get(0);
assertThat(sourcePluginError.getPipelineName(), equalTo("test-pipeline-1"));
assertThat(sourcePluginError.getPluginName(), equalTo("file"));
assertThat(sourcePluginError.getException(), notNullValue());
// Buffer plugin gets error due to instantiated source is null
final PluginError bufferPluginError = pluginErrorCollector.getPluginErrors().get(1);
assertThat(bufferPluginError.getPipelineName(), equalTo("test-pipeline-1"));
assertThat(bufferPluginError.getPluginName(), equalTo("bounded_blocking"));
assertThat(bufferPluginError.getException(), notNullValue());
}

@ParameterizedTest
Expand Down
12 changes: 7 additions & 5 deletions ...ine-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public Enum<?> deserialize(final JsonParser p, final DeserializationContext ctxt
final String enumValue = node.asText();

final Optional<Method> jsonCreator = findJsonCreatorMethod();
final Optional<Method> jsonValueMethod = findJsonValueMethodForClass();

try {
jsonCreator.ifPresent(method -> method.setAccessible(true));
Expand All @@ -52,7 +53,11 @@ public Enum<?> deserialize(final JsonParser p, final DeserializationContext ctxt
try {
if (jsonCreator.isPresent() && enumConstant.equals(jsonCreator.get().invoke(null, enumValue))) {
return (Enum<?>) enumConstant;
} else if (jsonCreator.isEmpty() && enumConstant.toString().toLowerCase().equals(enumValue)) {
} else if (jsonCreator.isEmpty() && jsonValueMethod.isPresent()
&& jsonValueMethod.get().invoke(enumConstant).equals(enumValue)) {
return (Enum<?>) enumConstant;
} else if (jsonCreator.isEmpty() && jsonValueMethod.isEmpty() &&
((Enum<?>) enumConstant).name().equals(enumValue)) {
return (Enum<?>) enumConstant;
}
} catch (IllegalAccessException | InvocationTargetException e) {
Expand All @@ -63,9 +68,6 @@ public Enum<?> deserialize(final JsonParser p, final DeserializationContext ctxt
jsonCreator.ifPresent(method -> method.setAccessible(false));
}



final Optional<Method> jsonValueMethod = findJsonValueMethodForClass();
final List<Object> listOfEnums = jsonValueMethod.map(method -> Arrays.stream(enumClass.getEnumConstants())
.map(valueEnum -> {
try {
Expand All @@ -75,7 +77,7 @@ public Enum<?> deserialize(final JsonParser p, final DeserializationContext ctxt
}
})
.collect(Collectors.toList())).orElseGet(() -> Arrays.stream(enumClass.getEnumConstants())
.map(valueEnum -> valueEnum.toString().toLowerCase())
.map(valueEnum -> ((Enum<?>) valueEnum).name())
.collect(Collectors.toList()));

throw new IllegalArgumentException(String.format(INVALID_ENUM_VALUE_ERROR_FORMAT, enumValue, listOfEnums));
Expand Down
64 changes: 60 additions & 4 deletions ...parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,45 @@ void enum_class_with_json_creator_annotation_returns_expected_enum_constant(fina
assertThat(result, equalTo(testEnumOption));
}

@ParameterizedTest
@EnumSource(TestEnumWithJsonValue.class)
void enum_class_with_no_json_creator_and_a_json_value_annotation_returns_expected_enum_constant(final TestEnumWithJsonValue testEnumOption) throws IOException {
final EnumDeserializer objectUnderTest = createObjectUnderTest(TestEnumWithJsonValue.class);
final JsonParser jsonParser = mock(JsonParser.class);
final DeserializationContext deserializationContext = mock(DeserializationContext.class);
when(jsonParser.getCodec()).thenReturn(objectMapper);

when(objectMapper.readTree(jsonParser)).thenReturn(new TextNode(testEnumOption.toString()));

Enum<?> result = objectUnderTest.deserialize(jsonParser, deserializationContext);

assertThat(result, equalTo(testEnumOption));
}

@ParameterizedTest
@EnumSource(TestEnumOnlyUppercase.class)
void enum_class_with_just_enum_values_returns_expected_enum_constant(final TestEnumOnlyUppercase testEnumOption) throws IOException {
final EnumDeserializer objectUnderTest = createObjectUnderTest(TestEnumOnlyUppercase.class);
final JsonParser jsonParser = mock(JsonParser.class);
final DeserializationContext deserializationContext = mock(DeserializationContext.class);
when(jsonParser.getCodec()).thenReturn(objectMapper);

when(objectMapper.readTree(jsonParser)).thenReturn(new TextNode(testEnumOption.name()));

Enum<?> result = objectUnderTest.deserialize(jsonParser, deserializationContext);

assertThat(result, equalTo(testEnumOption));
}
Copy link
Member

Choose a reason for hiding this comment

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

We may also benefit from a test where the enum has a toString() which returns a different value, but is not marked with @JsonValue. In this case, we should be using the enum's name(). So we can verify this situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that already covered with this test void enum_class_without_json_creator_or_json_value_annotation_returns_expected_enum_constant

Copy link
Member

Choose a reason for hiding this comment

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

For that test, toString() returns the name, so you can't be sure if the code is using name() or toString().

Can you change that enum's toString() to return a random UUID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure will make that change


@ParameterizedTest
@EnumSource(TestEnumWithoutJsonCreator.class)
void enum_class_without_json_creator_annotation_returns_expected_enum_constant(final TestEnumWithoutJsonCreator enumWithoutJsonCreator) throws IOException {
void enum_class_without_json_creator_or_json_value_annotation_returns_expected_enum_constant(final TestEnumWithoutJsonCreator enumWithoutJsonCreator) throws IOException {
final EnumDeserializer objectUnderTest = createObjectUnderTest(TestEnumWithoutJsonCreator.class);
final JsonParser jsonParser = mock(JsonParser.class);
final DeserializationContext deserializationContext = mock(DeserializationContext.class);
when(jsonParser.getCodec()).thenReturn(objectMapper);

when(objectMapper.readTree(jsonParser)).thenReturn(new TextNode(enumWithoutJsonCreator.toString()));
when(objectMapper.readTree(jsonParser)).thenReturn(new TextNode(enumWithoutJsonCreator.name()));

Enum<?> result = objectUnderTest.deserialize(jsonParser, deserializationContext);

Expand Down Expand Up @@ -116,7 +146,7 @@ void enum_class_with_invalid_value_and_no_jsonValue_annotation_throws_IllegalArg
assertThat(exception, notNullValue());
final String expectedErrorMessage = "Invalid value \"" + invalidValue + "\". Valid options include";
assertThat(exception.getMessage(), Matchers.startsWith(expectedErrorMessage));

assertThat(exception.getMessage(), containsString("[TEST]"));
}

@Test
Expand Down Expand Up @@ -155,6 +185,27 @@ static TestEnum fromOptionValue(final String option) {
}
}

private enum TestEnumWithJsonValue {
TEST_ONE("test_json_value_one"),
TEST_TWO("test_json_value_two"),
TEST_THREE("test_json_value_three");
private static final Map<String, TestEnum> NAMES_MAP = Arrays.stream(TestEnum.values())
.collect(Collectors.toMap(TestEnum::toString, Function.identity()));
private final String name;
TestEnumWithJsonValue(final String name) {
this.name = name;
}

@JsonValue
public String toString() {
return this.name;
}

static TestEnum fromOptionValue(final String option) {
return NAMES_MAP.get(option);
}
}

private enum TestEnumWithoutJsonCreator {
TEST("test");
private static final Map<String, TestEnumWithoutJsonCreator> NAMES_MAP = Arrays.stream(TestEnumWithoutJsonCreator.values())
Expand All @@ -164,11 +215,16 @@ private enum TestEnumWithoutJsonCreator {
this.name = name;
}
public String toString() {
return this.name;
return UUID.randomUUID().toString();
}

static TestEnumWithoutJsonCreator fromOptionValue(final String option) {
return NAMES_MAP.get(option);
}
}

private enum TestEnumOnlyUppercase {
VALUE_ONE,
VALUE_TWO;
}
}
1 change: 1 addition & 0 deletions ...-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ ObjectMapper extensionPluginConfigObjectMapper() {
final SimpleModule simpleModule = new SimpleModule();
simpleModule.addDeserializer(Duration.class, new DataPrepperDurationDeserializer());
simpleModule.addDeserializer(Enum.class, new EnumDeserializer());

simpleModule.addDeserializer(ByteCount.class, new ByteCountDeserializer());

return new ObjectMapper()
Expand Down
Loading