diff --git a/docs/changelog/126417.yaml b/docs/changelog/126417.yaml new file mode 100644 index 0000000000000..8fd456becacdb --- /dev/null +++ b/docs/changelog/126417.yaml @@ -0,0 +1,5 @@ +pr: 126417 +summary: Correctly handle nulls in nested paths in the remove processor +area: Ingest Node +type: bug +issues: [] diff --git a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java index 2c64d41b77d74..215f2ac569dd5 100644 --- a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java +++ b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java @@ -93,25 +93,21 @@ public Factory(ScriptService scriptService) { } @Override - public RemoveProcessor create( - Map registry, - String processorTag, - String description, - Map config - ) throws Exception { - final List compiledTemplatesToRemove = getTemplates(processorTag, config, "field"); - final List compiledTemplatesToKeep = getTemplates(processorTag, config, "keep"); + public RemoveProcessor create(Map registry, String tag, String description, Map config) + throws Exception { + final List compiledTemplatesToRemove = getTemplates(tag, config, "field"); + final List compiledTemplatesToKeep = getTemplates(tag, config, "keep"); if (compiledTemplatesToRemove.isEmpty() && compiledTemplatesToKeep.isEmpty()) { - throw newConfigurationException(TYPE, processorTag, "keep", "or [field] must be specified"); + throw newConfigurationException(TYPE, tag, "keep", "or [field] must be specified"); } if (compiledTemplatesToRemove.isEmpty() == false && compiledTemplatesToKeep.isEmpty() == false) { - throw newConfigurationException(TYPE, processorTag, "keep", "and [field] cannot both be used in the same processor"); + throw newConfigurationException(TYPE, tag, "keep", "and [field] cannot both be used in the same processor"); } - boolean ignoreMissing = ConfigurationUtils.readBooleanProperty(TYPE, processorTag, config, "ignore_missing", false); - return new RemoveProcessor(processorTag, description, compiledTemplatesToRemove, compiledTemplatesToKeep, ignoreMissing); + boolean ignoreMissing = ConfigurationUtils.readBooleanProperty(TYPE, tag, config, "ignore_missing", false); + return new RemoveProcessor(tag, description, compiledTemplatesToRemove, compiledTemplatesToKeep, ignoreMissing); } private List getTemplates(String processorTag, Map config, String propertyName) { 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 b18a7ff26e30e..2745c4229675d 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 @@ -22,13 +22,13 @@ import java.util.Map; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; public class RemoveProcessorTests extends ESTestCase { public void testRemoveFields() throws Exception { - IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random()); - String field = RandomDocumentPicks.randomExistingFieldName(random(), ingestDocument); + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random()); + String field = RandomDocumentPicks.randomExistingFieldName(random(), document); Processor processor = new RemoveProcessor( randomAlphaOfLength(10), null, @@ -36,19 +36,19 @@ public void testRemoveFields() throws Exception { List.of(), false ); - processor.execute(ingestDocument); - assertThat(ingestDocument.hasField(field), equalTo(false)); + processor.execute(document); + assertThat(document.hasField(field), is(false)); } public void testRemoveNonExistingField() throws Exception { - IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); String fieldName = RandomDocumentPicks.randomFieldName(random()); Map config = new HashMap<>(); config.put("field", fieldName); - String processorTag = randomAlphaOfLength(10); - Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config); + String tag = randomAlphaOfLength(10); + Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, tag, null, config); try { - processor.execute(ingestDocument); + processor.execute(document); fail("remove field should have failed"); } catch (IllegalArgumentException e) { assertThat(e.getMessage(), containsString("not present as part of path [" + fieldName + "]")); @@ -56,14 +56,60 @@ public void testRemoveNonExistingField() throws Exception { } public void testIgnoreMissing() throws Exception { - IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); String fieldName = RandomDocumentPicks.randomFieldName(random()); Map config = new HashMap<>(); config.put("field", fieldName); config.put("ignore_missing", true); - String processorTag = randomAlphaOfLength(10); - Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config); - processor.execute(ingestDocument); + String tag = randomAlphaOfLength(10); + Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, tag, null, config); + processor.execute(document); + } + + public void testIgnoreMissingAndNullInPath() throws Exception { + Map source = new HashMap<>(); + Map some = new HashMap<>(); + Map map = new HashMap<>(); + Map path = new HashMap<>(); + + switch (randomIntBetween(0, 6)) { + case 0 -> { + // empty source + } + case 1 -> { + source.put("some", null); + } + case 2 -> { + some.put("map", null); + source.put("some", some); + } + case 3 -> { + some.put("map", map); + source.put("some", some); + } + case 4 -> { + map.put("path", null); + some.put("map", map); + source.put("some", some); + } + case 5 -> { + map.put("path", path); + some.put("map", map); + source.put("some", some); + } + case 6 -> { + map.put("path", "foobar"); + some.put("map", map); + source.put("some", some); + } + } + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source); + Map config = new HashMap<>(); + config.put("field", "some.map.path"); + config.put("ignore_missing", true); + Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, null, null, config); + processor.execute(document); + assertThat(document.hasField("some.map.path"), is(false)); } public void testKeepFields() throws Exception { @@ -76,7 +122,7 @@ public void testKeepFields() throws Exception { source.put("age", 55); source.put("address", address); - IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), source); + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source); List fieldsToKeep = List.of( new TestTemplateService.MockTemplateScript.Factory("name"), @@ -84,16 +130,16 @@ public void testKeepFields() throws Exception { ); Processor processor = new RemoveProcessor(randomAlphaOfLength(10), null, new ArrayList<>(), fieldsToKeep, false); - processor.execute(ingestDocument); - assertTrue(ingestDocument.hasField("name")); - assertTrue(ingestDocument.hasField("address")); - assertTrue(ingestDocument.hasField("address.street")); - assertFalse(ingestDocument.hasField("age")); - assertFalse(ingestDocument.hasField("address.number")); - assertTrue(ingestDocument.hasField("_index")); - assertTrue(ingestDocument.hasField("_version")); - assertTrue(ingestDocument.hasField("_id")); - assertTrue(ingestDocument.hasField("_version_type")); + processor.execute(document); + assertTrue(document.hasField("name")); + assertTrue(document.hasField("address")); + assertTrue(document.hasField("address.street")); + assertFalse(document.hasField("age")); + assertFalse(document.hasField("address.number")); + assertTrue(document.hasField("_index")); + assertTrue(document.hasField("_version")); + assertTrue(document.hasField("_id")); + assertTrue(document.hasField("_version_type")); } public void testShouldKeep(String a, String b) { @@ -105,55 +151,37 @@ public void testShouldKeep(String a, String b) { source.put("name", "eric clapton"); source.put("address", address); - IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), source); + IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source); - assertTrue(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("name")), ingestDocument)); + assertTrue(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("name")), document)); - assertTrue(RemoveProcessor.shouldKeep("age", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), ingestDocument)); + assertTrue(RemoveProcessor.shouldKeep("age", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), document)); - assertFalse(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), ingestDocument)); + assertFalse(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), document)); assertTrue( - RemoveProcessor.shouldKeep( - "address", - List.of(new TestTemplateService.MockTemplateScript.Factory("address.street")), - ingestDocument - ) + RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address.street")), document) ); assertTrue( - RemoveProcessor.shouldKeep( - "address", - List.of(new TestTemplateService.MockTemplateScript.Factory("address.number")), - ingestDocument - ) + RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address.number")), document) ); assertTrue( - RemoveProcessor.shouldKeep( - "address.street", - List.of(new TestTemplateService.MockTemplateScript.Factory("address")), - ingestDocument - ) + RemoveProcessor.shouldKeep("address.street", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), document) ); assertTrue( - RemoveProcessor.shouldKeep( - "address.number", - List.of(new TestTemplateService.MockTemplateScript.Factory("address")), - ingestDocument - ) + RemoveProcessor.shouldKeep("address.number", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), document) ); - assertTrue( - RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), ingestDocument) - ); + assertTrue(RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), document)); assertFalse( RemoveProcessor.shouldKeep( "address.street", List.of(new TestTemplateService.MockTemplateScript.Factory("address.number")), - ingestDocument + document ) ); } diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 321fa7486b1b6..0d2314411bd92 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -344,7 +344,7 @@ public void removeField(String path, boolean ignoreMissing) { } String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1]; - if (context == null) { + if (context == null && ignoreMissing == false) { throw new IllegalArgumentException(Errors.cannotRemove(path, leafKey, null)); } else if (context instanceof IngestCtxMap map) { // optimization: handle IngestCtxMap separately from Map if (map.containsKey(leafKey)) { diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java index b7120eec3d252..1723eec384977 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java @@ -860,8 +860,14 @@ public void testRemoveFieldIgnoreMissing() { // if ignoreMissing is false, we throw an exception for values that aren't found IllegalArgumentException e; - 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]")); + 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]")); + } // but no exception is thrown if ignoreMissing is true document.removeField("fizz.some.nonsense", true);