diff --git a/docs/changelog/136649.yaml b/docs/changelog/136649.yaml new file mode 100644 index 0000000000000..5fa9e47f131c0 --- /dev/null +++ b/docs/changelog/136649.yaml @@ -0,0 +1,5 @@ +pr: 136649 +summary: Fix append processor `ignore_empty_values` edge case +area: Ingest Node +type: bug +issues: [] diff --git a/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/380_append_edge_cases.yml b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/380_append_edge_cases.yml new file mode 100644 index 0000000000000..9551d01890e5f --- /dev/null +++ b/modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/380_append_edge_cases.yml @@ -0,0 +1,236 @@ +--- +setup: + - requires: + cluster_features: [ "ingest.append.ignore_empty_values_fix" ] + reason: "These tests all verify that a bug in ignore_empty_values has been fixed" + - do: + ingest.put_pipeline: + id: "test-pipeline-1" + body: > + { + "processors": [ + { + "append": { + "if": "ctx?.templating != true", + "field": "foo", + "copy_from": "bar", + "allow_duplicates": false, + "ignore_empty_values": true + } + }, + { + "append": { + "if": "ctx?.templating == true", + "field": "foo", + "value": "{{bar}}", + "allow_duplicates": true, + "ignore_empty_values": true + } + }, + { + "remove": { + "field": "bar", + "ignore_missing": true + } + }, + { + "script": { + "source": "def foo = ctx.foo; ctx.foo_is_null = (foo == null);" + } + } + ] + } + - do: + indices.create: + index: "test-some-index" + +--- +teardown: + - do: + indices.delete: + index: "test-some-index" + ignore_unavailable: true + - do: + ingest.delete_pipeline: + id: "test-pipeline-1" + ignore: 404 + +--- +"scalar values will become a list if necessary": + - do: + index: + index: test-some-index + id: 1 + pipeline: test-pipeline-1 + body: > + { + "foo": "2", + "bar": "3", + "templating": false + } + + - do: + get: + index: test-some-index + id: "1" + - match: { _source.foo: ["2", "3"] } + + - do: + index: + index: test-some-index + id: 2 + pipeline: test-pipeline-1 + body: > + { + "foo": "2", + "bar": "2", + "templating": true + } + + - do: + get: + index: test-some-index + id: "2" + - match: { _source.foo: ["2", "2"] } + + - do: + index: + index: test-some-index + id: 3 + pipeline: test-pipeline-1 + body: > + { + "foo": "2", + "bar": "3", + "templating": true + } + + - do: + get: + index: test-some-index + id: "3" + - match: { _source.foo: ["2", "3"] } + +--- +"if a value doesn't change because of ignoring duplicates or empty values, it doesn't become a list": + - do: + index: + index: test-some-index + id: 1 + pipeline: test-pipeline-1 + body: > + { + "foo": "2", + "bar": "2", + "templating": false + } + + - do: + get: + index: test-some-index + id: "1" + - match: { _source.foo: "2" } + + - do: + index: + index: test-some-index + id: 2 + pipeline: test-pipeline-1 + body: > + { + "foo": "2", + "bar": "", + "templating": false + } + + - do: + get: + index: test-some-index + id: "2" + - match: { _source.foo: "2" } + + - do: + index: + index: test-some-index + id: 3 + pipeline: test-pipeline-1 + body: > + { + "foo": "2", + "bar": "", + "templating": true + } + + - do: + get: + index: test-some-index + id: "3" + - match: { _source.foo: "2" } + +--- +"an absent value isn't added in the absence of change": + - do: + index: + index: test-some-index + id: 1 + pipeline: test-pipeline-1 + body: > + { + "bar": "", + "templating": false + } + + - do: + get: + index: test-some-index + id: "1" + - match: { _source.foo_is_null: true } + + - do: + index: + index: test-some-index + id: 2 + pipeline: test-pipeline-1 + body: > + { + "bar": "", + "templating": true + } + + - do: + get: + index: test-some-index + id: "2" + - match: { _source.foo_is_null: true } + + - do: + index: + index: test-some-index + id: 3 + pipeline: test-pipeline-1 + body: > + { + "templating": false + } + + - do: + get: + index: test-some-index + id: "3" + - match: { _source.foo_is_null: true } + + - do: + index: + index: test-some-index + id: 4 + pipeline: test-pipeline-1 + body: > + { + "templating": true + } + + - do: + get: + index: test-some-index + id: "4" + - match: { _source.foo_is_null: true } diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index e79accd758dc7..d0d7687fcbde3 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -879,7 +879,9 @@ private void setFieldValue(String path, Object value, boolean append, boolean al if (object == NOT_FOUND) { List list = new ArrayList<>(); appendValues(list, value, allowDuplicates, ignoreEmptyValues); - map.put(leafKey, list); + if (list.isEmpty() == false) { + map.put(leafKey, list); + } } else { Object list = appendValues(object, value, allowDuplicates, ignoreEmptyValues); if (list != object) { @@ -897,7 +899,9 @@ private void setFieldValue(String path, Object value, boolean append, boolean al if (object == NOT_FOUND) { List list = new ArrayList<>(); appendValues(list, value, allowDuplicates, ignoreEmptyValues); - map.put(leafKey, list); + if (list.isEmpty() == false) { + map.put(leafKey, list); + } } else { Object list = appendValues(object, value, allowDuplicates, ignoreEmptyValues); if (list != object) { @@ -949,49 +953,24 @@ private static Object appendValues(Object maybeList, Object value, boolean allow list = new ArrayList<>(); list.add(maybeList); } - if (allowDuplicates) { - innerAppendValues(list, value, ignoreEmptyValues); - return list; - } else { - // if no values were appended due to duplication, return the original object so the ingest document remains unmodified - return innerAppendValuesWithoutDuplicates(list, value, ignoreEmptyValues) ? list : maybeList; - } - } - - // helper method for use in appendValues above, please do not call this directly except from that method - private static void innerAppendValues(List list, Object value, boolean ignoreEmptyValues) { - if (value instanceof List l) { - if (ignoreEmptyValues) { - l.forEach((v) -> { - if (valueNotEmpty(v)) { - list.add(v); - } - }); - } else { - list.addAll(l); - } - } else if (ignoreEmptyValues == false || valueNotEmpty(value)) { - list.add(value); - } - } - // helper method for use in appendValues above, please do not call this directly except from that method - private static boolean innerAppendValuesWithoutDuplicates(List list, Object value, boolean ignoreEmptyValues) { boolean valuesWereAppended = false; if (value instanceof List valueList) { for (Object val : valueList) { - if (list.contains(val) == false && (ignoreEmptyValues == false || valueNotEmpty(val))) { + if ((allowDuplicates || list.contains(val) == false) && (ignoreEmptyValues == false || valueNotEmpty(val))) { list.add(val); valuesWereAppended = true; } } } else { - if (list.contains(value) == false && (ignoreEmptyValues == false || valueNotEmpty(value))) { + if ((allowDuplicates || list.contains(value) == false) && (ignoreEmptyValues == false || valueNotEmpty(value))) { list.add(value); valuesWereAppended = true; } } - return valuesWereAppended; + + // if no values were appended due to duplication/empties, return the original object so the ingest document remains unmodified + return valuesWereAppended ? list : maybeList; } private static boolean valueNotEmpty(Object value) { diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestFeatures.java b/server/src/main/java/org/elasticsearch/ingest/IngestFeatures.java index 240e68468f1eb..5567a0553f754 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestFeatures.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestFeatures.java @@ -19,6 +19,7 @@ public class IngestFeatures implements FeatureSpecification { private static final NodeFeature INGEST_APPEND_COPY_FROM = new NodeFeature("ingest.append.copy_from", true); private static final NodeFeature INGEST_APPEND_IGNORE_EMPTY_VALUES = new NodeFeature("ingest.append.ignore_empty_values", true); private static final NodeFeature RANDOM_SAMPLING = new NodeFeature("random_sampling", true); + private static final NodeFeature INGEST_APPEND_IGNORE_EMPTY_VALUES_FIX = new NodeFeature("ingest.append.ignore_empty_values_fix", true); @Override public Set getFeatures() { @@ -27,6 +28,12 @@ public Set getFeatures() { @Override public Set getTestFeatures() { - return Set.of(SIMULATE_INGEST_400_ON_FAILURE, INGEST_APPEND_COPY_FROM, INGEST_APPEND_IGNORE_EMPTY_VALUES, RANDOM_SAMPLING); + return Set.of( + SIMULATE_INGEST_400_ON_FAILURE, + INGEST_APPEND_COPY_FROM, + INGEST_APPEND_IGNORE_EMPTY_VALUES, + RANDOM_SAMPLING, + INGEST_APPEND_IGNORE_EMPTY_VALUES_FIX + ); } }