Fix simulate remove ingest processor throwing illegal_argument_exception

Signed-off-by: Gao Binlong <[email protected]>
opensearch-project · Dec 13, 2023 · 72154e9 · 72154e9
1 parent 074bc6a
commit 72154e9
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -195,6 +195,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix remote shards balancer and remove unused variables ([#11167](https://github.com/opensearch-project/OpenSearch/pull/11167))
 - Fix bug where replication lag grows post primary relocation ([#11238](https://github.com/opensearch-project/OpenSearch/pull/11238))
 - Fix template setting override for replication type ([#11417](https://github.com/opensearch-project/OpenSearch/pull/11417))
+- Fix simulate remove ingest processor throwing illegal_argument_exception
 
 ### 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
@@ -88,16 +88,18 @@ public IngestDocument execute(IngestDocument document) {
                 throw new IllegalArgumentException("cannot remove metadata field [" + path + "]");
             }
             // removing _id is disallowed when there's an external version specified in the request
-            String versionType = document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class);
-            if (path.equals(IngestDocument.Metadata.ID.getFieldName())
-                && !Objects.equals(versionType, VersionType.toString(VersionType.INTERNAL))) {
-                Long version = document.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class);
-                throw new IllegalArgumentException(
-                    "cannot remove metadata field [_id] when specifying external version for the document, version: "
-                        + version
-                        + ", version_type: "
-                        + versionType
-                );
+            if (path.equals(IngestDocument.Metadata.ID.getFieldName())) {
+                String versionType = document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class, true);
+                if (Objects.equals(versionType, VersionType.toString(VersionType.EXTERNAL))
+                    || Objects.equals(versionType, VersionType.toString(VersionType.EXTERNAL_GTE))) {
+                    Long version = document.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class, true);
+                    throw new IllegalArgumentException(
+                        "cannot remove metadata field [_id] when specifying external version for the document, version: "
+                            + version
+                            + ", version_type: "
+                            + versionType
+                    );
+                }
             }
             document.removeField(path);
         });

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
@@ -180,5 +180,21 @@ public void testRemoveMetadataField() throws Exception {
                         assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false));
                     }
         }
+
+        // test remove _id when _version_type is null
+        IngestDocument ingestDocumentWithNoVersionType = new IngestDocument(
+            RandomDocumentPicks.randomString(random()),
+            RandomDocumentPicks.randomString(random()),
+            RandomDocumentPicks.randomString(random()),
+            null,
+            null,
+            RandomDocumentPicks.randomSource(random())
+        );
+        Map<String, Object> config = new HashMap<>();
+        config.put("field", IngestDocument.Metadata.ID.getFieldName());
+        String processorTag = randomAlphaOfLength(10);
+        Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config);
+        processor.execute(ingestDocumentWithNoVersionType);
+        assertThat(ingestDocumentWithNoVersionType.hasField(IngestDocument.Metadata.ID.getFieldName()), equalTo(false));
     }
 }
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
@@ -5,6 +5,69 @@ teardown:
         id: "my_pipeline"
         ignore: 404
 
+---
+"Test simulate API works well with remove processor":
+  - do:
+      ingest.put_pipeline:
+        id: "my_pipeline"
+        body:  >
+          {
+            "description": "_description",
+            "processors": [
+              {
+                "remove" : {
+                  "field" : "{{foo}}"
+                }
+              }
+            ]
+          }
+  - match: { acknowledged: true }
+
+  # test simulating existing pipeline works well
+  - do:
+      ingest.simulate:
+        id: "my_pipeline"
+        body: >
+          {
+            "docs": [
+              {
+                "_source": {
+                  "foo": "bar",
+                  "bar": "zoo"
+                }
+              }
+            ]
+          }
+  - length: { docs: 1 }
+  - match: { docs.0.doc._source: { "foo": "bar" } }
+
+  # test simulating inflight pipeline works well
+  - do:
+      ingest.simulate:
+        body: >
+          {
+            "pipeline": {
+              "description": "_description",
+              "processors": [
+                {
+                  "remove" : {
+                    "field" : "{{foo}}"
+                  }
+                }
+              ]
+            },
+            "docs": [
+              {
+                "_source": {
+                  "foo": "bar",
+                  "bar": "zoo"
+                }
+              }
+            ]
+          }
+  - length: { docs: 1 }
+  - match: { docs.0.doc._source: { "foo": "bar" } }
+
 ---
 "Test remove processor with non-existing field and without ignore_missing":
   - do:
@@ -227,3 +290,32 @@ teardown:
         version: 1
         version_type: "external"
         body: { message: "foo bar baz" }
+
+  # test simulating pipeline with removing _id
+  - do:
+      ingest.simulate:
+        body: >
+          {
+            "pipeline": {
+              "description": "_description",
+              "processors": [
+                {
+                  "remove" : {
+                    "field" : "_id"
+                  }
+                }
+              ]
+            },
+            "docs": [
+              {
+                "_version_type": "external_gte",
+                "_version": 1,
+                "_source": {
+                  "foo": "bar",
+                  "bar": "zoo"
+                }
+              }
+            ]
+          }
+  - match: { docs.0.error.type: "illegal_argument_exception" }
+  - match: { docs.0.error.reason: "cannot remove metadata field [_id] when specifying external version for the document, version: 1, version_type: external_gte" }