Add custom Jackson deserializer to handle empty plugin configs and re… by Davidding4718 · Pull Request #6598 · opensearch-project/data-prepper

Davidding4718 · 2026-03-03T00:07:08Z

Description

After upgrading Jackson, empty YAML plugin configurations like stdout: were being deserialized as empty strings "" instead of null/empty objects, causing MismatchedInputException. This change adds a custom Jackson deserializer to PluginModel that explicitly handles the valid formats (null, empty value, {}) and rejects empty strings "" with a clear error message. The custom serializer is also updated to output {} for empty (non-null) plugin settings.

Issues Resolved

NULL

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…ject empty strings Signed-off-by: Siqi Ding <dingdd@amazon.com>

oeyh

Thanks for working on this! A few comments below:

oeyh · 2026-03-17T20:31:35Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/configuration/PluginModel.java

+    static {
+        SERIALIZER_OBJECT_MAPPER = new ObjectMapper();
+        // Note: We don't configure coercion here because our custom deserializer
+        // handles all the cases (null, empty, {}, and rejects empty strings)
+    }


Does this static block change anything from the original:

private static final ObjectMapper SERIALIZER_OBJECT_MAPPER = new ObjectMapper();

Yea, revert this small section.

I think you are working on code that I originally experimented with. I once had some configurations on the ObjectMapper that are no longer relevant.

oeyh · 2026-03-17T20:35:36Z

...epper-api/src/test/java/org/opensearch/dataprepper/model/configuration/PluginModelTests.java

+            JsonMappingException.class,
+            () -> mapper.readValue(inputStream, PluginModel.class)
+        );
+        assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("Empty string is not allowed"));


Use static import instead.

oeyh · 2026-03-17T20:35:44Z

...epper-api/src/test/java/org/opensearch/dataprepper/model/configuration/PluginModelTests.java

+            JsonMappingException.class,
+            () -> mapper.readValue(yaml, PluginModel.class)
+        );
+        assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("String values not allowed"));


Use static import instead.

oeyh · 2026-03-17T20:35:53Z

...est/java/org/opensearch/dataprepper/model/configuration/SamplePipelineConfigurationTest.java

+            JsonMappingException.class,
+            () -> objectMapper.readValue(inputStream, PipelinesDataFlowModel.class)
+        );
+        assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("Empty string is not allowed"));


oeyh · 2026-03-17T20:37:47Z

.../test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_not_present.yaml

@@ -1,2 +1,2 @@
 ---
-customPlugin:
+customPlugin: {}


Should we add new tests with {}, instead of changing the previous test? Otherwise, null case is no longer tested.
Similar for other tests.

Yea, I think we should keep this file the way it was.

dlvenable

Thank you @Davidding4718 !

dlvenable · 2026-03-18T02:33:56Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/configuration/PluginModel.java

+    static {
+        SERIALIZER_OBJECT_MAPPER = new ObjectMapper();
+        // Note: We don't configure coercion here because our custom deserializer
+        // handles all the cases (null, empty, {}, and rejects empty strings)
+    }


Yea, revert this small section.

I think you are working on code that I originally experimented with. I once had some configurations on the ObjectMapper that are no longer relevant.

dlvenable · 2026-03-18T02:34:55Z

...epper-api/src/test/java/org/opensearch/dataprepper/model/configuration/PluginModelTests.java

+        final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_empty_string.yaml");
+        final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
+
+        final JsonMappingException exception = org.junit.jupiter.api.Assertions.assertThrows(


Another place to use a static import.

dlvenable · 2026-03-18T02:35:56Z

...est/java/org/opensearch/dataprepper/model/configuration/SamplePipelineConfigurationTest.java

+        objectMapper = new ObjectMapper(new YAMLFactory());
+    }
+
+    // --- Valid pipeline scenarios ---


Please don't have sections like this. They don't remain over time. There are two approaches:

You can use @Nested to group them, but I tend to avoid @Nested unless there is common setup.

Just remove this comment.

Thanks! Will remove it in new commit

dlvenable · 2026-03-18T02:36:24Z

...est/java/org/opensearch/dataprepper/model/configuration/SamplePipelineConfigurationTest.java

+        assertThat(pipeline.getSinks().get(0).getPluginSettings().size(), equalTo(0));
+    }
+
+    // --- Invalid pipeline scenario ---


Again, handle this comment.

Thanks! Will remove it in new commit

dlvenable · 2026-03-18T02:37:49Z

.../test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_not_present.yaml

@@ -1,2 +1,2 @@
 ---
-customPlugin:
+customPlugin: {}


Yea, I think we should keep this file the way it was.

dlvenable · 2026-03-18T02:40:31Z

data-prepper-api/src/test/resources/pipeline_with_extension.yaml

 test-pipeline:
  source:
-    testSource: null
+    testSource: {}


Do we need to modify this file?

I think the only scenario where exiting inputs will no longer pass is when we do a deserialize followed by a serialize.

Maybe a better way to test this would be to perform another deserialize and then compare the maps?

dlvenable · 2026-03-18T02:41:43Z

...epper-api/src/test/java/org/opensearch/dataprepper/model/configuration/PluginModelTests.java

+    @Test
+    final void testRoundTrip_withNullValue() throws IOException {
+        final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_null.yaml");
+        final ObjectMapper mapper = new ObjectMapper(new YAMLFactory().enable(YAMLGenerator.Feature.USE_PLATFORM_LINE_BREAKS));


Do we need this feature enabled? enable(YAMLGenerator.Feature.USE_PLATFORM_LINE_BREAKS)

github-actions · 2026-03-24T20:12:05Z

✅ License Header Check Passed

All newly added files have proper license headers. Great work! 🎉

oeyh · 2026-03-27T16:09:03Z

...api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_null.yaml

@@ -1,2 +1,2 @@
 ---
-customPlugin: null
+customPlugin: {}


This should be null. There's another file below for {}.

oeyh · 2026-03-27T16:16:21Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/configuration/PluginModel.java

+                    throw context.weirdStringException(value, Map.class,
+                            "String values not allowed for plugin '" + pluginName + "'");
+                }
+            }


No else branch here. Can you check what will happen if jsonParser.currentToken() is not START_OBJECT or VALUE_NULL or VALUE_STRING. For example,:

test-pipeline: source: stdin: 123 # or true/false, or array [1, 2] processor: - uppercase_string: null sink: - stdout: null

Good point! Will add a else condition with following unit test.

oeyh · 2026-03-27T16:24:06Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/configuration/PluginModel.java

-            gen.writeObjectField(value.getPluginName(), serializedInner);
+
+            final String jsonString = SERIALIZER_OBJECT_MAPPER.writeValueAsString(value.innerModel);
+            final Map<String, Object> serializedInner = SERIALIZER_OBJECT_MAPPER.readValue(jsonString, Map.class);


Can you clarify why this serialization and immediate deserialization is necessary here and add a comment above the code?

Added a comment in the code. The round-trip is needed because innerModel can be a subclass (e.g. SinkInternalJsonModel) with extra Jackson-annotated fields. Serializing to JSON and reading back as a Map lets Jackson collect all those fields together, so we can check if the result is empty and write it out correctly. Directly accessing pluginSettings would miss any extra fields defined on subclasses.

Signed-off-by: Siqi Ding <dingdd@amazon.com>

Davidding4718 requested review from KarstenSchnitter, dinujoh, dlvenable, engechas, graytaylor0, kkondaka, oeyh, san81, sb2k16 and srikanthjg as code owners March 3, 2026 00:07

Add custom Jackson deserializer to handle empty plugin configs and re…

4cee36f

…ject empty strings Signed-off-by: Siqi Ding <dingdd@amazon.com>

Davidding4718 force-pushed the jackson-update-serailization branch from 1427075 to 4cee36f Compare March 3, 2026 16:17

Davidding4718 marked this pull request as draft March 3, 2026 19:20

Davidding4718 marked this pull request as ready for review March 3, 2026 20:43

dlvenable added this to the v2.15 milestone Mar 3, 2026

Davidding4718 force-pushed the jackson-update-serailization branch 2 times, most recently from 5336805 to 2792906 Compare March 10, 2026 17:35

oeyh reviewed Mar 17, 2026

View reviewed changes

dlvenable requested changes Mar 18, 2026

View reviewed changes

Davidding4718 force-pushed the jackson-update-serailization branch from 2792906 to 9fd97de Compare March 24, 2026 20:08

oeyh reviewed Mar 27, 2026

View reviewed changes

fix: update Jackson BOM to 2.20.2 and fix logstash expected YAML

ff519f6

Signed-off-by: Siqi Ding <dingdd@amazon.com>

Davidding4718 force-pushed the jackson-update-serailization branch from 9fd97de to ff519f6 Compare March 27, 2026 20:39

fix: update Jackson BOM to 2.20.2 and fix logstash expected YAML

846416a

Signed-off-by: Siqi Ding <dingdd@amazon.com>

Davidding4718 force-pushed the jackson-update-serailization branch from e81d6c8 to 846416a Compare March 27, 2026 21:35

Conversation

Davidding4718 commented Mar 3, 2026

Description

Issues Resolved

Check List

Uh oh!

oeyh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlvenable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ License Header Check Passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 24, 2026 •

edited

Loading