Fix allow_duplicates edge case bug in append processor (elastic#134319) (elastic#134378)

Author: joegallo

docs/changelog/134319.yaml

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

modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/360_append_allow_duplicates.yml

@@ -0,0 +1,110 @@
+---
+setup:
+  - do:
+      ingest.put_pipeline:
+        id: "test-pipeline-1"
+        body: >
+          {
+            "processors": [
+              {
+                "append": {
+                  "if": "ctx?.templating == true",
+                  "field": "dest",
+                  "value": ["{{three}}", "{{three}}", "{{three}}"],
+                  "allow_duplicates": false
+                }
+              },
+              {
+                "foreach": {
+                  "description": "templating results in strings, so convert them if necessary",
+                  "field": "dest",
+                  "processor": {
+                    "convert": {
+                      "field": "_ingest._value",
+                      "type": "long"
+                    }
+                  }
+                }
+              },
+              {
+                "remove": {
+                  "description": "we only care about the dest, so remove everything else",
+                  "keep": ["dest"]
+                }
+              }
+            ]
+          }
+  - 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
+
+---
+"allow_duplicates (false) removes duplicates with a present array and templating":
+  - do:
+      index:
+        index: test-some-index
+        id: 1
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "templating": true,
+            "three": 3,
+            "dest": ["3"],
+            "note": "blargh, duplicate removal is based on strings, because of templating"
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "1"
+  - match: { _source.dest: [3] }
+
+---
+"allow_duplicates (false) removes duplicates with an empty array and templating":
+  - do:
+      index:
+        index: test-some-index
+        id: 1
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "templating": true,
+            "three": 3,
+            "dest": []
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "1"
+  - match: { _source.dest: [3] }
+
+---
+"allow_duplicates (false) removes duplicates with a missing array and templating":
+  - do:
+      index:
+        index: test-some-index
+        id: 1
+        pipeline: test-pipeline-1
+        body: >
+          {
+            "templating": true,
+            "three": 3
+          }
+
+  - do:
+      get:
+        index: test-some-index
+        id: "1"
+  - match: { _source.dest: [3] }

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

@@ -600,7 +600,7 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
                 Object object = map.getOrDefault(leafKey, NOT_FOUND); // getOrDefault is faster than containsKey + get
                 if (object == NOT_FOUND) {
                     List<Object> list = new ArrayList<>();
-                    appendValues(list, value);
+                    appendValues(list, value, allowDuplicates);
                     map.put(leafKey, list);
                 } else {
                     Object list = appendValues(object, value, allowDuplicates);
@@ -618,7 +618,7 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
                 Object object = map.getOrDefault(leafKey, NOT_FOUND); // getOrDefault is faster than containsKey + get
                 if (object == NOT_FOUND) {
                     List<Object> list = new ArrayList<>();
-                    appendValues(list, value);
+                    appendValues(list, value, allowDuplicates);
                     map.put(leafKey, list);
                 } else {
                     Object list = appendValues(object, value, allowDuplicates);
@@ -668,23 +668,25 @@ private static Object appendValues(Object maybeList, Object value, boolean allow
             list.add(maybeList);
         }
         if (allowDuplicates) {
-            appendValues(list, value);
+            innerAppendValues(list, value);
             return list;
         } else {
             // if no values were appended due to duplication, return the original object so the ingest document remains unmodified
-            return appendValuesWithoutDuplicates(list, value) ? list : maybeList;
+            return innerAppendValuesWithoutDuplicates(list, value) ? list : maybeList;
         }
     }
 
-    private static void appendValues(List<Object> list, Object value) {
+    // 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) {
         if (value instanceof List<?> l) {
             list.addAll(l);
         } else {
             list.add(value);
         }
     }
 
-    private static boolean appendValuesWithoutDuplicates(List<Object> list, Object 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 valuesWereAppended = false;
         if (value instanceof List<?> valueList) {
             for (Object val : valueList) {