Correctly handle non-integers in nested paths in the remove processor (#127006)

Author: joegallo

docs/changelog/127006.yaml

@@ -0,0 +1,5 @@
+pr: 127006
+summary: Correctly handle non-integers in nested paths in the remove processor
+area: Ingest Node
+type: bug
+issues: []

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java

@@ -16,6 +16,7 @@
 import org.elasticsearch.script.TemplateScript;
 import org.elasticsearch.test.ESTestCase;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
@@ -103,6 +104,7 @@ public void testIgnoreMissingAndNullInPath() throws Exception {
                 some.put("map", map);
                 source.put("some", some);
             }
+            default -> throw new AssertionError("failure, got illegal switch case");
         }
         IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source);
         Map<String, Object> config = new HashMap<>();
@@ -113,6 +115,53 @@ public void testIgnoreMissingAndNullInPath() throws Exception {
         assertThat(document.hasField("some.map.path"), is(false));
     }
 
+    public void testIgnoreMissingAndNonIntegerInPath() throws Exception {
+        Map<String, Object> source = new HashMap<>();
+        Map<String, Object> some = new HashMap<>();
+        List<Object> array = new ArrayList<>();
+        Map<String, Object> path = new HashMap<>();
+
+        switch (randomIntBetween(0, 6)) {
+            case 0 -> {
+                // empty source
+            }
+            case 1 -> {
+                source.put("some", null);
+            }
+            case 2 -> {
+                some.put("array", null);
+                source.put("some", some);
+            }
+            case 3 -> {
+                some.put("array", array);
+                source.put("some", some);
+            }
+            case 4 -> {
+                array.add(null);
+                some.put("array", array);
+                source.put("some", some);
+            }
+            case 5 -> {
+                array.add(path);
+                some.put("array", array);
+                source.put("some", some);
+            }
+            case 6 -> {
+                array.add("foobar");
+                some.put("array", array);
+                source.put("some", some);
+            }
+            default -> throw new AssertionError("failure, got illegal switch case");
+        }
+        IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source);
+        Map<String, Object> config = new HashMap<>();
+        config.put("field", "some.array.path");
+        config.put("ignore_missing", true);
+        Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, null, null, config, null);
+        processor.execute(document);
+        assertThat(document.hasField("some.array.path"), is(false));
+    }
+
     public void testKeepFields() throws Exception {
         Map<String, Object> address = new HashMap<>();
         address.put("street", "Ipiranga Street");

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

@@ -360,11 +360,13 @@ public void removeField(String path, boolean ignoreMissing) {
                 throw new IllegalArgumentException(Errors.notPresent(path, leafKey));
             }
         } else if (context instanceof List<?> list) {
-            int index;
+            int index = -1;
             try {
                 index = Integer.parseInt(leafKey);
             } catch (NumberFormatException e) {
-                throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e);
+                if (ignoreMissing == false) {
+                    throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e);
+                }
             }
             if (index < 0 || index >= list.size()) {
                 if (ignoreMissing == false) {

server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java

@@ -860,13 +860,25 @@ public void testRemoveFieldIgnoreMissing() {
 
         // if ignoreMissing is false, we throw an exception for values that aren't found
         IllegalArgumentException e;
-        if (randomBoolean()) {
-            document.setFieldValue("fizz.some", (Object) null);
-            e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
-            assertThat(e.getMessage(), is("cannot remove [nonsense] from null as part of path [fizz.some.nonsense]"));
-        } else {
-            e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
-            assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]"));
+        switch (randomIntBetween(0, 2)) {
+            case 0 -> {
+                document.setFieldValue("fizz.some", (Object) null);
+                e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
+                assertThat(e.getMessage(), is("cannot remove [nonsense] from null as part of path [fizz.some.nonsense]"));
+            }
+            case 1 -> {
+                document.setFieldValue("fizz.some", List.of("foo", "bar"));
+                e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
+                assertThat(
+                    e.getMessage(),
+                    is("[nonsense] is not an integer, cannot be used as an index as part of path [fizz.some.nonsense]")
+                );
+            }
+            case 2 -> {
+                e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
+                assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]"));
+            }
+            default -> throw new AssertionError("failure, got illegal switch case");
         }
 
         // but no exception is thrown if ignoreMissing is true