Fix remove ingest processor handing ignore_missing parameter not corr…

…ectly (opensearch-project#10089) * Fix remove ingest processor handing ignore_missing parameter not correctly Signed-off-by: Gao Binlong <[email protected]> * modify change log Signed-off-by: Gao Binlong <[email protected]> * Fix test failure and optimize some code Signed-off-by: Gao Binlong <[email protected]> * Minor change Signed-off-by: Gao Binlong <[email protected]> * Modify test code Signed-off-by: Gao Binlong <[email protected]> * Split a test method to two Signed-off-by: Gao Binlong <[email protected]> --------- Signed-off-by: Gao Binlong <[email protected]>
reta · Sep 25, 2023 · c44337e · c44337e
1 parent b83f433
commit c44337e
Show file tree

Hide file tree

Showing 5 changed files with 155 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -116,6 +116,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - 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/10045))
 - Fix concurrent search NPE when track_total_hits, terminate_after and size=0 are used ([#10082](https://github.com/opensearch-project/OpenSearch/pull/10082))
+- Fix remove ingest processor handing ignore_missing parameter not correctly ([10089](https://github.com/opensearch-project/OpenSearch/pull/10089))
 
 ### Security
 

diff --git a/modules/ingest-common/src/main/java/org/opensearch/ingest/common/RemoveProcessor.java b/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;
     }
 

diff --git a/modules/ingest-common/src/test/java/org/opensearch/ingest/common/RemoveProcessorTests.java b/modules/ingest-common/src/test/java/org/opensearch/ingest/common/RemoveProcessorTests.java
@@ -42,7 +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 {
@@ -67,12 +66,44 @@ public void testRemoveNonExistingField() throws Exception {
         config.put("field", fieldName);
         String processorTag = randomAlphaOfLength(10);
         Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config);
-        try {
-            processor.execute(ingestDocument);
-            fail("remove field should have failed");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("not present as part of path [" + fieldName + "]"));
-        }
+        assertThrows(
+            "field [" + fieldName + "] doesn't exist",
+            IllegalArgumentException.class,
+            () -> { processor.execute(ingestDocument); }
+        );
+
+        Map<String, Object> configWithEmptyField = new HashMap<>();
+        configWithEmptyField.put("field", "");
+        processorTag = randomAlphaOfLength(10);
+        Processor removeProcessorWithEmptyField = new RemoveProcessor.Factory(TestTemplateService.instance()).create(
+            null,
+            processorTag,
+            null,
+            configWithEmptyField
+        );
+        assertThrows(
+            "field path cannot be null nor empty",
+            IllegalArgumentException.class,
+            () -> removeProcessorWithEmptyField.execute(ingestDocument)
+        );
+    }
+
+    public void testRemoveEmptyField() throws Exception {
+        IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>());
+        Map<String, Object> config = new HashMap<>();
+        config.put("field", "");
+        String processorTag = randomAlphaOfLength(10);
+        Processor removeProcessorWithEmptyField = new RemoveProcessor.Factory(TestTemplateService.instance()).create(
+            null,
+            processorTag,
+            null,
+            config
+        );
+        assertThrows(
+            "field path cannot be null nor empty",
+            IllegalArgumentException.class,
+            () -> removeProcessorWithEmptyField.execute(ingestDocument)
+        );
     }
 
     public void testIgnoreMissing() throws Exception {
@@ -84,5 +115,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);
     }
 }
diff --git a/...gest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml b/...gest-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" }
diff --git a/...cies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml b/...cies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml
@@ -208,7 +208,7 @@
         id: 1
   - length: { _source: 2 }
   - match: { _source.do_nothing: "foo" }
-  - match: { _source.error: "processor first_processor [remove]: field [field_to_remove] not present as part of path [field_to_remove]" }
+  - match: { _source.error: "processor first_processor [remove]: field [field_to_remove] doesn't exist" }
 
 ---
 "Test rolling up json object arrays":