Add jsonValue check on Enum deserializer, default to checking name() …

…function of enum (opensearch-project#5103) Signed-off-by: Taylor Gray <tylgry@amazon.com>
graytaylor0 · Oct 25, 2024 · 9065acb · 9065acb
1 parent 0653e74
commit 9065acb
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 16 deletions.
diff --git a/.../src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java b/.../src/integrationTest/java/org/opensearch/dataprepper/integration/PipelinesWithAcksIT.java
@@ -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;
@@ -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";

diff --git a/...repper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java b/...repper-core/src/main/java/org/opensearch/dataprepper/core/parser/PipelineTransformer.java
@@ -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)

diff --git a/...r-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java b/...r-core/src/test/java/org/opensearch/dataprepper/core/parser/PipelineTransformerTests.java
@@ -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

diff --git a/...ine-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java b/...ine-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializer.java
@@ -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));
@@ -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) {
@@ -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 {
@@ -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));

diff --git a/...parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java b/...parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java
@@ -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));
+    }
+
     @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);
 
@@ -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
@@ -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())
@@ -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;
+    }
 }
diff --git a/...-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java b/...-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java
@@ -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()