From 283fe28bf4b0ace8b1444a41cb9307c9ae8f1c8a Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Thu, 19 Oct 2023 13:32:42 +0800 Subject: [PATCH 01/10] Ignore metadata fields when removing fields by remove ingest processor Signed-off-by: Gao Binlong --- CHANGELOG.md | 1 + .../ingest/common/RemoveProcessor.java | 11 ++- .../ingest/common/RemoveProcessorTests.java | 31 ++++++- .../test/ingest/290_remove_processor.yml | 90 +++++++++++++++++++ 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b40878066960a..61ef68b0b135a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [BUG] Fix java.lang.SecurityException in repository-gcs plugin ([#10642](https://github.com/opensearch-project/OpenSearch/pull/10642)) - Add telemetry tracer/metric enable flag and integ test. ([#10395](https://github.com/opensearch-project/OpenSearch/pull/10395)) - Add instrumentation for indexing in transport bulk action and transport shard bulk action. ([#10273](https://github.com/opensearch-project/OpenSearch/pull/10273)) +- Ignore metadata fields when removing fields by remove ingest processor ### Deprecated 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 index 93a35eef4d396..cb3a206538a68 100644 --- 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 @@ -43,6 +43,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; /** @@ -67,6 +68,11 @@ public List getFields() { @Override public IngestDocument execute(IngestDocument document) { + final Set metadataFields = document.getMetadata() + .keySet() + .stream() + .map(IngestDocument.Metadata::getFieldName) + .collect(Collectors.toSet()); fields.forEach(field -> { String path = document.renderTemplate(field); final boolean fieldPathIsNullOrEmpty = Strings.isNullOrEmpty(path); @@ -79,7 +85,10 @@ public IngestDocument execute(IngestDocument document) { throw new IllegalArgumentException("field [" + path + "] doesn't exist"); } } - document.removeField(path); + // ignore metadata fields such as _index, _id, etc. + if (!metadataFields.contains(path)) { + 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 index 8f729c6a39bbd..f2d03ddcc9411 100644 --- 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 @@ -40,7 +40,9 @@ import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.hamcrest.Matchers.equalTo; @@ -48,7 +50,7 @@ public class RemoveProcessorTests extends OpenSearchTestCase { public void testRemoveFields() throws Exception { IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random()); - String field = RandomDocumentPicks.randomExistingFieldName(random(), ingestDocument); + String field = RandomDocumentPicks.addRandomField(random(), ingestDocument, randomAlphaOfLength(10)); Processor processor = new RemoveProcessor( randomAlphaOfLength(10), null, @@ -124,4 +126,31 @@ public void testIgnoreMissing() throws Exception { processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, configWithEmptyField); processor.execute(ingestDocument); } + + public void testRemoveMetadataField() throws Exception { + IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); + // ignore _if_seq_no + List metadataFields = ingestDocument.getMetadata() + .keySet() + .stream() + .map(IngestDocument.Metadata::getFieldName) + .collect(Collectors.toList()); + String metadataFieldName = metadataFields.get(randomIntBetween(0, metadataFields.size() - 1)); + Map config = new HashMap<>(); + config.put("field", metadataFieldName); + String processorTag = randomAlphaOfLength(10); + Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config); + // _if_seq_no and _if_primary_term do not exist in the enriched document, removing them will throw IllegalArgumentException + if (metadataFieldName.equals(IngestDocument.Metadata.IF_SEQ_NO.getFieldName()) + || metadataFieldName.equals(IngestDocument.Metadata.IF_PRIMARY_TERM.getFieldName())) { + assertThrows( + "field: [" + metadataFieldName + "] doesn't exist", + IllegalArgumentException.class, + () -> processor.execute(ingestDocument) + ); + } else { + // for other metadata fields such as _index, id, ignore the removing operation + assertThat(ingestDocument.hasField(metadataFieldName), equalTo(true)); + } + } } diff --git a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml index ff5a17136afa2..5f17d11031e81 100644 --- a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml +++ b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml @@ -91,3 +91,93 @@ teardown: index: test id: 1 - match: { _source.message: "foo bar baz" } + +--- +"Test remove metadata field": + - do: + ingest.put_pipeline: + id: "my_pipeline" + body: > + { + "description": "_description", + "processors": [ + { + "remove" : { + "field" : "{{foo}}" + } + } + ] + } + - match: { acknowledged: true } + + - do: + index: + index: test + id: 1 + pipeline: "my_pipeline" + body: { + foo: "_index" + } + - do: + get: + index: test + id: 1 + - match: { _source: { foo: "_index" } } + + - do: + ingest.put_pipeline: + id: "my_pipeline" + body: > + { + "description": "_description", + "processors": [ + { + "remove" : { + "field" : "_id" + } + } + ] + } + - 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" } } + + - do: + ingest.put_pipeline: + id: "my_pipeline" + body: > + { + "description": "_description", + "processors": [ + { + "remove" : { + "field" : "_version" + } + } + ] + } + - match: { acknowledged: true } + + - do: + index: + index: test + id: "test_id_10000" + pipeline: "my_pipeline" + version: 1 + version_type: "external" + body: { message: "foo bar baz" } + - do: + get: + index: test + id: 1 + - match: { _source: { message: "foo bar baz" } } From ed8223358d9b0dd78f4f4f70acb24cc5f8e8f05b Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Wed, 25 Oct 2023 10:15:27 +0800 Subject: [PATCH 02/10] Modify change log Signed-off-by: Gao Binlong --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61ef68b0b135a..ff6b5c8700bde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,7 +120,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [BUG] Fix java.lang.SecurityException in repository-gcs plugin ([#10642](https://github.com/opensearch-project/OpenSearch/pull/10642)) - Add telemetry tracer/metric enable flag and integ test. ([#10395](https://github.com/opensearch-project/OpenSearch/pull/10395)) - Add instrumentation for indexing in transport bulk action and transport shard bulk action. ([#10273](https://github.com/opensearch-project/OpenSearch/pull/10273)) -- Ignore metadata fields when removing fields by remove ingest processor +- Ignore metadata fields when removing fields by remove ingest processor ([#10895](https://github.com/opensearch-project/OpenSearch/pull/10895)) ### Deprecated From a4a46667430051c6b0e62c0b7b4941f159d8f665 Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Fri, 17 Nov 2023 11:15:40 +0800 Subject: [PATCH 03/10] Throw exception when removing some metadata fields Signed-off-by: Gao Binlong --- CHANGELOG.md | 2 +- .../ingest/common/RemoveProcessor.java | 28 +++++--- .../ingest/common/RemoveProcessorTests.java | 34 ++++++++-- .../test/ingest/290_remove_processor.yml | 65 +++++++++++++++---- 4 files changed, 104 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad7a3b382cea9..4762701ad5ac7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,7 +130,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [BUG] Fix java.lang.SecurityException in repository-gcs plugin ([#10642](https://github.com/opensearch-project/OpenSearch/pull/10642)) - Add telemetry tracer/metric enable flag and integ test. ([#10395](https://github.com/opensearch-project/OpenSearch/pull/10395)) - Add instrumentation for indexing in transport bulk action and transport shard bulk action. ([#10273](https://github.com/opensearch-project/OpenSearch/pull/10273)) -- Ignore metadata fields when removing fields by remove ingest processor ([#10895](https://github.com/opensearch-project/OpenSearch/pull/10895)) +- Disallow removing some metadata fields by remove ingest processor ([#10895](https://github.com/opensearch-project/OpenSearch/pull/10895)) - [BUG] Disable sort optimization for HALF_FLOAT ([#10999](https://github.com/opensearch-project/OpenSearch/pull/10999)) - Refactor common parts from the Rounding class into a separate 'round' package ([#11023](https://github.com/opensearch-project/OpenSearch/issues/11023)) - Performance improvement for MultiTerm Queries on Keyword fields ([#7057](https://github.com/opensearch-project/OpenSearch/issues/7057)) 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 index cb3a206538a68..15550d02c8256 100644 --- 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 @@ -33,6 +33,7 @@ package org.opensearch.ingest.common; import org.opensearch.core.common.Strings; +import org.opensearch.index.VersionType; import org.opensearch.ingest.AbstractProcessor; import org.opensearch.ingest.ConfigurationUtils; import org.opensearch.ingest.IngestDocument; @@ -68,11 +69,6 @@ public List getFields() { @Override public IngestDocument execute(IngestDocument document) { - final Set metadataFields = document.getMetadata() - .keySet() - .stream() - .map(IngestDocument.Metadata::getFieldName) - .collect(Collectors.toSet()); fields.forEach(field -> { String path = document.renderTemplate(field); final boolean fieldPathIsNullOrEmpty = Strings.isNullOrEmpty(path); @@ -85,10 +81,26 @@ public IngestDocument execute(IngestDocument document) { throw new IllegalArgumentException("field [" + path + "] doesn't exist"); } } - // ignore metadata fields such as _index, _id, etc. - if (!metadataFields.contains(path)) { - document.removeField(path); + // cannot remove _index, _version and _version_type. + if (path.equals(IngestDocument.Metadata.INDEX.getFieldName()) + || path.equals(IngestDocument.Metadata.VERSION.getFieldName()) + || path.equals(IngestDocument.Metadata.VERSION_TYPE.getFieldName())) { + throw new IllegalArgumentException("cannot remove metadata field [" + path + "]"); } + // removing _id is disallowed when there's an external version specified in the request + if (path.equals(IngestDocument.Metadata.ID.getFieldName()) + && !document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class) + .equals(VersionType.INTERNAL.name().toLowerCase())) { + Long version = document.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); + String versionType = document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); + throw new IllegalArgumentException( + "cannot remove metadata field [_id] when specifying external version for the document, version: " + + version + + ", version_type: " + + versionType + ); + } + 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 index f2d03ddcc9411..d0ce43c481c77 100644 --- 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 @@ -32,6 +32,7 @@ package org.opensearch.ingest.common; +import org.opensearch.index.VersionType; import org.opensearch.ingest.IngestDocument; import org.opensearch.ingest.Processor; import org.opensearch.ingest.RandomDocumentPicks; @@ -129,7 +130,6 @@ public void testIgnoreMissing() throws Exception { public void testRemoveMetadataField() throws Exception { IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); - // ignore _if_seq_no List metadataFields = ingestDocument.getMetadata() .keySet() .stream() @@ -148,9 +148,35 @@ public void testRemoveMetadataField() throws Exception { IllegalArgumentException.class, () -> processor.execute(ingestDocument) ); - } else { - // for other metadata fields such as _index, id, ignore the removing operation - assertThat(ingestDocument.hasField(metadataFieldName), equalTo(true)); + } else if (metadataFieldName.equals(IngestDocument.Metadata.INDEX.getFieldName()) + || metadataFieldName.equals(IngestDocument.Metadata.VERSION.getFieldName()) + || metadataFieldName.equals(IngestDocument.Metadata.VERSION_TYPE.getFieldName())) { + // _index, _version and _version_type cannot be removed + assertThrows( + "cannot remove metadata field [" + metadataFieldName + "]", + IllegalArgumentException.class, + () -> processor.execute(ingestDocument) + ); + } else if (metadataFieldName.equals(IngestDocument.Metadata.ID.getFieldName())) { + Long version = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); + String versionType = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); + if (!versionType.equals(VersionType.INTERNAL.name().toLowerCase())) { + assertThrows( + "cannot remove metadata field [_id] when specifying external version for the document, version: " + + version + + ", version_type: " + + versionType, + IllegalArgumentException.class, + () -> processor.execute(ingestDocument) + ); + } else { + processor.execute(ingestDocument); + assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); + } + } else if (metadataFieldName.equals(IngestDocument.Metadata.ROUTING.getFieldName()) + && ingestDocument.hasField(IngestDocument.Metadata.ROUTING.getFieldName())) { + processor.execute(ingestDocument); + assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); } } } diff --git a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml index 5f17d11031e81..28ac00fe066a3 100644 --- a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml +++ b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml @@ -111,6 +111,7 @@ teardown: - match: { acknowledged: true } - do: + catch: /cannot remove metadata field \[\_index\]/ index: index: test id: 1 @@ -118,11 +119,32 @@ teardown: body: { foo: "_index" } + - do: - get: + ingest.put_pipeline: + id: "my_pipeline" + body: > + { + "description": "_description", + "processors": [ + { + "remove" : { + "field" : "_version" + } + } + ] + } + - match: { acknowledged: true } + + - do: + catch: /cannot remove metadata field \[\_version\]/ + index: index: test id: 1 - - match: { _source: { foo: "_index" } } + pipeline: "my_pipeline" + body: { + foo: "bar" + } - do: ingest.put_pipeline: @@ -133,7 +155,7 @@ teardown: "processors": [ { "remove" : { - "field" : "_id" + "field" : "_version_type" } } ] @@ -141,16 +163,39 @@ teardown: - match: { acknowledged: true } - do: + catch: /cannot remove metadata field \[\_version\_type\]/ index: index: test id: 1 pipeline: "my_pipeline" - body: { message: "foo bar baz" } + body: { + foo: "bar" + } + - do: - get: + ingest.put_pipeline: + id: "my_pipeline" + body: > + { + "description": "_description", + "processors": [ + { + "remove" : { + "field" : ["_id", "_routing"] + } + } + ] + } + - match: { acknowledged: true } + + - do: + index: index: test id: 1 - - match: { _source: { message: "foo bar baz" } } + routing: abc + pipeline: "my_pipeline" + body: { message: "foo bar baz" } + - match: { acknowledged: true } - do: ingest.put_pipeline: @@ -161,7 +206,7 @@ teardown: "processors": [ { "remove" : { - "field" : "_version" + "field" : "_id" } } ] @@ -169,6 +214,7 @@ teardown: - match: { acknowledged: true } - do: + catch: /cannot remove metadata field [_id] when specifying external version for the document/ index: index: test id: "test_id_10000" @@ -176,8 +222,3 @@ teardown: version: 1 version_type: "external" body: { message: "foo bar baz" } - - do: - get: - index: test - id: 1 - - match: { _source: { message: "foo bar baz" } } From 3b787e5a5de41d0d7c1081c634daa669eb5d9225 Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Fri, 17 Nov 2023 14:02:57 +0800 Subject: [PATCH 04/10] Format the code Signed-off-by: Gao Binlong --- .../main/java/org/opensearch/ingest/common/RemoveProcessor.java | 1 - .../java/org/opensearch/ingest/common/RemoveProcessorTests.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) 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 index 15550d02c8256..d9252e6bbd74f 100644 --- 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 @@ -44,7 +44,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.stream.Collectors; /** 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 index d0ce43c481c77..0bb5185afc191 100644 --- 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 @@ -177,6 +177,6 @@ public void testRemoveMetadataField() throws Exception { && ingestDocument.hasField(IngestDocument.Metadata.ROUTING.getFieldName())) { processor.execute(ingestDocument); assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); - } + } } } From bee41d327babf22749780a7b08616cba46a93ff5 Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Fri, 17 Nov 2023 14:09:08 +0800 Subject: [PATCH 05/10] Remove calling toLowerCase() Signed-off-by: Gao Binlong --- .../main/java/org/opensearch/ingest/common/RemoveProcessor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index d9252e6bbd74f..bbb475b79cdec 100644 --- 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 @@ -89,7 +89,7 @@ public IngestDocument execute(IngestDocument document) { // removing _id is disallowed when there's an external version specified in the request if (path.equals(IngestDocument.Metadata.ID.getFieldName()) && !document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class) - .equals(VersionType.INTERNAL.name().toLowerCase())) { + .equals(VersionType.INTERNAL.toString())) { Long version = document.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); String versionType = document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); throw new IllegalArgumentException( From 7c82da7f1772d25703c5c184e14ad6d8c389058d Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Mon, 20 Nov 2023 11:45:53 +0800 Subject: [PATCH 06/10] Remove calling toLowerCase() Signed-off-by: Gao Binlong --- .../java/org/opensearch/ingest/common/RemoveProcessorTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 0bb5185afc191..2b753fd757e8a 100644 --- 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 @@ -160,7 +160,7 @@ public void testRemoveMetadataField() throws Exception { } else if (metadataFieldName.equals(IngestDocument.Metadata.ID.getFieldName())) { Long version = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); String versionType = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); - if (!versionType.equals(VersionType.INTERNAL.name().toLowerCase())) { + if (!versionType.equals(VersionType.INTERNAL.toString())) { assertThrows( "cannot remove metadata field [_id] when specifying external version for the document, version: " + version From 8c6df8c9673c8243b1de4d83f530394dccfc4ddc Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Mon, 20 Nov 2023 14:32:54 +0800 Subject: [PATCH 07/10] Fix test failure Signed-off-by: Gao Binlong --- .../java/org/opensearch/ingest/common/RemoveProcessor.java | 2 +- .../org/opensearch/ingest/common/RemoveProcessorTests.java | 2 +- .../rest-api-spec/test/ingest/290_remove_processor.yml | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) 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 index bbb475b79cdec..e7732180f7336 100644 --- 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 @@ -89,7 +89,7 @@ public IngestDocument execute(IngestDocument document) { // removing _id is disallowed when there's an external version specified in the request if (path.equals(IngestDocument.Metadata.ID.getFieldName()) && !document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class) - .equals(VersionType.INTERNAL.toString())) { + .equals(VersionType.toString(VersionType.INTERNAL))) { Long version = document.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); String versionType = document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); throw new IllegalArgumentException( 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 index 2b753fd757e8a..83569a32f645f 100644 --- 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 @@ -160,7 +160,7 @@ public void testRemoveMetadataField() throws Exception { } else if (metadataFieldName.equals(IngestDocument.Metadata.ID.getFieldName())) { Long version = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); String versionType = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); - if (!versionType.equals(VersionType.INTERNAL.toString())) { + if (!versionType.equals(VersionType.toString(VersionType.INTERNAL))) { assertThrows( "cannot remove metadata field [_id] when specifying external version for the document, version: " + version diff --git a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml index 28ac00fe066a3..3079aea220d4a 100644 --- a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml +++ b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml @@ -195,7 +195,7 @@ teardown: routing: abc pipeline: "my_pipeline" body: { message: "foo bar baz" } - - match: { acknowledged: true } + - match: { result: created } - do: ingest.put_pipeline: @@ -214,7 +214,7 @@ teardown: - match: { acknowledged: true } - do: - catch: /cannot remove metadata field [_id] when specifying external version for the document/ + catch: /cannot remove metadata field \[\_id\] when specifying external version for the document/ index: index: test id: "test_id_10000" From 12238e7fbd6404c3e5e812c575c8051d4271ee6a Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Mon, 20 Nov 2023 14:36:54 +0800 Subject: [PATCH 08/10] Add skip config in yml test file Signed-off-by: Gao Binlong --- .../rest-api-spec/test/ingest/290_remove_processor.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml index 3079aea220d4a..4811769d04f0e 100644 --- a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml +++ b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_remove_processor.yml @@ -92,8 +92,13 @@ teardown: id: 1 - match: { _source.message: "foo bar baz" } +#Related issue: https://github.com/opensearch-project/OpenSearch/issues/10732 --- "Test remove metadata field": + - skip: + version: " - 2.11.99" + reason: "The bug was fixed in 2.12" + - do: ingest.put_pipeline: id: "my_pipeline" From 2ea8390955caf35f1ce0b75f0bf7f95fb4789a13 Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Mon, 20 Nov 2023 15:56:36 +0800 Subject: [PATCH 09/10] Improve test coverage Signed-off-by: Gao Binlong --- .../ingest/common/RemoveProcessorTests.java | 72 ++++++++++--------- 1 file changed, 37 insertions(+), 35 deletions(-) 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 index 83569a32f645f..1a5630a4730f2 100644 --- 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 @@ -135,48 +135,50 @@ public void testRemoveMetadataField() throws Exception { .stream() .map(IngestDocument.Metadata::getFieldName) .collect(Collectors.toList()); - String metadataFieldName = metadataFields.get(randomIntBetween(0, metadataFields.size() - 1)); - Map config = new HashMap<>(); - config.put("field", metadataFieldName); - String processorTag = randomAlphaOfLength(10); - Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config); - // _if_seq_no and _if_primary_term do not exist in the enriched document, removing them will throw IllegalArgumentException - if (metadataFieldName.equals(IngestDocument.Metadata.IF_SEQ_NO.getFieldName()) - || metadataFieldName.equals(IngestDocument.Metadata.IF_PRIMARY_TERM.getFieldName())) { - assertThrows( - "field: [" + metadataFieldName + "] doesn't exist", - IllegalArgumentException.class, - () -> processor.execute(ingestDocument) - ); - } else if (metadataFieldName.equals(IngestDocument.Metadata.INDEX.getFieldName()) - || metadataFieldName.equals(IngestDocument.Metadata.VERSION.getFieldName()) - || metadataFieldName.equals(IngestDocument.Metadata.VERSION_TYPE.getFieldName())) { - // _index, _version and _version_type cannot be removed + + for (String metadataFieldName : metadataFields) { + Map config = new HashMap<>(); + config.put("field", metadataFieldName); + String processorTag = randomAlphaOfLength(10); + Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config); + // _if_seq_no and _if_primary_term do not exist in the enriched document, removing them will throw IllegalArgumentException + if (metadataFieldName.equals(IngestDocument.Metadata.IF_SEQ_NO.getFieldName()) + || metadataFieldName.equals(IngestDocument.Metadata.IF_PRIMARY_TERM.getFieldName())) { assertThrows( - "cannot remove metadata field [" + metadataFieldName + "]", + "field: [" + metadataFieldName + "] doesn't exist", IllegalArgumentException.class, () -> processor.execute(ingestDocument) ); - } else if (metadataFieldName.equals(IngestDocument.Metadata.ID.getFieldName())) { - Long version = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); - String versionType = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); - if (!versionType.equals(VersionType.toString(VersionType.INTERNAL))) { + } else if (metadataFieldName.equals(IngestDocument.Metadata.INDEX.getFieldName()) + || metadataFieldName.equals(IngestDocument.Metadata.VERSION.getFieldName()) + || metadataFieldName.equals(IngestDocument.Metadata.VERSION_TYPE.getFieldName())) { + // _index, _version and _version_type cannot be removed assertThrows( - "cannot remove metadata field [_id] when specifying external version for the document, version: " - + version - + ", version_type: " - + versionType, + "cannot remove metadata field [" + metadataFieldName + "]", IllegalArgumentException.class, () -> processor.execute(ingestDocument) ); - } else { - processor.execute(ingestDocument); - assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); - } - } else if (metadataFieldName.equals(IngestDocument.Metadata.ROUTING.getFieldName()) - && ingestDocument.hasField(IngestDocument.Metadata.ROUTING.getFieldName())) { - processor.execute(ingestDocument); - assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); - } + } else if (metadataFieldName.equals(IngestDocument.Metadata.ID.getFieldName())) { + Long version = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); + String versionType = ingestDocument.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); + if (!versionType.equals(VersionType.toString(VersionType.INTERNAL))) { + assertThrows( + "cannot remove metadata field [_id] when specifying external version for the document, version: " + + version + + ", version_type: " + + versionType, + IllegalArgumentException.class, + () -> processor.execute(ingestDocument) + ); + } else { + processor.execute(ingestDocument); + assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); + } + } else if (metadataFieldName.equals(IngestDocument.Metadata.ROUTING.getFieldName()) + && ingestDocument.hasField(IngestDocument.Metadata.ROUTING.getFieldName())) { + processor.execute(ingestDocument); + assertThat(ingestDocument.hasField(metadataFieldName), equalTo(false)); + } + } } } From 079d65eca6f351012ca74e90fd857f9845bd7feb Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Tue, 21 Nov 2023 12:19:59 +0800 Subject: [PATCH 10/10] Optimize some code Signed-off-by: Gao Binlong --- .../java/org/opensearch/ingest/common/RemoveProcessor.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 index e7732180f7336..bb3d4bca47859 100644 --- 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 @@ -44,6 +44,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.stream.Collectors; /** @@ -87,11 +88,10 @@ 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()) - && !document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class) - .equals(VersionType.toString(VersionType.INTERNAL))) { + && !Objects.equals(versionType, VersionType.toString(VersionType.INTERNAL))) { Long version = document.getFieldValue(IngestDocument.Metadata.VERSION.getFieldName(), Long.class); - String versionType = document.getFieldValue(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), String.class); throw new IllegalArgumentException( "cannot remove metadata field [_id] when specifying external version for the document, version: " + version