Skip to content

Commit 4cee36f

Browse files
Davidding4718Davidding4718
committed
Add custom Jackson deserializer to handle empty plugin configs and reject empty strings
Signed-off-by: Siqi Ding <dingdd@amazon.com>
1 parent 6eb06fe commit 4cee36f

18 files changed

+336
-38
lines changed

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

Lines changed: 63 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,10 @@
1212
import com.fasterxml.jackson.annotation.JsonAnyGetter;
1313
import com.fasterxml.jackson.annotation.JsonAnySetter;
1414
import com.fasterxml.jackson.annotation.JsonCreator;
15-
import com.fasterxml.jackson.core.JacksonException;
1615
import com.fasterxml.jackson.core.JsonGenerator;
1716
import com.fasterxml.jackson.core.JsonParser;
17+
import com.fasterxml.jackson.core.JsonToken;
1818
import com.fasterxml.jackson.databind.DeserializationContext;
19-
import com.fasterxml.jackson.databind.JsonNode;
2019
import com.fasterxml.jackson.databind.ObjectMapper;
2120
import com.fasterxml.jackson.databind.SerializerProvider;
2221
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
@@ -26,11 +25,9 @@
2625

2726
import java.io.IOException;
2827
import java.util.HashMap;
29-
import java.util.Iterator;
3028
import java.util.Map;
3129
import java.util.Objects;
3230
import java.util.function.BiFunction;
33-
import java.util.function.Supplier;
3431

3532
/**
3633
* Model class for a Plugin in Configuration YAML containing name of the Plugin and its associated settings
@@ -41,7 +38,13 @@
4138
@JsonDeserialize(using = PluginModel.PluginModelDeserializer.class)
4239
public class PluginModel {
4340

44-
private static final ObjectMapper SERIALIZER_OBJECT_MAPPER = new ObjectMapper();
41+
private static final ObjectMapper SERIALIZER_OBJECT_MAPPER;
42+
43+
static {
44+
SERIALIZER_OBJECT_MAPPER = new ObjectMapper();
45+
// Note: We don't configure coercion here because our custom deserializer
46+
// handles all the cases (null, empty, {}, and rejects empty strings)
47+
}
4548

4649
private final String pluginName;
4750
private final InternalJsonModel innerModel;
@@ -117,10 +120,28 @@ public PluginModelSerializer(final Class<PluginModel> valueClass) {
117120
public void serialize(
118121
final PluginModel value, final JsonGenerator gen, final SerializerProvider provider) throws IOException {
119122
gen.writeStartObject();
120-
Map<String, Object> serializedInner = SERIALIZER_OBJECT_MAPPER.convertValue(value.innerModel, Map.class);
121-
if(serializedInner != null && serializedInner.isEmpty())
122-
serializedInner = null;
123-
gen.writeObjectField(value.getPluginName(), serializedInner);
123+
124+
// Serialize the inner model to JSON string then back to Map
125+
// This properly respects all Jackson annotations including @JsonInclude on subclasses
126+
String jsonString = SERIALIZER_OBJECT_MAPPER.writeValueAsString(value.innerModel);
127+
Map<String, Object> serializedInner = SERIALIZER_OBJECT_MAPPER.readValue(jsonString, Map.class);
128+
129+
if (serializedInner.isEmpty()) {
130+
// Inner model has no content - check if pluginSettings was explicitly null
131+
// to decide between null and {}
132+
if (value.innerModel.pluginSettings == null) {
133+
// Explicitly null settings -> serialize as null
134+
gen.writeObjectField(value.getPluginName(), null);
135+
} else {
136+
// Empty (non-null) settings -> serialize as {}
137+
gen.writeFieldName(value.getPluginName());
138+
gen.writeStartObject();
139+
gen.writeEndObject();
140+
}
141+
} else {
142+
// Inner model has content (plugin settings or subclass fields like routes)
143+
gen.writeObjectField(value.getPluginName(), serializedInner);
144+
}
124145
gen.writeEndObject();
125146
}
126147
}
@@ -136,7 +157,7 @@ public void serialize(
136157
static final class PluginModelDeserializer extends AbstractPluginModelDeserializer<PluginModel, InternalJsonModel> {
137158

138159
public PluginModelDeserializer() {
139-
super(PluginModel.class, InternalJsonModel.class, PluginModel::new, InternalJsonModel::new);
160+
super(PluginModel.class, InternalJsonModel.class, PluginModel::new);
140161
}
141162
}
142163

@@ -156,32 +177,48 @@ abstract static class AbstractPluginModelDeserializer<T extends PluginModel, M e
156177

157178
private final Class<M> innerModelClass;
158179
private final BiFunction<String, M, T> constructorFunction;
159-
private final Supplier<M> emptyInnerModelConstructor;
160180

161181
protected AbstractPluginModelDeserializer(
162182
final Class<T> valueClass,
163183
final Class<M> innerModelClass,
164-
final BiFunction<String, M, T> constructorFunction,
165-
final Supplier<M> emptyInnerModelConstructor) {
184+
final BiFunction<String, M, T> constructorFunction) {
166185
super(valueClass);
167186
this.innerModelClass = innerModelClass;
168187
this.constructorFunction = constructorFunction;
169-
this.emptyInnerModelConstructor = emptyInnerModelConstructor;
170188
}
171189

172190
@Override
173-
public PluginModel deserialize(final JsonParser jsonParser, final DeserializationContext context) throws IOException, JacksonException {
174-
final JsonNode node = jsonParser.getCodec().readTree(jsonParser);
175-
176-
final Iterator<Map.Entry<String, JsonNode>> fields = node.fields();
177-
final Map.Entry<String, JsonNode> onlyField = fields.next();
178-
179-
final String pluginName = onlyField.getKey();
180-
final JsonNode value = onlyField.getValue();
181-
182-
M innerModel = SERIALIZER_OBJECT_MAPPER.convertValue(value, innerModelClass);
183-
if(innerModel == null)
184-
innerModel = emptyInnerModelConstructor.get();
191+
public PluginModel deserialize(final JsonParser jsonParser, final DeserializationContext context) throws IOException {
192+
ObjectMapper mapper = (ObjectMapper) jsonParser.getCodec();
193+
194+
jsonParser.nextToken();
195+
196+
final String pluginName = jsonParser.currentName();
197+
jsonParser.nextToken();
198+
199+
Map<String, Object> data = new HashMap<>();
200+
if (jsonParser.currentToken() == JsonToken.START_OBJECT) {
201+
data = mapper.readValue(jsonParser, Map.class);
202+
} else if (jsonParser.currentToken() == JsonToken.VALUE_NULL) {
203+
// null value -> treat as empty object (acceptable format)
204+
data = new HashMap<>();
205+
} else if (jsonParser.currentToken() == JsonToken.VALUE_STRING) {
206+
String value = jsonParser.getValueAsString();
207+
// Empty string "" is NOT allowed - throw exception
208+
// Any other string value is also not allowed
209+
if (value.isEmpty()) {
210+
throw context.weirdStringException(value, Map.class,
211+
"Empty string is not allowed for plugin '" + pluginName + "'. Use null, empty (no value), or {} instead.");
212+
} else {
213+
throw context.weirdStringException(value, Map.class,
214+
"String values not allowed for plugin '" + pluginName + "'");
215+
}
216+
}
217+
while (jsonParser.currentToken() != JsonToken.END_OBJECT) {
218+
jsonParser.nextToken();
219+
}
220+
221+
final M innerModel = SERIALIZER_OBJECT_MAPPER.convertValue(data, innerModelClass);
185222

186223
return constructorFunction.apply(pluginName, innerModel);
187224
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ private static List<String> validateKeys(List<String> input, String tag) {
164164

165165
static class SinkModelDeserializer extends AbstractPluginModelDeserializer<SinkModel, SinkInternalJsonModel> {
166166
SinkModelDeserializer() {
167-
super(SinkModel.class, SinkInternalJsonModel.class, SinkModel::new, () -> new SinkInternalJsonModel(null, null, null, null, null));
167+
super(SinkModel.class, SinkInternalJsonModel.class, SinkModel::new);
168168
}
169169
}
170170
}

‎data-prepper-api/src/test/java/org/opensearch/dataprepper/model/configuration/PluginModelTests.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,85 @@ final void testUsingCustomDeserializer_with_array() throws JsonParseException, J
141141
assertThat(readValue.listOfPlugins.get(1).getPluginSettings().get("key2"), equalTo("value2"));
142142
}
143143

144+
@Test
145+
final void testRoundTrip_withEmptyObject() throws IOException {
146+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_empty_object.yaml");
147+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory().enable(YAMLGenerator.Feature.USE_PLATFORM_LINE_BREAKS));
148+
149+
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
150+
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
151+
assertThat(pluginModel1.getPluginSettings().size(), equalTo(0));
152+
153+
final String serialized = mapper.writeValueAsString(pluginModel1);
154+
assertThat(serialized.contains("{}"), equalTo(true));
155+
156+
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
157+
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
158+
assertThat(pluginModel2.getPluginSettings().size(), equalTo(0));
159+
}
160+
161+
@Test
162+
final void testRoundTrip_withNullValue() throws IOException {
163+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_null.yaml");
164+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory().enable(YAMLGenerator.Feature.USE_PLATFORM_LINE_BREAKS));
165+
166+
// null input -> deserializes to empty settings
167+
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
168+
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
169+
assertThat(pluginModel1.getPluginSettings().size(), equalTo(0));
170+
171+
// empty settings -> serializes as {}
172+
final String serialized = mapper.writeValueAsString(pluginModel1);
173+
assertThat(serialized.contains("{}"), equalTo(true));
174+
175+
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
176+
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
177+
assertThat(pluginModel2.getPluginSettings().size(), equalTo(0));
178+
}
179+
180+
@Test
181+
final void testRoundTrip_withEmptyValue() throws IOException {
182+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_empty_value.yaml");
183+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory().enable(YAMLGenerator.Feature.USE_PLATFORM_LINE_BREAKS));
184+
185+
// empty value (no value after colon) -> deserializes to empty settings
186+
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
187+
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
188+
assertThat(pluginModel1.getPluginSettings().size(), equalTo(0));
189+
190+
// empty settings -> serializes as {}
191+
final String serialized = mapper.writeValueAsString(pluginModel1);
192+
assertThat(serialized.contains("{}"), equalTo(true));
193+
194+
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
195+
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
196+
assertThat(pluginModel2.getPluginSettings().size(), equalTo(0));
197+
}
198+
199+
@Test
200+
final void testDeserialize_emptyString_throwsException() throws IOException {
201+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_empty_string.yaml");
202+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
203+
204+
final JsonMappingException exception = org.junit.jupiter.api.Assertions.assertThrows(
205+
JsonMappingException.class,
206+
() -> mapper.readValue(inputStream, PluginModel.class)
207+
);
208+
assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("Empty string is not allowed"));
209+
}
210+
211+
@Test
212+
final void testDeserialize_nonEmptyString_throwsException() throws IOException {
213+
final String yaml = "customPlugin: someStringValue";
214+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
215+
216+
final JsonMappingException exception = org.junit.jupiter.api.Assertions.assertThrows(
217+
JsonMappingException.class,
218+
() -> mapper.readValue(yaml, PluginModel.class)
219+
);
220+
assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("String values not allowed"));
221+
}
222+
144223
static Map<String, Object> validPluginSettings() {
145224
final Map<String, Object> settings = new HashMap<>();
146225
settings.put("key1", "value1");

‎data-prepper-api/src/test/java/org/opensearch/dataprepper/model/configuration/SamplePipelineConfigurationTest.java‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*
5+
* The OpenSearch Contributors require contributions made to
6+
* this file be licensed under the Apache-2.0 license or a
7+
* compatible open source license.
8+
*/
9+
10+
package org.opensearch.dataprepper.model.configuration;
11+
12+
import com.fasterxml.jackson.databind.JsonMappingException;
13+
import com.fasterxml.jackson.databind.ObjectMapper;
14+
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
15+
import org.junit.jupiter.api.BeforeEach;
16+
import org.junit.jupiter.params.ParameterizedTest;
17+
import org.junit.jupiter.params.provider.ValueSource;
18+
import org.junit.jupiter.api.Test;
19+
20+
import java.io.IOException;
21+
import java.io.InputStream;
22+
23+
import static org.hamcrest.CoreMatchers.equalTo;
24+
import static org.hamcrest.CoreMatchers.notNullValue;
25+
import static org.hamcrest.MatcherAssert.assertThat;
26+
import static org.junit.jupiter.api.Assertions.assertThrows;
27+
28+
/**
29+
* End-to-end tests for sample pipeline YAML configurations, verifying that
30+
* valid plugin config formats are accepted and invalid ones are rejected.
31+
*
32+
* Valid formats: null, empty value (no value after colon), {}
33+
* Invalid format: "" (empty string)
34+
*/
35+
class SamplePipelineConfigurationTest {
36+
37+
private static final String PIPELINE_NAME = "test-pipeline";
38+
39+
private ObjectMapper objectMapper;
40+
41+
@BeforeEach
42+
void setup() {
43+
objectMapper = new ObjectMapper(new YAMLFactory());
44+
}
45+
46+
// --- Valid pipeline scenarios ---
47+
48+
@ParameterizedTest(name = "pipeline with plugin config [{0}] should deserialize successfully")
49+
@ValueSource(strings = {
50+
"sample_pipelines/sample_pipeline_plugin_null.yaml",
51+
"sample_pipelines/sample_pipeline_plugin_empty_value.yaml",
52+
"sample_pipelines/sample_pipeline_plugin_empty_object.yaml",
53+
"sample_pipelines/sample_pipeline_plugin_with_settings.yaml"
54+
})
55+
void deserialize_validPipeline_succeeds(final String resourcePath) throws IOException {
56+
final InputStream inputStream = getClass().getResourceAsStream(resourcePath);
57+
58+
final PipelinesDataFlowModel model = objectMapper.readValue(inputStream, PipelinesDataFlowModel.class);
59+
60+
assertThat(model, notNullValue());
61+
assertThat(model.getPipelines(), notNullValue());
62+
assertThat(model.getPipelines().containsKey(PIPELINE_NAME), equalTo(true));
63+
64+
final PipelineModel pipeline = model.getPipelines().get(PIPELINE_NAME);
65+
assertThat(pipeline.getSource(), notNullValue());
66+
assertThat(pipeline.getSource().getPluginName(), notNullValue());
67+
assertThat(pipeline.getSinks(), notNullValue());
68+
assertThat(pipeline.getSinks().size(), equalTo(1));
69+
}
70+
71+
@Test
72+
void deserialize_pipeline_withPluginSettings_hasCorrectValues() throws IOException {
73+
final InputStream inputStream = getClass().getResourceAsStream("sample_pipelines/sample_pipeline_plugin_with_settings.yaml");
74+
75+
final PipelinesDataFlowModel model = objectMapper.readValue(inputStream, PipelinesDataFlowModel.class);
76+
final PipelineModel pipeline = model.getPipelines().get(PIPELINE_NAME);
77+
78+
// source: http with host/port settings
79+
assertThat(pipeline.getSource().getPluginName(), equalTo("http"));
80+
assertThat(pipeline.getSource().getPluginSettings().get("host"), equalTo("0.0.0.0"));
81+
assertThat(pipeline.getSource().getPluginSettings().get("port"), equalTo(2021));
82+
83+
// processor: grok
84+
assertThat(pipeline.getProcessors().get(0).getPluginName(), equalTo("grok"));
85+
86+
// sink: opensearch with hosts/credentials
87+
assertThat(pipeline.getSinks().get(0).getPluginName(), equalTo("opensearch"));
88+
assertThat(pipeline.getSinks().get(0).getPluginSettings().containsKey("hosts"), equalTo(true));
89+
}
90+
91+
@ParameterizedTest(name = "pipeline with null/empty plugin [{0}] should have empty plugin settings")
92+
@ValueSource(strings = {
93+
"sample_pipelines/sample_pipeline_plugin_null.yaml",
94+
"sample_pipelines/sample_pipeline_plugin_empty_value.yaml",
95+
"sample_pipelines/sample_pipeline_plugin_empty_object.yaml"
96+
})
97+
void deserialize_pipeline_withEmptyPluginConfig_hasEmptySettings(final String resourcePath) throws IOException {
98+
final InputStream inputStream = getClass().getResourceAsStream(resourcePath);
99+
100+
final PipelinesDataFlowModel model = objectMapper.readValue(inputStream, PipelinesDataFlowModel.class);
101+
final PipelineModel pipeline = model.getPipelines().get(PIPELINE_NAME);
102+
103+
assertThat(pipeline.getSource().getPluginSettings().size(), equalTo(0));
104+
assertThat(pipeline.getProcessors().get(0).getPluginSettings().size(), equalTo(0));
105+
assertThat(pipeline.getSinks().get(0).getPluginSettings().size(), equalTo(0));
106+
}
107+
108+
// --- Invalid pipeline scenario ---
109+
110+
@Test
111+
void deserialize_pipeline_withEmptyStringPluginConfig_throwsException() {
112+
final InputStream inputStream = getClass().getResourceAsStream("sample_pipelines/sample_pipeline_plugin_empty_string.yaml");
113+
114+
final JsonMappingException exception = assertThrows(
115+
JsonMappingException.class,
116+
() -> objectMapper.readValue(inputStream, PipelinesDataFlowModel.class)
117+
);
118+
assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("Empty string is not allowed"));
119+
}
120+
}

‎data-prepper-api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_empty_string.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
---
2+
customPlugin: ""

‎data-prepper-api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_not_present.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
---
2-
customPlugin:
2+
customPlugin: {}

‎data-prepper-api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_null.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
---
2-
customPlugin: null
2+
customPlugin: {}

‎data-prepper-api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_with_empty_object.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
---
2+
customPlugin: {}

‎data-prepper-api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_with_empty_value.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
---
2+
customPlugin:

‎data-prepper-api/src/test/resources/org/opensearch/dataprepper/model/configuration/plugin_model_with_null.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
---
2+
customPlugin: null

0 commit comments

Comments
 (0)
⚡