Improved the NPE fix by providing empty maps when they are null or ab…

…sent in the YAML. #568 Signed-off-by: David Venable <[email protected]>
opensearch-project · Nov 12, 2021 · 003e68a · 003e68a
1 parent 5c9e2a2
commit 003e68a
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 18 deletions.
diff --git a/...va/org/opensearch/dataprepper/logstash/mapping/DefaultLogstashPluginAttributesMapper.java b/...va/org/opensearch/dataprepper/logstash/mapping/DefaultLogstashPluginAttributesMapper.java
@@ -22,16 +22,14 @@ public Map<String, Object> mapAttributes(final List<LogstashAttribute> logstashA
 
         Objects.requireNonNull(logstashAttributes);
         Objects.requireNonNull(logstashAttributesMappings);
+        Objects.requireNonNull(logstashAttributesMappings.getMappedAttributeNames());
+        Objects.requireNonNull(logstashAttributesMappings.getAdditionalAttributes());
 
-        final Map<String, Object> pluginSettings = new LinkedHashMap<>();
-
-        if(logstashAttributesMappings.getAdditionalAttributes() != null) {
-            pluginSettings.putAll(logstashAttributesMappings.getAdditionalAttributes());
-        }
+        final Map<String, Object> pluginSettings = new LinkedHashMap<>(logstashAttributesMappings.getAdditionalAttributes());
 
         final Map<String, String> mappedAttributeNames = logstashAttributesMappings.getMappedAttributeNames();
         logstashAttributes.forEach(logstashAttribute -> {
-            if (mappedAttributeNames != null && mappedAttributeNames.containsKey(logstashAttribute.getAttributeName())) {
+            if (mappedAttributeNames.containsKey(logstashAttribute.getAttributeName())) {
                 pluginSettings.put(
                         mappedAttributeNames.get(logstashAttribute.getAttributeName()),
                         logstashAttribute.getAttributeValue().getValue()

diff --git a/...src/main/java/org/opensearch/dataprepper/logstash/mapping/LogstashAttributesMappings.java b/...src/main/java/org/opensearch/dataprepper/logstash/mapping/LogstashAttributesMappings.java
@@ -13,7 +13,25 @@
  * @since 1.2
  */
 public interface LogstashAttributesMappings {
+    /**
+     * A map of attribute names in the Logstash configuration to
+     * the correct property name in Data Prepper.
+     * <p>
+     * This should not return null and should return empty if not defined.
+     *
+     * @return A Map
+     * @since 1.2
+     */
     Map<String, String> getMappedAttributeNames();
 
+    /**
+     * A map of Data Prepper property names to values to set on those
+     * properties.
+     * <p>
+     * This should not return null and should return empty if not defined.
+     *
+     * @return A Map
+     * @since 1.2
+     */
     Map<String, Object> getAdditionalAttributes();
 }
diff --git a/...ation/src/main/java/org/opensearch/dataprepper/logstash/mapping/LogstashMappingModel.java b/...ation/src/main/java/org/opensearch/dataprepper/logstash/mapping/LogstashMappingModel.java
@@ -6,6 +6,8 @@
 package org.opensearch.dataprepper.logstash.mapping;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonSetter;
+import com.fasterxml.jackson.annotation.Nulls;
 
 import java.util.Map;
 
@@ -23,9 +25,11 @@ class LogstashMappingModel implements LogstashAttributesMappings {
     private String attributesMapperClass;
 
     @JsonProperty
+    @JsonSetter(nulls = Nulls.AS_EMPTY)
     private Map<String, String> mappedAttributeNames;
 
     @JsonProperty
+    @JsonSetter(nulls = Nulls.AS_EMPTY)
     private Map<String, Object> additionalAttributes;
 
     public String getPluginName() {

diff --git a/...rg/opensearch/dataprepper/logstash/mapping/DefaultLogstashPluginAttributesMapperTest.java b/...rg/opensearch/dataprepper/logstash/mapping/DefaultLogstashPluginAttributesMapperTest.java
@@ -65,6 +65,26 @@ void mapAttributes_throws_with_null_Mappings() {
                 () -> objectUnderTest.mapAttributes(logstashAttributes, null));
     }
 
+    @Test
+    void mapAttributes_throws_with_null_Mappings_mappedAttributeNames() {
+        final DefaultLogstashPluginAttributesMapper objectUnderTest = createObjectUnderTest();
+
+        when(mappings.getMappedAttributeNames()).thenReturn(null);
+
+        assertThrows(NullPointerException.class,
+                () -> objectUnderTest.mapAttributes(logstashAttributes, mappings));
+    }
+
+    @Test
+    void mapAttributes_throws_with_null_Mappings_additionalAttributes() {
+        final DefaultLogstashPluginAttributesMapper objectUnderTest = createObjectUnderTest();
+
+        when(mappings.getAdditionalAttributes()).thenReturn(null);
+
+        assertThrows(NullPointerException.class,
+                () -> objectUnderTest.mapAttributes(logstashAttributes, mappings));
+    }
+
     @Test
     void mapAttributes_sets_mapped_attributes() {
         final String dataPrepperAttribute = UUID.randomUUID().toString();
@@ -94,16 +114,4 @@ void mapAttributes_sets_additional_attributes_to_those_values() {
         assertThat(actualPluginSettings, hasKey(additionalAttributeName));
         assertThat(actualPluginSettings.get(additionalAttributeName), equalTo(additionalAttributeValue));
     }
-
-    @Test
-    void mapAttributes_with_null_mappings_and_attributes() {
-        when(mappings.getMappedAttributeNames()).thenReturn(null);
-        when(mappings.getAdditionalAttributes()).thenReturn(null);
-
-        final Map<String, Object> actualPluginSettings =
-                createObjectUnderTest().mapAttributes(logstashAttributes, mappings);
-
-        assertThat(actualPluginSettings, notNullValue());
-        assertThat(actualPluginSettings.size(), equalTo(0));
-    }
 }
diff --git a/...n/src/test/java/org/opensearch/dataprepper/logstash/mapping/LogstashMappingModelTest.java b/...n/src/test/java/org/opensearch/dataprepper/logstash/mapping/LogstashMappingModelTest.java
@@ -41,4 +41,17 @@ void deserialize_from_JSON_should_have_expected_value() throws IOException {
         assertThat(logstashMappingModel.getAdditionalAttributes().get("addA"), equalTo(true));
         assertThat(logstashMappingModel.getAdditionalAttributes().get("addB"), equalTo("staticValueB"));
     }
+
+    @Test
+    void deserialize_from_JSON_with_null_values_has_empty_lists() throws IOException {
+
+        final LogstashMappingModel logstashMappingModel = objectMapper.readValue(this.getClass().getResourceAsStream("sample-with-nulls.mapping.yaml"), LogstashMappingModel.class);
+
+        assertThat(logstashMappingModel.getPluginName(), equalTo("samplePlugin"));
+        assertThat(logstashMappingModel.getAttributesMapperClass(), equalTo("org.opensearch.dataprepper.Placeholder"));
+        assertThat(logstashMappingModel.getMappedAttributeNames(), notNullValue());
+        assertThat(logstashMappingModel.getMappedAttributeNames().size(), equalTo(0));
+        assertThat(logstashMappingModel.getAdditionalAttributes(), notNullValue());
+        assertThat(logstashMappingModel.getAdditionalAttributes().size(), equalTo(0));
+    }
 }
diff --git a/...test/resources/org/opensearch/dataprepper/logstash/mapping/sample-with-nulls.mapping.yaml b/...test/resources/org/opensearch/dataprepper/logstash/mapping/sample-with-nulls.mapping.yaml
@@ -0,0 +1,4 @@
+pluginName: samplePlugin
+attributesMapperClass: org.opensearch.dataprepper.Placeholder
+mappedAttributeNames: null
+additionalAttributes: null