diff --git a/docs/changelog/127006.yaml b/docs/changelog/127006.yaml new file mode 100644 index 0000000000000..fa41bce2791f6 --- /dev/null +++ b/docs/changelog/127006.yaml @@ -0,0 +1,5 @@ +pr: 127006 +summary: Correctly handle non-integers in nested paths in the remove processor +area: Ingest Node +type: bug +issues: [] diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java index 0b598c4528ef4..72819ad506745 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java @@ -16,6 +16,7 @@ import org.elasticsearch.script.TemplateScript; import org.elasticsearch.test.ESTestCase; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -103,6 +104,7 @@ public void testIgnoreMissingAndNullInPath() throws Exception { some.put("map", map); source.put("some", some); } + default -> throw new AssertionError("failure, got illegal switch case"); } IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source); Map config = new HashMap<>(); @@ -113,6 +115,53 @@ public void testIgnoreMissingAndNullInPath() throws Exception { assertThat(document.hasField("some.map.path"), is(false)); } + public void testIgnoreMissingAndNonIntegerInPath() throws Exception { + Map source = new HashMap<>(); + Map some = new HashMap<>(); + List array = new ArrayList<>(); + Map path = new HashMap<>(); + + switch (randomIntBetween(0, 6)) { + case 0 -> { + // empty source + } + case 1 -> { + source.put("some", null); + } + case 2 -> { + some.put("array", null); + source.put("some", some); + } + case 3 -> { + some.put("array", array); + source.put("some", some); + } + case 4 -> { + array.add(null); + some.put("array", array); + source.put("some", some); + } + case 5 -> { + array.add(path); + some.put("array", array); + source.put("some", some); + } + case 6 -> { + array.add("foobar"); + some.put("array", array); + source.put("some", some); + } + default -> throw new AssertionError("failure, got illegal switch case"); + } + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source); + Map config = new HashMap<>(); + config.put("field", "some.array.path"); + config.put("ignore_missing", true); + Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, null, null, config, null); + processor.execute(document); + assertThat(document.hasField("some.array.path"), is(false)); + } + public void testKeepFields() throws Exception { Map address = new HashMap<>(); address.put("street", "Ipiranga Street"); diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 235f1efd4b348..2d03460fe43ed 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -360,11 +360,13 @@ public void removeField(String path, boolean ignoreMissing) { throw new IllegalArgumentException(Errors.notPresent(path, leafKey)); } } else if (context instanceof List list) { - int index; + int index = -1; try { index = Integer.parseInt(leafKey); } catch (NumberFormatException e) { - throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e); + if (ignoreMissing == false) { + throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e); + } } if (index < 0 || index >= list.size()) { if (ignoreMissing == false) { diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java index 1723eec384977..a0152ea63ef53 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java @@ -860,13 +860,25 @@ public void testRemoveFieldIgnoreMissing() { // if ignoreMissing is false, we throw an exception for values that aren't found IllegalArgumentException e; - if (randomBoolean()) { - document.setFieldValue("fizz.some", (Object) null); - e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false)); - assertThat(e.getMessage(), is("cannot remove [nonsense] from null as part of path [fizz.some.nonsense]")); - } else { - e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false)); - assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]")); + switch (randomIntBetween(0, 2)) { + case 0 -> { + document.setFieldValue("fizz.some", (Object) null); + e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false)); + assertThat(e.getMessage(), is("cannot remove [nonsense] from null as part of path [fizz.some.nonsense]")); + } + case 1 -> { + document.setFieldValue("fizz.some", List.of("foo", "bar")); + e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false)); + assertThat( + e.getMessage(), + is("[nonsense] is not an integer, cannot be used as an index as part of path [fizz.some.nonsense]") + ); + } + case 2 -> { + e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false)); + assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]")); + } + default -> throw new AssertionError("failure, got illegal switch case"); } // but no exception is thrown if ignoreMissing is true