From c29f002653ee36b73136e9eacbfe43f89b5c84fe Mon Sep 17 00:00:00 2001 From: Taylor Gray Date: Fri, 25 Oct 2024 16:48:31 -0500 Subject: [PATCH] Revert "Add jsonValue check on Enum deserializer, default to checking name() function of enum (#5103)" This reverts commit 9065acbdbd72c89772a4a7679355626c3cd732a2. --- .../integration/PipelinesWithAcksIT.java | 2 - .../core/parser/PipelineTransformer.java | 4 +- .../core/parser/PipelineTransformerTests.java | 7 +- .../pipeline/parser/EnumDeserializer.java | 12 ++-- .../pipeline/parser/EnumDeserializerTest.java | 64 ++----------------- .../plugin/ObjectMapperConfiguration.java | 1 - 6 files changed, 16 insertions(+), 74 deletions(-) diff --git a/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java b/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java index e1893dffa7..4b5c4106d8 100644 --- a/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java +++ b/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java @@ -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; @@ -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"; diff --git a/data-prepper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java b/data-prepper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java index 374b9629c9..c8c7f2ed5e 100644 --- a/data-prepper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java +++ b/data-prepper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java @@ -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) diff --git a/data-prepper-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java b/data-prepper-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java index 9caa18820f..2725bcbd26 100644 --- a/data-prepper-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java +++ b/data-prepper-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java @@ -222,11 +222,16 @@ void parseConfiguration_with_invalid_root_source_pipeline_creates_empty_pipeline final Map 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 diff --git a/data-prepper-pipeline-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java b/data-prepper-pipeline-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java index e31fe6314f..e95f10b962 100644 --- a/data-prepper-pipeline-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java +++ b/data-prepper-pipeline-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java @@ -44,7 +44,6 @@ public Enum deserialize(final JsonParser p, final DeserializationContext ctxt final String enumValue = node.asText(); final Optional jsonCreator = findJsonCreatorMethod(); - final Optional jsonValueMethod = findJsonValueMethodForClass(); try { jsonCreator.ifPresent(method -> method.setAccessible(true)); @@ -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) { @@ -68,6 +63,9 @@ public Enum deserialize(final JsonParser p, final DeserializationContext ctxt jsonCreator.ifPresent(method -> method.setAccessible(false)); } + + + final Optional jsonValueMethod = findJsonValueMethodForClass(); final List listOfEnums = jsonValueMethod.map(method -> Arrays.stream(enumClass.getEnumConstants()) .map(valueEnum -> { try { @@ -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)); diff --git a/data-prepper-pipeline-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java b/data-prepper-pipeline-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java index 1276ccad7a..990fd60e7d 100644 --- a/data-prepper-pipeline-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java +++ b/data-prepper-pipeline-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java @@ -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); @@ -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 @@ -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 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 NAMES_MAP = Arrays.stream(TestEnumWithoutJsonCreator.values()) @@ -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; - } } diff --git a/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java b/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java index 513ff3d2fa..0ec428397b 100644 --- a/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java +++ b/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java @@ -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()