Skip to content

Commit 73b809a

Browse files
Davidding4718Davidding4718
authored
Add custom Jackson deserializer to handle empty plugin configs and reject empty strings (#6598)
Signed-off-by: Siqi Ding <dingdd@amazon.com>
1 parent fa7557d commit 73b809a

22 files changed

+538
-33
lines changed

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

Lines changed: 70 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@
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;
19+
import com.fasterxml.jackson.databind.JsonMappingException;
2020
import com.fasterxml.jackson.databind.ObjectMapper;
2121
import com.fasterxml.jackson.databind.SerializerProvider;
2222
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
@@ -26,7 +26,6 @@
2626

2727
import java.io.IOException;
2828
import java.util.HashMap;
29-
import java.util.Iterator;
3029
import java.util.Map;
3130
import java.util.Objects;
3231
import java.util.function.BiFunction;
@@ -62,7 +61,6 @@ public class PluginModel {
6261
*/
6362
static class InternalJsonModel {
6463
@JsonAnySetter
65-
@JsonAnyGetter
6664
private final Map<String, Object> pluginSettings;
6765

6866
@JsonCreator
@@ -73,6 +71,11 @@ static class InternalJsonModel {
7371
InternalJsonModel(final Map<String, Object> pluginSettings) {
7472
this.pluginSettings = pluginSettings;
7573
}
74+
75+
@JsonAnyGetter
76+
Map<String, Object> getPluginSettingsForSerialization() {
77+
return pluginSettings != null ? pluginSettings : new HashMap<>();
78+
}
7679
}
7780

7881
public PluginModel(final String pluginName, final Map<String, Object> pluginSettings) {
@@ -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 innerModel to a Map via a JSON round-trip so that all Jackson-annotated
125+
// fields on potential subclasses of InternalJsonModel (e.g. SinkInternalJsonModel with
126+
// routes, tagsTargetKey, etc.) are included. Directly reading pluginSettings would miss
127+
// those extra fields. The resulting Map is then inspected to distinguish between a
128+
// truly empty/null inner model and one that has actual content to write.
129+
final String jsonString = SERIALIZER_OBJECT_MAPPER.writeValueAsString(value.innerModel);
130+
final Map<String, Object> serializedInner = SERIALIZER_OBJECT_MAPPER.readValue(jsonString, Map.class);
131+
132+
if (serializedInner.isEmpty()) {
133+
// Empty inner model: output null if pluginSettings was null, {} if it was an empty map
134+
if (value.innerModel.pluginSettings == null) {
135+
gen.writeObjectField(value.getPluginName(), null);
136+
} else {
137+
gen.writeFieldName(value.getPluginName());
138+
gen.writeStartObject();
139+
gen.writeEndObject();
140+
}
141+
} else {
142+
gen.writeObjectField(value.getPluginName(), serializedInner);
143+
}
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, () -> new InternalJsonModel(null));
140161
}
141162
}
142163

@@ -156,33 +177,57 @@ 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;
180+
private final Supplier<M> nullSettingsModelSupplier;
160181

161182
protected AbstractPluginModelDeserializer(
162183
final Class<T> valueClass,
163184
final Class<M> innerModelClass,
164185
final BiFunction<String, M, T> constructorFunction,
165-
final Supplier<M> emptyInnerModelConstructor) {
186+
final Supplier<M> nullSettingsModelSupplier) {
166187
super(valueClass);
167188
this.innerModelClass = innerModelClass;
168189
this.constructorFunction = constructorFunction;
169-
this.emptyInnerModelConstructor = emptyInnerModelConstructor;
190+
this.nullSettingsModelSupplier = nullSettingsModelSupplier;
170191
}
171192

172193
@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();
185-
194+
public PluginModel deserialize(final JsonParser jsonParser, final DeserializationContext context) throws IOException {
195+
final ObjectMapper mapper = (ObjectMapper) jsonParser.getCodec();
196+
197+
jsonParser.nextToken();
198+
199+
final String pluginName = jsonParser.currentName();
200+
jsonParser.nextToken();
201+
202+
boolean isNull = false;
203+
Map<String, Object> data = null;
204+
if (jsonParser.currentToken() == JsonToken.START_OBJECT) {
205+
data = mapper.readValue(jsonParser, Map.class);
206+
// readValue consumed up to the inner END_OBJECT; advance to the outer END_OBJECT
207+
jsonParser.nextToken();
208+
} else if (jsonParser.currentToken() == JsonToken.VALUE_NULL) {
209+
// null or no-value (stdout: null / stdout:) -> preserve as null settings
210+
isNull = true;
211+
// advance to the outer END_OBJECT
212+
jsonParser.nextToken();
213+
} else if (jsonParser.currentToken() == JsonToken.VALUE_STRING) {
214+
final String value = jsonParser.getValueAsString();
215+
if (value.isEmpty()) {
216+
throw context.weirdStringException(value, Map.class,
217+
"Empty string is not allowed for plugin '" + pluginName + "'. Use null, empty (no value), or {} instead.");
218+
} else {
219+
throw context.weirdStringException(value, Map.class,
220+
"String values not allowed for plugin '" + pluginName + "'");
221+
}
222+
} else {
223+
throw JsonMappingException.from(jsonParser,
224+
"Unexpected value for plugin '" + pluginName + "': expected an object, null, or no value, but got " +
225+
jsonParser.currentToken());
226+
}
227+
228+
final M innerModel = isNull
229+
? nullSettingsModelSupplier.get()
230+
: SERIALIZER_OBJECT_MAPPER.convertValue(data, innerModelClass);
186231
return constructorFunction.apply(pluginName, innerModel);
187232
}
188233
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,8 @@ 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,
168+
() -> new SinkInternalJsonModel(null, null, null, null, null, null));
168169
}
169170
}
170171
}

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

Lines changed: 135 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,17 @@
2626
import java.io.Reader;
2727
import java.nio.charset.Charset;
2828
import java.nio.charset.StandardCharsets;
29+
import java.util.Arrays;
2930
import java.util.HashMap;
3031
import java.util.List;
3132
import java.util.Map;
3233

3334
import static org.hamcrest.CoreMatchers.equalTo;
3435
import static org.hamcrest.CoreMatchers.notNullValue;
3536
import static org.hamcrest.MatcherAssert.assertThat;
37+
import static org.hamcrest.Matchers.containsString;
3638
import static org.hamcrest.Matchers.hasKey;
39+
import static org.junit.jupiter.api.Assertions.assertThrows;
3740

3841
class PluginModelTests {
3942

@@ -75,14 +78,14 @@ final void testSerialization_empty_plugin_to_YAML() throws JsonGenerationExcepti
7578

7679
final String serialized = mapper.writeValueAsString(pluginModel);
7780

78-
InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_null.yaml");
81+
InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_empty_object.yaml");
7982

8083
assertThat(serialized, notNullValue());
81-
assertThat(serialized, equalTo(convertInputStreamToString(inputStream)));
84+
assertThat(serialized, equalTo(stripComments(convertInputStreamToString(inputStream))));
8285
}
8386

8487
@ParameterizedTest
85-
@ValueSource(strings = {"plugin_model_empty.yaml", "plugin_model_not_present.yaml", "plugin_model_null.yaml"})
88+
@ValueSource(strings = {"plugin_model_with_empty_object.yaml"})
8689
final void deserialize_with_empty_inner(final String resourceName) throws IOException {
8790
final InputStream inputStream = PluginModelTests.class.getResourceAsStream(resourceName);
8891

@@ -94,6 +97,17 @@ final void deserialize_with_empty_inner(final String resourceName) throws IOExce
9497
assertThat(pluginModel.getPluginSettings().size(), equalTo(0));
9598
}
9699

100+
@Test
101+
final void deserialize_with_no_value_returns_null_settings() throws IOException {
102+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_not_present.yaml");
103+
104+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
105+
106+
final PluginModel pluginModel = mapper.readValue(inputStream, PluginModel.class);
107+
assertThat(pluginModel.getPluginName(), equalTo("customPlugin"));
108+
assertThat(pluginModel.getPluginSettings(), equalTo(null));
109+
}
110+
97111
@Test
98112
final void testUsingCustomSerializerWithPluginSettings_noExceptions() throws JsonGenerationException, JsonMappingException, IOException {
99113
final PluginModel pluginModel = new PluginModel("customPlugin", validPluginSettings());
@@ -141,6 +155,116 @@ final void testUsingCustomDeserializer_with_array() throws JsonParseException, J
141155
assertThat(readValue.listOfPlugins.get(1).getPluginSettings().get("key2"), equalTo("value2"));
142156
}
143157

158+
@Test
159+
final void testUsingCustomDeserializer_with_array_of_three_preserves_all_entries() throws JsonParseException, JsonMappingException, IOException {
160+
InputStream inputStream = PluginModelTests.class.getResourceAsStream("/list_of_plugins_multiple_fields.yaml");
161+
162+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
163+
164+
final PluginHolder readValue = mapper.readValue(inputStream, PluginHolder.class);
165+
assertThat(readValue, notNullValue());
166+
assertThat(readValue.listOfPlugins, notNullValue());
167+
assertThat(readValue.listOfPlugins.size(), equalTo(3));
168+
assertThat(readValue.listOfPlugins.get(0).getPluginName(), equalTo("customPluginA"));
169+
assertThat(readValue.listOfPlugins.get(0).getPluginSettings().get("key1"), equalTo("value1"));
170+
assertThat(readValue.listOfPlugins.get(1).getPluginName(), equalTo("customPluginB"));
171+
assertThat(readValue.listOfPlugins.get(1).getPluginSettings().get("key2"), equalTo("value2"));
172+
assertThat(readValue.listOfPlugins.get(2).getPluginName(), equalTo("customPluginC"));
173+
assertThat(readValue.listOfPlugins.get(2).getPluginSettings().get("key3"), equalTo("value3"));
174+
}
175+
176+
@Test
177+
final void testRoundTrip_withEmptyObject() throws IOException {
178+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_empty_object.yaml");
179+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
180+
181+
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
182+
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
183+
assertThat(pluginModel1.getPluginSettings().size(), equalTo(0));
184+
185+
final String serialized = mapper.writeValueAsString(pluginModel1);
186+
assertThat(serialized.contains("{}"), equalTo(true));
187+
188+
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
189+
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
190+
assertThat(pluginModel2.getPluginSettings().size(), equalTo(0));
191+
}
192+
193+
@Test
194+
final void testRoundTrip_withNullValue() throws IOException {
195+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_null.yaml");
196+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
197+
198+
// explicit null input -> deserializes with null settings (preserves null)
199+
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
200+
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
201+
assertThat(pluginModel1.getPluginSettings(), equalTo(null));
202+
203+
// null settings -> serializes back as null (round-trip preserved)
204+
final String serialized = mapper.writeValueAsString(pluginModel1);
205+
assertThat(serialized.contains("null"), equalTo(true));
206+
207+
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
208+
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
209+
assertThat(pluginModel2.getPluginSettings(), equalTo(null));
210+
}
211+
212+
@Test
213+
final void testRoundTrip_withEmptyValue() throws IOException {
214+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_with_empty_value.yaml");
215+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
216+
217+
// no-value (customPlugin:) -> same as null -> deserializes with null settings
218+
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
219+
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
220+
assertThat(pluginModel1.getPluginSettings(), equalTo(null));
221+
222+
// null settings -> serializes back as null (round-trip preserved)
223+
final String serialized = mapper.writeValueAsString(pluginModel1);
224+
assertThat(serialized.contains("null"), equalTo(true));
225+
226+
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
227+
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
228+
assertThat(pluginModel2.getPluginSettings(), equalTo(null));
229+
}
230+
231+
@Test
232+
final void testDeserialize_emptyString_throwsException() throws IOException {
233+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_empty_string.yaml");
234+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
235+
236+
final JsonMappingException exception = assertThrows(
237+
JsonMappingException.class,
238+
() -> mapper.readValue(inputStream, PluginModel.class)
239+
);
240+
assertThat(exception.getMessage(), containsString("Empty string is not allowed"));
241+
}
242+
243+
@Test
244+
final void testDeserialize_nonEmptyString_throwsException() throws IOException {
245+
final String yaml = "customPlugin: someStringValue";
246+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
247+
248+
final JsonMappingException exception = assertThrows(
249+
JsonMappingException.class,
250+
() -> mapper.readValue(yaml, PluginModel.class)
251+
);
252+
assertThat(exception.getMessage(), containsString("String values not allowed"));
253+
}
254+
255+
@ParameterizedTest
256+
@ValueSource(strings = {"plugin_model_number_value.yaml", "plugin_model_boolean_value.yaml", "plugin_model_array_value.yaml"})
257+
final void testDeserialize_invalidTokenType_throwsException(final String resourceName) throws IOException {
258+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream(resourceName);
259+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
260+
261+
final JsonMappingException exception = assertThrows(
262+
JsonMappingException.class,
263+
() -> mapper.readValue(inputStream, PluginModel.class)
264+
);
265+
assertThat(exception.getMessage(), containsString("Unexpected value for plugin"));
266+
}
267+
144268
static Map<String, Object> validPluginSettings() {
145269
final Map<String, Object> settings = new HashMap<>();
146270
settings.put("key1", "value1");
@@ -160,4 +284,12 @@ static String convertInputStreamToString(InputStream inputStream) throws IOExcep
160284
return stringBuilder.toString();
161285
}
162286

287+
static String stripComments(final String content) {
288+
final String stripped = Arrays.stream(content.split("\n"))
289+
.filter(line -> !line.startsWith("#"))
290+
.collect(java.util.stream.Collectors.joining("\n"))
291+
.replaceAll("^\n+", "");
292+
return content.endsWith("\n") ? stripped + "\n" : stripped;
293+
}
294+
163295
}

0 commit comments

Comments
 (0)
⚡