INIT: better error handling

Signed-off-by: George Chen <qchea@amazon.com>
opensearch-project · Feb 24, 2025 · db8a48c · db8a48c
1 parent 94d1f15
commit db8a48c
Show file tree

Hide file tree

Showing 10 changed files with 169 additions and 13 deletions.
diff --git a/data-prepper-pipeline-parser/build.gradle b/data-prepper-pipeline-parser/build.gradle
@@ -16,6 +16,7 @@ dependencies {
     implementation 'com.fasterxml.jackson.core:jackson-databind:2.12.3'
     implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml'
     implementation 'org.apache.commons:commons-collections4:4.4'
+    implementation 'org.apache.commons:commons-text:1.13.0'
     implementation 'org.projectlombok:lombok:1.18.22'
     implementation 'com.jayway.jsonpath:json-path:2.6.0'
     implementation 'javax.inject:javax.inject:1'

diff --git a/.../org/opensearch/dataprepper/pipeline/parser/DataPrepperDeserializationProblemHandler.java b/.../org/opensearch/dataprepper/pipeline/parser/DataPrepperDeserializationProblemHandler.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.dataprepper.pipeline.parser;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.deser.ValueInstantiator;
+import com.fasterxml.jackson.databind.util.ClassUtil;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import java.io.IOException;
+
+@Named
+public class DataPrepperDeserializationProblemHandler extends DeserializationProblemHandler {
+
+    @Inject
+    public DataPrepperDeserializationProblemHandler() {
+        super();
+    }
+
+    @Override
+    public Object handleWeirdStringValue(DeserializationContext ctxt, Class<?> targetType, String valueToConvert, String failureMsg) throws IOException {
+        throw new IOException(
+                String.format("Cannot deserialize value of type %s from String \"%s\": %s",
+                        ClassUtil.nameOf(targetType), valueToConvert, failureMsg));
+    }
+
+    @Override
+    public Object handleUnexpectedToken(DeserializationContext ctxt, JavaType targetType, JsonToken t, JsonParser p, String failureMsg) throws IOException {
+        throw JsonMappingException.from(ctxt.getParser(),
+                String.format("Cannot deserialize value of type %s from %s.",
+                        ClassUtil.getTypeDescription(targetType), JsonToken.valueDescFor(t)));
+    }
+
+    @Override
+    public Object handleMissingInstantiator(DeserializationContext ctxt, Class<?> instClass, ValueInstantiator valueInsta, JsonParser p, String msg) throws IOException {
+        throw JsonMappingException.from(ctxt.getParser(),
+                String.format("Cannot deserialize '%s' into '%s'.", p.getText(), instClass.getName()));
+    }
+}
diff --git a/...r/src/main/java/org/opensearch/dataprepper/pipeline/parser/DataPrepperDurationParser.java b/...r/src/main/java/org/opensearch/dataprepper/pipeline/parser/DataPrepperDurationParser.java
@@ -20,14 +20,19 @@
 public class DataPrepperDurationParser {
     private static final String SIMPLE_DURATION_REGEX = "^(0|[1-9]\\d*)(s|ms)$";
     private static final Pattern SIMPLE_DURATION_PATTERN = Pattern.compile(SIMPLE_DURATION_REGEX);
+    private static final String INVALID_DURATION_ERROR_MESSAGE =
+            "Durations must use either ISO 8601 notation or simple notations for seconds (60s) or milliseconds (100ms). Whitespace is ignored.";
 
     public static Duration parse(final String durationString) {
+        if (durationString == null) {
+            throw new IllegalArgumentException(INVALID_DURATION_ERROR_MESSAGE);
+        }
         try {
             return Duration.parse(durationString);
         } catch (final DateTimeParseException e) {
             final Duration duration = parseSimpleDuration(durationString);
             if (duration == null) {
-                throw new IllegalArgumentException("Durations must use either ISO 8601 notation or simple notations for seconds (60s) or milliseconds (100ms). Whitespace is ignored.");
+                throw new IllegalArgumentException(INVALID_DURATION_ERROR_MESSAGE);
             }
             return duration;
         }

diff --git a/...rc/main/java/org/opensearch/dataprepper/pipeline/parser/PipelinesDataflowModelParser.java b/...rc/main/java/org/opensearch/dataprepper/pipeline/parser/PipelinesDataflowModelParser.java
@@ -26,7 +26,8 @@
 public class PipelinesDataflowModelParser {
     private static final Logger LOG = LoggerFactory.getLogger(PipelinesDataflowModelParser.class);
     private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(new YAMLFactory())
-            .enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY);
+            .enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY)
+            .addHandler(new DataPrepperDeserializationProblemHandler());
 
     private final PipelineConfigurationReader pipelineConfigurationReader;
 

diff --git a/.../opensearch/dataprepper/pipeline/parser/DataPrepperDeserializationProblemHandlerTest.java b/.../opensearch/dataprepper/pipeline/parser/DataPrepperDeserializationProblemHandlerTest.java
@@ -0,0 +1,78 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.dataprepper.pipeline.parser;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.deser.ValueInstantiator;
+import com.fasterxml.jackson.databind.util.ClassUtil;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import java.io.IOException;
+import java.util.UUID;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+class DataPrepperDeserializationProblemHandlerTest {
+    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+    private static final Class<?> TEST_TARGET_TYPE = Boolean.class;
+    private static final String TEST_VALUE = UUID.randomUUID().toString();
+    private static final String TEST_FAILURE_MESSAGE = UUID.randomUUID().toString();
+
+    @Mock
+    private DeserializationContext deserializationContext;
+
+    @Mock
+    private JsonParser jsonParser;
+
+    @Mock
+    private ValueInstantiator valueInstantiator;
+
+    private final DataPrepperDeserializationProblemHandler objectUnderTest = new DataPrepperDeserializationProblemHandler();
+
+    @Test
+    void testHandleWeirdStringValue() {
+        final IOException exception = assertThrows(IOException.class, () -> objectUnderTest.handleWeirdStringValue(
+                deserializationContext, TEST_TARGET_TYPE, TEST_VALUE, TEST_FAILURE_MESSAGE));
+        assertThat(exception.getMessage(), containsString(ClassUtil.nameOf(TEST_TARGET_TYPE)));
+        assertThat(exception.getMessage(), containsString(TEST_VALUE));
+        assertThat(exception.getMessage(), containsString(TEST_FAILURE_MESSAGE));
+    }
+
+    @Test
+    void testHandleUnexpectedToken() {
+        when(deserializationContext.getParser()).thenReturn(jsonParser);
+        final JavaType javaType = OBJECT_MAPPER.constructType(TEST_TARGET_TYPE);
+        final JsonToken jsonToken = JsonToken.END_OBJECT;
+        final JsonMappingException exception = assertThrows(JsonMappingException.class, () -> objectUnderTest.handleUnexpectedToken(
+                deserializationContext, javaType, jsonToken, jsonParser, TEST_FAILURE_MESSAGE));
+        assertThat(exception.getMessage(), containsString(ClassUtil.getTypeDescription(javaType)));
+        assertThat(exception.getMessage(), not(containsString(TEST_FAILURE_MESSAGE)));
+    }
+
+    @Test
+    void testHandleMissingInstantiator() throws IOException {
+        when(deserializationContext.getParser()).thenReturn(jsonParser);
+        when(jsonParser.getText()).thenReturn(UUID.randomUUID().toString());
+        final JsonMappingException exception = assertThrows(JsonMappingException.class, () -> objectUnderTest.handleMissingInstantiator(
+                deserializationContext, TEST_TARGET_TYPE, valueInstantiator, jsonParser, TEST_FAILURE_MESSAGE));
+        assertThat(exception.getMessage(), containsString(jsonParser.getText()));
+        assertThat(exception.getMessage(), containsString(TEST_TARGET_TYPE.getName()));
+        assertThat(exception.getMessage(), not(containsString(TEST_FAILURE_MESSAGE)));
+    }
+}
diff --git a/...c/test/java/org/opensearch/dataprepper/pipeline/parser/DataPrepperDurationParserTest.java b/...c/test/java/org/opensearch/dataprepper/pipeline/parser/DataPrepperDurationParserTest.java
@@ -17,6 +17,11 @@
 
 class DataPrepperDurationParserTest {
 
+    @Test
+    void nullValueThrowIllegalArgumentException() {
+        assertThrows(IllegalArgumentException.class, () -> DataPrepperDurationParser.parse(null));
+    }
+
     @ParameterizedTest
     @ValueSource(strings = {"6s1s", "60ms 100s", "20.345s", "-1s", "06s", "100m", "100sm", "100"})
     void invalidDurationStringsThrowIllegalArgumentException(final String durationString) {

diff --git a/...-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java b/...-framework/src/main/java/org/opensearch/dataprepper/plugin/ObjectMapperConfiguration.java
@@ -13,6 +13,7 @@
 import org.opensearch.dataprepper.model.plugin.PluginConfigVariable;
 import org.opensearch.dataprepper.model.types.ByteCount;
 import org.opensearch.dataprepper.pipeline.parser.ByteCountDeserializer;
+import org.opensearch.dataprepper.pipeline.parser.DataPrepperDeserializationProblemHandler;
 import org.opensearch.dataprepper.pipeline.parser.DataPrepperDurationDeserializer;
 import org.opensearch.dataprepper.pipeline.parser.EnumDeserializer;
 import org.opensearch.dataprepper.pipeline.parser.EventKeyDeserializer;
@@ -32,7 +33,8 @@ public class ObjectMapperConfiguration {
             Boolean.class, Character.class, PluginConfigVariable.class);
 
     @Bean(name = "extensionPluginConfigObjectMapper")
-    ObjectMapper extensionPluginConfigObjectMapper() {
+    ObjectMapper extensionPluginConfigObjectMapper(
+            final DataPrepperDeserializationProblemHandler dataPrepperDeserializationProblemHandler) {
         final SimpleModule simpleModule = new SimpleModule();
         simpleModule.addDeserializer(Duration.class, new DataPrepperDurationDeserializer());
         simpleModule.addDeserializer(Enum.class, new EnumDeserializer());
@@ -41,13 +43,15 @@ ObjectMapper extensionPluginConfigObjectMapper() {
 
         return new ObjectMapper()
                 .setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE)
-                .registerModule(simpleModule);
+                .registerModule(simpleModule)
+                .addHandler(dataPrepperDeserializationProblemHandler);
     }
 
     @Bean(name = "pluginConfigObjectMapper")
     ObjectMapper pluginConfigObjectMapper(
             final VariableExpander variableExpander,
-            final EventKeyFactory eventKeyFactory) {
+            final EventKeyFactory eventKeyFactory,
+            final DataPrepperDeserializationProblemHandler dataPrepperDeserializationProblemHandler) {
         final SimpleModule simpleModule = new SimpleModule();
         simpleModule.addDeserializer(Duration.class, new DataPrepperDurationDeserializer());
         simpleModule.addDeserializer(ByteCount.class, new ByteCountDeserializer());
@@ -58,6 +62,7 @@ ObjectMapper pluginConfigObjectMapper(
 
         return new ObjectMapper()
                 .setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE)
-                .registerModule(simpleModule);
+                .registerModule(simpleModule)
+                .addHandler(dataPrepperDeserializationProblemHandler);
     }
 }
diff --git a/...est/java/org/opensearch/dataprepper/plugin/ExtensionPluginConfigurationConverterTest.java b/...est/java/org/opensearch/dataprepper/plugin/ExtensionPluginConfigurationConverterTest.java
@@ -16,6 +16,7 @@
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.dataprepper.model.plugin.InvalidPluginConfigurationException;
+import org.opensearch.dataprepper.pipeline.parser.DataPrepperDeserializationProblemHandler;
 import org.opensearch.dataprepper.plugins.test.TestExtension;
 import org.opensearch.dataprepper.plugins.test.TestExtensionConfig;
 
@@ -43,7 +44,8 @@ class ExtensionPluginConfigurationConverterTest {
     @Mock
     private ConstraintViolation<Object> constraintViolation;
 
-    private final ObjectMapper objectMapper = new ObjectMapperConfiguration().extensionPluginConfigObjectMapper();
+    private final ObjectMapper objectMapper = new ObjectMapperConfiguration()
+            .extensionPluginConfigObjectMapper(new DataPrepperDeserializationProblemHandler());
     private ExtensionPluginConfigurationConverter objectUnderTest;
 
     @BeforeEach

diff --git a/...mework/src/test/java/org/opensearch/dataprepper/plugin/ObjectMapperConfigurationTest.java b/...mework/src/test/java/org/opensearch/dataprepper/plugin/ObjectMapperConfigurationTest.java
@@ -15,6 +15,7 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.dataprepper.model.event.EventKey;
 import org.opensearch.dataprepper.model.event.EventKeyFactory;
+import org.opensearch.dataprepper.pipeline.parser.DataPrepperDeserializationProblemHandler;
 
 import java.time.Duration;
 import java.util.Arrays;
@@ -38,18 +39,23 @@ class ObjectMapperConfigurationTest {
     @Mock
     private EventKeyFactory eventKeyFactory;
 
+    @Mock
+    private DataPrepperDeserializationProblemHandler dataPrepperDeserializationProblemHandler;
+
     @Test
     void test_duration_with_pluginConfigObjectMapper() {
         final String durationTestString = "10s";
-        final ObjectMapper objectMapper = objectMapperConfiguration.pluginConfigObjectMapper(variableExpander, eventKeyFactory);
+        final ObjectMapper objectMapper = objectMapperConfiguration.pluginConfigObjectMapper(
+                variableExpander, eventKeyFactory, dataPrepperDeserializationProblemHandler);
         final Duration duration = objectMapper.convertValue(durationTestString, Duration.class);
         assertThat(duration, equalTo(Duration.ofSeconds(10)));
     }
 
     @Test
     void test_enum_with_pluginConfigObjectMapper() throws JsonProcessingException {
         final String testModelAsString = "{ \"name\": \"my-name\", \"test_type\": \"test\" }";
-        final ObjectMapper objectMapper = objectMapperConfiguration.pluginConfigObjectMapper(variableExpander, eventKeyFactory);
+        final ObjectMapper objectMapper = objectMapperConfiguration.pluginConfigObjectMapper(
+                variableExpander, eventKeyFactory, dataPrepperDeserializationProblemHandler);
         final TestModel testModel = objectMapper.readValue(testModelAsString, TestModel.class);
         assertThat(testModel, notNullValue());
         assertThat(testModel.getTestType(), equalTo(TestType.TEST));
@@ -58,15 +64,17 @@ void test_enum_with_pluginConfigObjectMapper() throws JsonProcessingException {
     @Test
     void test_duration_with_extensionPluginConfigObjectMapper() {
         final String durationTestString = "10s";
-        final ObjectMapper objectMapper = objectMapperConfiguration.extensionPluginConfigObjectMapper();
+        final ObjectMapper objectMapper = objectMapperConfiguration.extensionPluginConfigObjectMapper(
+                dataPrepperDeserializationProblemHandler);
         final Duration duration = objectMapper.convertValue(durationTestString, Duration.class);
         assertThat(duration, equalTo(Duration.ofSeconds(10)));
     }
 
     @Test
     void test_enum_with_extensionPluginConfigObjectMapper() throws JsonProcessingException {
         final String testModelAsString = "{ \"name\": \"my-name\", \"test_type\": \"test\" }";
-        final ObjectMapper objectMapper = objectMapperConfiguration.extensionPluginConfigObjectMapper();
+        final ObjectMapper objectMapper = objectMapperConfiguration.extensionPluginConfigObjectMapper(
+                dataPrepperDeserializationProblemHandler);
         final TestModel testModel = objectMapper.readValue(testModelAsString, TestModel.class);
         assertThat(testModel, notNullValue());
         assertThat(testModel.getTestType(), equalTo(TestType.TEST));
@@ -77,7 +85,8 @@ void test_eventKey_with_pluginConfigObjectMapper() {
         final String testKey = "test";
         final EventKey eventKey = mock(EventKey.class);
         when(eventKeyFactory.createEventKey(testKey, EventKeyFactory.EventAction.ALL)).thenReturn(eventKey);
-        final ObjectMapper objectMapper = objectMapperConfiguration.pluginConfigObjectMapper(variableExpander, eventKeyFactory);
+        final ObjectMapper objectMapper = objectMapperConfiguration.pluginConfigObjectMapper(
+                variableExpander, eventKeyFactory, dataPrepperDeserializationProblemHandler);
         final EventKey actualEventKey = objectMapper.convertValue(testKey, EventKey.class);
         assertThat(actualEventKey, equalTo(eventKey));
     }

diff --git a/...lugin-framework/src/test/java/org/opensearch/dataprepper/plugin/VariableExpanderTest.java b/...lugin-framework/src/test/java/org/opensearch/dataprepper/plugin/VariableExpanderTest.java
@@ -20,6 +20,7 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.dataprepper.model.plugin.PluginConfigValueTranslator;
 import org.opensearch.dataprepper.model.plugin.PluginConfigVariable;
+import org.opensearch.dataprepper.pipeline.parser.DataPrepperDeserializationProblemHandler;
 
 import java.io.IOException;
 import java.math.BigDecimal;
@@ -38,7 +39,8 @@
 
 @ExtendWith(MockitoExtension.class)
 class VariableExpanderTest {
-    static final ObjectMapper OBJECT_MAPPER = new ObjectMapperConfiguration().extensionPluginConfigObjectMapper();
+    static final ObjectMapper OBJECT_MAPPER = new ObjectMapperConfiguration()
+            .extensionPluginConfigObjectMapper(new DataPrepperDeserializationProblemHandler());
     static final JsonFactory JSON_FACTORY = new MappingJsonFactory();
 
     @Mock