Skip to content

Commit ff519f6

Browse files
Davidding4718Davidding4718
committed
fix: update Jackson BOM to 2.20.2 and fix logstash expected YAML
Signed-off-by: Siqi Ding <dingdd@amazon.com>
1 parent 4cee36f commit ff519f6

File tree

17 files changed

+152
-72
lines changed

17 files changed

+152
-72
lines changed

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

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import com.fasterxml.jackson.core.JsonParser;
1717
import com.fasterxml.jackson.core.JsonToken;
1818
import com.fasterxml.jackson.databind.DeserializationContext;
19+
import com.fasterxml.jackson.databind.JsonMappingException;
1920
import com.fasterxml.jackson.databind.ObjectMapper;
2021
import com.fasterxml.jackson.databind.SerializerProvider;
2122
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
@@ -28,6 +29,7 @@
2829
import java.util.Map;
2930
import java.util.Objects;
3031
import java.util.function.BiFunction;
32+
import java.util.function.Supplier;
3133

3234
/**
3335
* Model class for a Plugin in Configuration YAML containing name of the Plugin and its associated settings
@@ -38,13 +40,7 @@
3840
@JsonDeserialize(using = PluginModel.PluginModelDeserializer.class)
3941
public class PluginModel {
4042

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-
}
43+
private static final ObjectMapper SERIALIZER_OBJECT_MAPPER = new ObjectMapper();
4844

4945
private final String pluginName;
5046
private final InternalJsonModel innerModel;
@@ -65,7 +61,6 @@ public class PluginModel {
6561
*/
6662
static class InternalJsonModel {
6763
@JsonAnySetter
68-
@JsonAnyGetter
6964
private final Map<String, Object> pluginSettings;
7065

7166
@JsonCreator
@@ -76,6 +71,11 @@ static class InternalJsonModel {
7671
InternalJsonModel(final Map<String, Object> pluginSettings) {
7772
this.pluginSettings = pluginSettings;
7873
}
74+
75+
@JsonAnyGetter
76+
Map<String, Object> getPluginSettingsForSerialization() {
77+
return pluginSettings != null ? pluginSettings : new HashMap<>();
78+
}
7979
}
8080

8181
public PluginModel(final String pluginName, final Map<String, Object> pluginSettings) {
@@ -121,27 +121,27 @@ public void serialize(
121121
final PluginModel value, final JsonGenerator gen, final SerializerProvider provider) throws IOException {
122122
gen.writeStartObject();
123123

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);
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);
128131

129132
if (serializedInner.isEmpty()) {
130-
// Inner model has no content - check if pluginSettings was explicitly null
131-
// to decide between null and {}
133+
// Empty inner model: output null if pluginSettings was null, {} if it was an empty map
132134
if (value.innerModel.pluginSettings == null) {
133-
// Explicitly null settings -> serialize as null
134135
gen.writeObjectField(value.getPluginName(), null);
135136
} else {
136-
// Empty (non-null) settings -> serialize as {}
137137
gen.writeFieldName(value.getPluginName());
138138
gen.writeStartObject();
139139
gen.writeEndObject();
140140
}
141141
} else {
142-
// Inner model has content (plugin settings or subclass fields like routes)
143142
gen.writeObjectField(value.getPluginName(), serializedInner);
144143
}
144+
145145
gen.writeEndObject();
146146
}
147147
}
@@ -157,7 +157,7 @@ public void serialize(
157157
static final class PluginModelDeserializer extends AbstractPluginModelDeserializer<PluginModel, InternalJsonModel> {
158158

159159
public PluginModelDeserializer() {
160-
super(PluginModel.class, InternalJsonModel.class, PluginModel::new);
160+
super(PluginModel.class, InternalJsonModel.class, PluginModel::new, () -> new InternalJsonModel(null));
161161
}
162162
}
163163

@@ -177,49 +177,57 @@ abstract static class AbstractPluginModelDeserializer<T extends PluginModel, M e
177177

178178
private final Class<M> innerModelClass;
179179
private final BiFunction<String, M, T> constructorFunction;
180+
private final Supplier<M> nullSettingsModelSupplier;
180181

181182
protected AbstractPluginModelDeserializer(
182183
final Class<T> valueClass,
183184
final Class<M> innerModelClass,
184-
final BiFunction<String, M, T> constructorFunction) {
185+
final BiFunction<String, M, T> constructorFunction,
186+
final Supplier<M> nullSettingsModelSupplier) {
185187
super(valueClass);
186188
this.innerModelClass = innerModelClass;
187189
this.constructorFunction = constructorFunction;
190+
this.nullSettingsModelSupplier = nullSettingsModelSupplier;
188191
}
189192

190193
@Override
191194
public PluginModel deserialize(final JsonParser jsonParser, final DeserializationContext context) throws IOException {
192-
ObjectMapper mapper = (ObjectMapper) jsonParser.getCodec();
195+
final ObjectMapper mapper = (ObjectMapper) jsonParser.getCodec();
193196

194197
jsonParser.nextToken();
195198

196199
final String pluginName = jsonParser.currentName();
197200
jsonParser.nextToken();
198201

199-
Map<String, Object> data = new HashMap<>();
202+
boolean isNull = false;
203+
Map<String, Object> data = null;
200204
if (jsonParser.currentToken() == JsonToken.START_OBJECT) {
201205
data = mapper.readValue(jsonParser, Map.class);
206+
// readValue consumed up to the inner END_OBJECT; advance to the outer END_OBJECT
207+
jsonParser.nextToken();
202208
} else if (jsonParser.currentToken() == JsonToken.VALUE_NULL) {
203-
// null value -> treat as empty object (acceptable format)
204-
data = new HashMap<>();
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();
205213
} 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
214+
final String value = jsonParser.getValueAsString();
209215
if (value.isEmpty()) {
210216
throw context.weirdStringException(value, Map.class,
211217
"Empty string is not allowed for plugin '" + pluginName + "'. Use null, empty (no value), or {} instead.");
212218
} else {
213219
throw context.weirdStringException(value, Map.class,
214220
"String values not allowed for plugin '" + pluginName + "'");
215221
}
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());
216226
}
217-
while (jsonParser.currentToken() != JsonToken.END_OBJECT) {
218-
jsonParser.nextToken();
219-
}
220-
221-
final M innerModel = SERIALIZER_OBJECT_MAPPER.convertValue(data, innerModelClass);
222227

228+
final M innerModel = isNull
229+
? nullSettingsModelSupplier.get()
230+
: SERIALIZER_OBJECT_MAPPER.convertValue(data, innerModelClass);
223231
return constructorFunction.apply(pluginName, innerModel);
224232
}
225233
}

‎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);
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: 63 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@
3333
import static org.hamcrest.CoreMatchers.equalTo;
3434
import static org.hamcrest.CoreMatchers.notNullValue;
3535
import static org.hamcrest.MatcherAssert.assertThat;
36+
import static org.hamcrest.Matchers.containsString;
3637
import static org.hamcrest.Matchers.hasKey;
38+
import static org.junit.jupiter.api.Assertions.assertThrows;
3739

3840
class PluginModelTests {
3941

@@ -75,14 +77,14 @@ final void testSerialization_empty_plugin_to_YAML() throws JsonGenerationExcepti
7577

7678
final String serialized = mapper.writeValueAsString(pluginModel);
7779

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

8082
assertThat(serialized, notNullValue());
8183
assertThat(serialized, equalTo(convertInputStreamToString(inputStream)));
8284
}
8385

8486
@ParameterizedTest
85-
@ValueSource(strings = {"plugin_model_empty.yaml", "plugin_model_not_present.yaml", "plugin_model_null.yaml"})
87+
@ValueSource(strings = {"plugin_model_with_empty_object.yaml"})
8688
final void deserialize_with_empty_inner(final String resourceName) throws IOException {
8789
final InputStream inputStream = PluginModelTests.class.getResourceAsStream(resourceName);
8890

@@ -94,6 +96,17 @@ final void deserialize_with_empty_inner(final String resourceName) throws IOExce
9496
assertThat(pluginModel.getPluginSettings().size(), equalTo(0));
9597
}
9698

99+
@Test
100+
final void deserialize_with_no_value_returns_null_settings() throws IOException {
101+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_not_present.yaml");
102+
103+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
104+
105+
final PluginModel pluginModel = mapper.readValue(inputStream, PluginModel.class);
106+
assertThat(pluginModel.getPluginName(), equalTo("customPlugin"));
107+
assertThat(pluginModel.getPluginSettings(), equalTo(null));
108+
}
109+
97110
@Test
98111
final void testUsingCustomSerializerWithPluginSettings_noExceptions() throws JsonGenerationException, JsonMappingException, IOException {
99112
final PluginModel pluginModel = new PluginModel("customPlugin", validPluginSettings());
@@ -141,10 +154,28 @@ final void testUsingCustomDeserializer_with_array() throws JsonParseException, J
141154
assertThat(readValue.listOfPlugins.get(1).getPluginSettings().get("key2"), equalTo("value2"));
142155
}
143156

157+
@Test
158+
final void testUsingCustomDeserializer_with_array_of_three_preserves_all_entries() throws JsonParseException, JsonMappingException, IOException {
159+
InputStream inputStream = PluginModelTests.class.getResourceAsStream("/list_of_plugins_multiple_fields.yaml");
160+
161+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
162+
163+
final PluginHolder readValue = mapper.readValue(inputStream, PluginHolder.class);
164+
assertThat(readValue, notNullValue());
165+
assertThat(readValue.listOfPlugins, notNullValue());
166+
assertThat(readValue.listOfPlugins.size(), equalTo(3));
167+
assertThat(readValue.listOfPlugins.get(0).getPluginName(), equalTo("customPluginA"));
168+
assertThat(readValue.listOfPlugins.get(0).getPluginSettings().get("key1"), equalTo("value1"));
169+
assertThat(readValue.listOfPlugins.get(1).getPluginName(), equalTo("customPluginB"));
170+
assertThat(readValue.listOfPlugins.get(1).getPluginSettings().get("key2"), equalTo("value2"));
171+
assertThat(readValue.listOfPlugins.get(2).getPluginName(), equalTo("customPluginC"));
172+
assertThat(readValue.listOfPlugins.get(2).getPluginSettings().get("key3"), equalTo("value3"));
173+
}
174+
144175
@Test
145176
final void testRoundTrip_withEmptyObject() throws IOException {
146177
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));
178+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
148179

149180
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
150181
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
@@ -161,63 +192,76 @@ final void testRoundTrip_withEmptyObject() throws IOException {
161192
@Test
162193
final void testRoundTrip_withNullValue() throws IOException {
163194
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));
195+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
165196

166-
// null input -> deserializes to empty settings
197+
// explicit null input -> deserializes with null settings (preserves null)
167198
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
168199
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
169-
assertThat(pluginModel1.getPluginSettings().size(), equalTo(0));
200+
assertThat(pluginModel1.getPluginSettings(), equalTo(null));
170201

171-
// empty settings -> serializes as {}
202+
// null settings -> serializes back as null (round-trip preserved)
172203
final String serialized = mapper.writeValueAsString(pluginModel1);
173-
assertThat(serialized.contains("{}"), equalTo(true));
204+
assertThat(serialized.contains("null"), equalTo(true));
174205

175206
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
176207
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
177-
assertThat(pluginModel2.getPluginSettings().size(), equalTo(0));
208+
assertThat(pluginModel2.getPluginSettings(), equalTo(null));
178209
}
179210

180211
@Test
181212
final void testRoundTrip_withEmptyValue() throws IOException {
182213
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));
214+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
184215

185-
// empty value (no value after colon) -> deserializes to empty settings
216+
// no-value (customPlugin:) -> same as null -> deserializes with null settings
186217
final PluginModel pluginModel1 = mapper.readValue(inputStream, PluginModel.class);
187218
assertThat(pluginModel1.getPluginName(), equalTo("customPlugin"));
188-
assertThat(pluginModel1.getPluginSettings().size(), equalTo(0));
219+
assertThat(pluginModel1.getPluginSettings(), equalTo(null));
189220

190-
// empty settings -> serializes as {}
221+
// null settings -> serializes back as null (round-trip preserved)
191222
final String serialized = mapper.writeValueAsString(pluginModel1);
192-
assertThat(serialized.contains("{}"), equalTo(true));
223+
assertThat(serialized.contains("null"), equalTo(true));
193224

194225
final PluginModel pluginModel2 = mapper.readValue(serialized, PluginModel.class);
195226
assertThat(pluginModel2.getPluginName(), equalTo("customPlugin"));
196-
assertThat(pluginModel2.getPluginSettings().size(), equalTo(0));
227+
assertThat(pluginModel2.getPluginSettings(), equalTo(null));
197228
}
198229

199230
@Test
200231
final void testDeserialize_emptyString_throwsException() throws IOException {
201232
final InputStream inputStream = PluginModelTests.class.getResourceAsStream("plugin_model_empty_string.yaml");
202233
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
203234

204-
final JsonMappingException exception = org.junit.jupiter.api.Assertions.assertThrows(
235+
final JsonMappingException exception = assertThrows(
205236
JsonMappingException.class,
206237
() -> mapper.readValue(inputStream, PluginModel.class)
207238
);
208-
assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("Empty string is not allowed"));
239+
assertThat(exception.getMessage(), containsString("Empty string is not allowed"));
209240
}
210241

211242
@Test
212243
final void testDeserialize_nonEmptyString_throwsException() throws IOException {
213244
final String yaml = "customPlugin: someStringValue";
214245
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
215246

216-
final JsonMappingException exception = org.junit.jupiter.api.Assertions.assertThrows(
247+
final JsonMappingException exception = assertThrows(
217248
JsonMappingException.class,
218249
() -> mapper.readValue(yaml, PluginModel.class)
219250
);
220-
assertThat(exception.getMessage(), org.hamcrest.Matchers.containsString("String values not allowed"));
251+
assertThat(exception.getMessage(), containsString("String values not allowed"));
252+
}
253+
254+
@ParameterizedTest
255+
@ValueSource(strings = {"plugin_model_number_value.yaml", "plugin_model_boolean_value.yaml", "plugin_model_array_value.yaml"})
256+
final void testDeserialize_invalidTokenType_throwsException(final String resourceName) throws IOException {
257+
final InputStream inputStream = PluginModelTests.class.getResourceAsStream(resourceName);
258+
final ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
259+
260+
final JsonMappingException exception = assertThrows(
261+
JsonMappingException.class,
262+
() -> mapper.readValue(inputStream, PluginModel.class)
263+
);
264+
assertThat(exception.getMessage(), containsString("Unexpected value for plugin"));
221265
}
222266

223267
static Map<String, Object> validPluginSettings() {

0 commit comments

Comments
 (0)
⚡