Skip to content

Correctly handle nulls in nested paths in the remove processor #126417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 7, 2025
Merged

Correctly handle nulls in nested paths in the remove processor #126417

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c134b04
Conserve precious characters
joegallo Apr 7, 2025
242dbe6
Add failing tests
joegallo Apr 7, 2025
4fe651c
Correcly handle nulls during path traversal
joegallo Apr 7, 2025
b7eb098
Update docs/changelog/126417.yaml
joegallo Apr 7, 2025
8c8437f
Remove some accidental code
joegallo Apr 7, 2025
760e13a
Merge branch 'main' into remove-processor-null-in-path
joegallo Apr 7, 2025
e9aad07
Merge branch 'main' into remove-processor-null-in-path
joegallo Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/126417.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 126417
summary: Correctly handle nulls in nested paths in the remove processor
area: Ingest Node
type: bug
issues: []
14 changes: 7 additions & 7 deletions modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,24 +96,24 @@ public Factory(ScriptService scriptService) {
@Override
public RemoveProcessor create(
Map<String, Processor.Factory> registry,
String processorTag,
String tag,
String description,
Map<String, Object> config,
ProjectId projectId
) throws Exception {
final List<TemplateScript.Factory> compiledTemplatesToRemove = getTemplates(processorTag, config, "field");
final List<TemplateScript.Factory> compiledTemplatesToKeep = getTemplates(processorTag, config, "keep");
final List<TemplateScript.Factory> compiledTemplatesToRemove = getTemplates(tag, config, "field");
final List<TemplateScript.Factory> 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<TemplateScript.Factory> getTemplates(String processorTag, Map<String, Object> config, String propertyName) {
Expand Down
139 changes: 87 additions & 52 deletions ...les/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,48 +22,101 @@
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,
List.of(new TestTemplateService.MockTemplateScript.Factory(field)),
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<String, Object> config = new HashMap<>();
config.put("field", fieldName);
String processorTag = randomAlphaOfLength(10);
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config, null);
String tag = randomAlphaOfLength(10);
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, tag, null, config, null);
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 + "]"));
}
}

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<String, Object> 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, null);
processor.execute(ingestDocument);
String tag = randomAlphaOfLength(10);
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, tag, null, config, null);
processor.execute(document);
}

public void testIgnoreMissingAndNullInPath() throws Exception {
Map<String, Object> source = new HashMap<>();
Map<String, Object> some = new HashMap<>();
Map<String, Object> map = new HashMap<>();
Map<String, Object> 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);
}
}

if (randomBoolean()) {
source.put("some", null);
} else {
some.put("map", null);
source.put("some", some);
}
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source);
Map<String, Object> 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, null);
processor.execute(document);
assertThat(document.hasField("some.map.path"), is(false));
}

public void testKeepFields() throws Exception {
Expand All @@ -76,24 +129,24 @@ 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<TemplateScript.Factory> fieldsToKeep = List.of(
new TestTemplateService.MockTemplateScript.Factory("name"),
new TestTemplateService.MockTemplateScript.Factory("address.street")
);

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) {
Expand All @@ -105,55 +158,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
)
);
}
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/ingest/IngestDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,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)) {
Expand Down
10 changes: 8 additions & 2 deletions server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading