Skip to content

Commit

Permalink
Revert "Add jsonValue check on Enum deserializer, default to checking…
Browse files Browse the repository at this point in the history 
… name() function of enum (opensearch-project#5103)"

This reverts commit 9065acb.
  • Loading branch information
@graytaylor0
graytaylor0 committed Oct 25, 2024
1 parent 9065acb commit c29f002
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 74 deletions.
2 changes: 0 additions & 2 deletions .../src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

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 @@ -29,7 +28,6 @@
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: 1 addition & 3 deletions ...repper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,7 @@ private void buildPipelineFromConfiguration(
Buffer pipelineDefinedBuffer = null;
final PluginSetting bufferPluginSetting = pipelineConfiguration.getBufferPluginSetting();
try {
if (source != null) {
pipelineDefinedBuffer = pluginFactory.loadPlugin(Buffer.class, bufferPluginSetting, source.getDecoder());
}
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: 6 additions & 1 deletion ...r-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,16 @@ 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(1));
assertThat(pluginErrorCollector.getPluginErrors().size(), equalTo(2));
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: 5 additions & 7 deletions ...ine-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ 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 @@ -53,11 +52,7 @@ 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() && jsonValueMethod.isPresent()
&& jsonValueMethod.get().invoke(enumConstant).equals(enumValue)) {
return (Enum<?>) enumConstant;
} else if (jsonCreator.isEmpty() && jsonValueMethod.isEmpty() &&
((Enum<?>) enumConstant).name().equals(enumValue)) {
} else if (jsonCreator.isEmpty() && enumConstant.toString().toLowerCase().equals(enumValue)) {
return (Enum<?>) enumConstant;
}
} catch (IllegalAccessException | InvocationTargetException e) {
Expand All @@ -68,6 +63,9 @@ 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 @@ -77,7 +75,7 @@ public Enum<?> deserialize(final JsonParser p, final DeserializationContext ctxt
}
})
.collect(Collectors.toList())).orElseGet(() -> Arrays.stream(enumClass.getEnumConstants())
.map(valueEnum -> ((Enum<?>) valueEnum).name())
.map(valueEnum -> valueEnum.toString().toLowerCase())
.collect(Collectors.toList()));

throw new IllegalArgumentException(String.format(INVALID_ENUM_VALUE_ERROR_FORMAT, enumValue, listOfEnums));
Expand Down
64 changes: 4 additions & 60 deletions ...parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,45 +66,15 @@ 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));
}

@ParameterizedTest
@EnumSource(TestEnumWithoutJsonCreator.class)
void enum_class_without_json_creator_or_json_value_annotation_returns_expected_enum_constant(final TestEnumWithoutJsonCreator enumWithoutJsonCreator) throws IOException {
void enum_class_without_json_creator_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.name()));
when(objectMapper.readTree(jsonParser)).thenReturn(new TextNode(enumWithoutJsonCreator.toString()));

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

Expand Down Expand Up @@ -146,7 +116,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 @@ -185,27 +155,6 @@ 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 @@ -215,16 +164,11 @@ private enum TestEnumWithoutJsonCreator {
this.name = name;
}
public String toString() {
return UUID.randomUUID().toString();
return this.name;
}

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

private enum TestEnumOnlyUppercase {
VALUE_ONE,
VALUE_TWO;
}
}
1 change: 0 additions & 1 deletion ...-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ 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

0 comments on commit c29f002

Please sign in to comment.