Fix append processor ignore_empty_values edge case

docs/changelog/136649.yaml

@@ -0,0 +1,5 @@
+pr: 136649
+summary: Fix append processor `ignore_empty_values` edge case
+area: Ingest Node
+type: bug
+issues: []

modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/380_append_edge_cases.yml

@@ -0,0 +1,236 @@
+---
+setup:
+  - requires:
+      cluster_features: [ "ingest.append.ignore_empty_values_fix" ]
+      reason: "These tests all verify that a bug in ignore_empty_values has been fixed"
+  - do:
+      ingest.put_pipeline:
+        id: "test-pipeline-1"
+        body: >
+          {
+            "processors": [
+              {
+                "append": {
+                  "if": "ctx?.templating != true",
+                  "field": "foo",
+                  "copy_from": "bar",
+                  "allow_duplicates": false,
+                  "ignore_empty_values": true
+                }
+              },
+              {
+                "append": {
+                  "if": "ctx?.templating == true",
+                  "field": "foo",
+                  "value": "{{bar}}",
+                  "allow_duplicates": true,
+                  "ignore_empty_values": true
+                }
+              },
+              {
+                "remove": {
+                  "field": "bar",
+                  "ignore_missing": true
+                }
+              },
+              {
+                "script": {
+                  "source": "def foo = ctx.foo; ctx.foo_is_null = (foo == null);"
+                }
+              }
+            ]
+          }
+  - do:
+      indices.create:
+        index: "test-some-index"
+
+---
+teardown:
+  - do:
+      indices.delete:
+        index: "test-some-index"
+        ignore_unavailable: true
+  - do:
+      ingest.delete_pipeline:
+        id: "test-pipeline-1"
+        ignore: 404
+
+---
+"scalar values will become a list if necessary":
+  - do:
+      index:
+        index: test-some-index
+        id: 1
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "foo": "2",
+            "bar": "3",
+            "templating": false
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "1"
+  - match: { _source.foo: ["2", "3"] }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 2
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "foo": "2",
+            "bar": "2",
+            "templating": true
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "2"
+  - match: { _source.foo: ["2", "2"] }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 3
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "foo": "2",
+            "bar": "3",
+            "templating": true
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "3"
+  - match: { _source.foo: ["2", "3"] }
+
+---
+"if a value doesn't change because of ignoring duplicates or empty values, it doesn't become a list":
+  - do:
+      index:
+        index: test-some-index
+        id: 1
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "foo": "2",
+            "bar": "2",
+            "templating": false
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "1"
+  - match: { _source.foo: "2" }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 2
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "foo": "2",
+            "bar": "",
+            "templating": false
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "2"
+  - match: { _source.foo: "2" }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 3
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "foo": "2",
+            "bar": "",
+            "templating": true
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "3"
+  - match: { _source.foo: "2" }
+
+---
+"an absent value isn't added in the absence of change":
+  - do:
+      index:
+        index: test-some-index
+        id: 1
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "bar": "",
+            "templating": false
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "1"
+  - match: { _source.foo_is_null: true }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 2
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "bar": "",
+            "templating": true
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "2"
+  - match: { _source.foo_is_null: true }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 3
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "templating": false
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "3"
+  - match: { _source.foo_is_null: true }
+
+  - do:
+      index:
+        index: test-some-index
+        id: 4
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "templating": true
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "4"
+  - match: { _source.foo_is_null: true }

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

@@ -879,7 +879,9 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
                 if (object == NOT_FOUND) {
                     List<Object> list = new ArrayList<>();
                     appendValues(list, value, allowDuplicates, ignoreEmptyValues);
-                    map.put(leafKey, list);
+                    if (list.isEmpty() == false) {
+                        map.put(leafKey, list);
+                    }
                 } else {
                     Object list = appendValues(object, value, allowDuplicates, ignoreEmptyValues);
                     if (list != object) {
@@ -897,7 +899,9 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
                 if (object == NOT_FOUND) {
                     List<Object> list = new ArrayList<>();
                     appendValues(list, value, allowDuplicates, ignoreEmptyValues);
-                    map.put(leafKey, list);
+                    if (list.isEmpty() == false) {
+                        map.put(leafKey, list);
+                    }
                 } else {
                     Object list = appendValues(object, value, allowDuplicates, ignoreEmptyValues);
                     if (list != object) {
@@ -949,49 +953,24 @@ private static Object appendValues(Object maybeList, Object value, boolean allow
             list = new ArrayList<>();
             list.add(maybeList);
         }
-        if (allowDuplicates) {
-            innerAppendValues(list, value, ignoreEmptyValues);
-            return list;
-        } else {
-            // if no values were appended due to duplication, return the original object so the ingest document remains unmodified
-            return innerAppendValuesWithoutDuplicates(list, value, ignoreEmptyValues) ? list : maybeList;
-        }
-    }
-
-    // helper method for use in appendValues above, please do not call this directly except from that method
-    private static void innerAppendValues(List<Object> list, Object value, boolean ignoreEmptyValues) {
-        if (value instanceof List<?> l) {
-            if (ignoreEmptyValues) {
-                l.forEach((v) -> {
-                    if (valueNotEmpty(v)) {
-                        list.add(v);
-                    }
-                });
-            } else {
-                list.addAll(l);
-            }
-        } else if (ignoreEmptyValues == false || valueNotEmpty(value)) {
-            list.add(value);
-        }
-    }
 
-    // helper method for use in appendValues above, please do not call this directly except from that method
-    private static boolean innerAppendValuesWithoutDuplicates(List<Object> list, Object value, boolean ignoreEmptyValues) {
         boolean valuesWereAppended = false;
         if (value instanceof List<?> valueList) {
             for (Object val : valueList) {
-                if (list.contains(val) == false && (ignoreEmptyValues == false || valueNotEmpty(val))) {
+                if ((allowDuplicates || list.contains(val) == false) && (ignoreEmptyValues == false || valueNotEmpty(val))) {
                     list.add(val);
                     valuesWereAppended = true;
                 }
             }
         } else {
-            if (list.contains(value) == false && (ignoreEmptyValues == false || valueNotEmpty(value))) {
+            if ((allowDuplicates || list.contains(value) == false) && (ignoreEmptyValues == false || valueNotEmpty(value))) {
                 list.add(value);
                 valuesWereAppended = true;
             }
         }
-        return valuesWereAppended;
+
+        // if no values were appended due to duplication/empties, return the original object so the ingest document remains unmodified
+        return valuesWereAppended ? list : maybeList;
     }
 
     private static boolean valueNotEmpty(Object value) {

server/src/main/java/org/elasticsearch/ingest/IngestFeatures.java

@@ -19,6 +19,7 @@ public class IngestFeatures implements FeatureSpecification {
     private static final NodeFeature INGEST_APPEND_COPY_FROM = new NodeFeature("ingest.append.copy_from", true);
     private static final NodeFeature INGEST_APPEND_IGNORE_EMPTY_VALUES = new NodeFeature("ingest.append.ignore_empty_values", true);
     private static final NodeFeature RANDOM_SAMPLING = new NodeFeature("random_sampling", true);
+    private static final NodeFeature INGEST_APPEND_IGNORE_EMPTY_VALUES_FIX = new NodeFeature("ingest.append.ignore_empty_values_fix", true);
 
     @Override
     public Set<NodeFeature> getFeatures() {
@@ -27,6 +28,12 @@ public Set<NodeFeature> getFeatures() {
 
     @Override
     public Set<NodeFeature> getTestFeatures() {
-        return Set.of(SIMULATE_INGEST_400_ON_FAILURE, INGEST_APPEND_COPY_FROM, INGEST_APPEND_IGNORE_EMPTY_VALUES, RANDOM_SAMPLING);
+        return Set.of(
+            SIMULATE_INGEST_400_ON_FAILURE,
+            INGEST_APPEND_COPY_FROM,
+            INGEST_APPEND_IGNORE_EMPTY_VALUES,
+            RANDOM_SAMPLING,
+            INGEST_APPEND_IGNORE_EMPTY_VALUES_FIX
+        );
     }
 }