Fix remove ingest processor handing ignore_missing parameter not correctly

CHANGELOG.md

@@ -102,6 +102,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Fixed
 - Fix ignore_missing parameter has no effect when using template snippet in rename ingest processor ([#9725](https://github.com/opensearch-project/OpenSearch/pull/9725))
 - Fix broken backward compatibility from 2.7 for IndexSorted field indices ([#10045](https://github.com/opensearch-project/OpenSearch/pull/9725))
+- Fix remove ingest processor handing ignore_missing parameter not correctly ([10089](https://github.com/opensearch-project/OpenSearch/pull/10089))
 
 ### Security
 

modules/ingest-common/src/main/java/org/opensearch/ingest/common/RemoveProcessor.java

@@ -32,6 +32,7 @@
 
 package org.opensearch.ingest.common;
 
+import org.opensearch.core.common.Strings;
 import org.opensearch.ingest.AbstractProcessor;
 import org.opensearch.ingest.ConfigurationUtils;
 import org.opensearch.ingest.IngestDocument;
@@ -66,16 +67,20 @@ public List<TemplateScript.Factory> getFields() {
 
     @Override
     public IngestDocument execute(IngestDocument document) {
-        if (ignoreMissing) {
-            fields.forEach(field -> {
-                String path = document.renderTemplate(field);
-                if (document.hasField(path)) {
-                    document.removeField(path);
+        fields.forEach(field -> {
+            String path = document.renderTemplate(field);
+            final boolean fieldPathIsNullOrEmpty = Strings.isNullOrEmpty(path);
+            if (fieldPathIsNullOrEmpty || document.hasField(path) == false) {
+                if (ignoreMissing) {
+                    return;
+                } else if (fieldPathIsNullOrEmpty) {
+                    throw new IllegalArgumentException("field path cannot be null nor empty");
+                } else {
+                    throw new IllegalArgumentException("field [" + path + "] doesn't exist");
                 }
-            });
-        } else {
-            fields.forEach(document::removeField);
-        }
+            }
+            document.removeField(path);
+        });
         return document;
     }
 

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

@@ -42,9 +42,6 @@
 import java.util.HashMap;
 import java.util.Map;
 
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.equalTo;
-
 public class RemoveProcessorTests extends OpenSearchTestCase {
 
     public void testRemoveFields() throws Exception {
@@ -57,7 +54,7 @@ public void testRemoveFields() throws Exception {
             false
         );
         processor.execute(ingestDocument);
-        assertThat(ingestDocument.hasField(field), equalTo(false));
+        assertFalse(ingestDocument.hasField(field));
     }
 
     public void testRemoveNonExistingField() throws Exception {
@@ -71,7 +68,18 @@ public void testRemoveNonExistingField() throws Exception {
             processor.execute(ingestDocument);
             fail("remove field should have failed");
         } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("not present as part of path [" + fieldName + "]"));
+            assertEquals(e.getMessage(), "field [" + fieldName + "] doesn't exist");
+        }
+
+        Map<String, Object> configWithEmptyField = new HashMap<>();
+        configWithEmptyField.put("field", "");
+        processorTag = randomAlphaOfLength(10);
+        processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, configWithEmptyField);
+        try {
+            processor.execute(ingestDocument);
+            fail("remove empty field should have failed");
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "field path cannot be null nor empty");
         }
     }
 
@@ -84,5 +92,13 @@ public void testIgnoreMissing() throws Exception {
         String processorTag = randomAlphaOfLength(10);
         Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config);
         processor.execute(ingestDocument);
+
+        // when using template snippet, the resolved field path maybe empty
+        Map<String, Object> configWithEmptyField = new HashMap<>();
+        configWithEmptyField.put("field", "");
+        configWithEmptyField.put("ignore_missing", true);
+        processorTag = randomAlphaOfLength(10);
+        processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, configWithEmptyField);
+        processor.execute(ingestDocument);
     }
 }

modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml

@@ -0,0 +1,93 @@
+---
+teardown:
+  - do:
+      ingest.delete_pipeline:
+        id: "my_pipeline"
+        ignore: 404
+
+---
+"Test remove processor with non-existing field and without ignore_missing":
+  - do:
+      ingest.put_pipeline:
+        id: "my_pipeline"
+        body:  >
+          {
+            "description": "_description",
+            "processors": [
+              {
+                "remove" : {
+                  "field" : "{{unknown}}"
+                }
+              }
+            ]
+          }
+  - match: { acknowledged: true }
+
+  - do:
+      catch: /field path cannot be null nor empty/
+      index:
+        index: test
+        id: 1
+        pipeline: "my_pipeline"
+        body: { message: "foo bar baz" }
+
+---
+"Test remove processor with resolved field path doesn't exist":
+  - do:
+      ingest.put_pipeline:
+        id: "my_pipeline"
+        body:  >
+          {
+            "description": "_description",
+            "processors": [
+              {
+                "remove" : {
+                  "field" : "{{foo}}"
+                }
+              }
+            ]
+          }
+  - match: { acknowledged: true }
+
+  - do:
+      catch: /field \[bar\] doesn\'t exist/
+      index:
+        index: test
+        id: 1
+        pipeline: "my_pipeline"
+        body: {
+          message: "foo bar baz",
+          foo: "bar"
+        }
+
+---
+"Test remove processor with non-existing field and ignore_missing":
+  - do:
+      ingest.put_pipeline:
+        id: "my_pipeline"
+        body:  >
+          {
+            "description": "_description",
+            "processors": [
+              {
+                "remove" : {
+                  "field" : "{{unknown}}",
+                  "ignore_missing" : true
+                }
+              }
+            ]
+          }
+  - match: { acknowledged: true }
+
+  - do:
+      index:
+        index: test
+        id: 1
+        pipeline: "my_pipeline"
+        body: { message: "foo bar baz" }
+
+  - do:
+      get:
+        index: test
+        id: 1
+  - match: { _source.message: "foo bar baz" }