Skip to content

Commit bead858

Browse files
joegallojoegallo
authored
Correctly handle nulls in nested paths in the remove processor (#126417)
1 parent 7929743 commit bead858

File tree

5 files changed

+101
-62
lines changed

5 files changed

+101
-62
lines changed

‎docs/changelog/126417.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 126417
2+
summary: Correctly handle nulls in nested paths in the remove processor
3+
area: Ingest Node
4+
type: bug
5+
issues: []

‎modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -96,24 +96,24 @@ public Factory(ScriptService scriptService) {
9696
@Override
9797
public RemoveProcessor create(
9898
Map<String, Processor.Factory> registry,
99-
String processorTag,
99+
String tag,
100100
String description,
101101
Map<String, Object> config,
102102
ProjectId projectId
103103
) throws Exception {
104-
final List<TemplateScript.Factory> compiledTemplatesToRemove = getTemplates(processorTag, config, "field");
105-
final List<TemplateScript.Factory> compiledTemplatesToKeep = getTemplates(processorTag, config, "keep");
104+
final List<TemplateScript.Factory> compiledTemplatesToRemove = getTemplates(tag, config, "field");
105+
final List<TemplateScript.Factory> compiledTemplatesToKeep = getTemplates(tag, config, "keep");
106106

107107
if (compiledTemplatesToRemove.isEmpty() && compiledTemplatesToKeep.isEmpty()) {
108-
throw newConfigurationException(TYPE, processorTag, "keep", "or [field] must be specified");
108+
throw newConfigurationException(TYPE, tag, "keep", "or [field] must be specified");
109109
}
110110

111111
if (compiledTemplatesToRemove.isEmpty() == false && compiledTemplatesToKeep.isEmpty() == false) {
112-
throw newConfigurationException(TYPE, processorTag, "keep", "and [field] cannot both be used in the same processor");
112+
throw newConfigurationException(TYPE, tag, "keep", "and [field] cannot both be used in the same processor");
113113
}
114114

115-
boolean ignoreMissing = ConfigurationUtils.readBooleanProperty(TYPE, processorTag, config, "ignore_missing", false);
116-
return new RemoveProcessor(processorTag, description, compiledTemplatesToRemove, compiledTemplatesToKeep, ignoreMissing);
115+
boolean ignoreMissing = ConfigurationUtils.readBooleanProperty(TYPE, tag, config, "ignore_missing", false);
116+
return new RemoveProcessor(tag, description, compiledTemplatesToRemove, compiledTemplatesToKeep, ignoreMissing);
117117
}
118118

119119
private List<TemplateScript.Factory> getTemplates(String processorTag, Map<String, Object> config, String propertyName) {

‎modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RemoveProcessorTests.java‎

Lines changed: 80 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -22,48 +22,94 @@
2222
import java.util.Map;
2323

2424
import static org.hamcrest.Matchers.containsString;
25-
import static org.hamcrest.Matchers.equalTo;
25+
import static org.hamcrest.Matchers.is;
2626

2727
public class RemoveProcessorTests extends ESTestCase {
2828

2929
public void testRemoveFields() throws Exception {
30-
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
31-
String field = RandomDocumentPicks.randomExistingFieldName(random(), ingestDocument);
30+
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random());
31+
String field = RandomDocumentPicks.randomExistingFieldName(random(), document);
3232
Processor processor = new RemoveProcessor(
3333
randomAlphaOfLength(10),
3434
null,
3535
List.of(new TestTemplateService.MockTemplateScript.Factory(field)),
3636
List.of(),
3737
false
3838
);
39-
processor.execute(ingestDocument);
40-
assertThat(ingestDocument.hasField(field), equalTo(false));
39+
processor.execute(document);
40+
assertThat(document.hasField(field), is(false));
4141
}
4242

4343
public void testRemoveNonExistingField() throws Exception {
44-
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>());
44+
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>());
4545
String fieldName = RandomDocumentPicks.randomFieldName(random());
4646
Map<String, Object> config = new HashMap<>();
4747
config.put("field", fieldName);
48-
String processorTag = randomAlphaOfLength(10);
49-
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config, null);
48+
String tag = randomAlphaOfLength(10);
49+
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, tag, null, config, null);
5050
try {
51-
processor.execute(ingestDocument);
51+
processor.execute(document);
5252
fail("remove field should have failed");
5353
} catch (IllegalArgumentException e) {
5454
assertThat(e.getMessage(), containsString("not present as part of path [" + fieldName + "]"));
5555
}
5656
}
5757

5858
public void testIgnoreMissing() throws Exception {
59-
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>());
59+
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>());
6060
String fieldName = RandomDocumentPicks.randomFieldName(random());
6161
Map<String, Object> config = new HashMap<>();
6262
config.put("field", fieldName);
6363
config.put("ignore_missing", true);
64-
String processorTag = randomAlphaOfLength(10);
65-
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, processorTag, null, config, null);
66-
processor.execute(ingestDocument);
64+
String tag = randomAlphaOfLength(10);
65+
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, tag, null, config, null);
66+
processor.execute(document);
67+
}
68+
69+
public void testIgnoreMissingAndNullInPath() throws Exception {
70+
Map<String, Object> source = new HashMap<>();
71+
Map<String, Object> some = new HashMap<>();
72+
Map<String, Object> map = new HashMap<>();
73+
Map<String, Object> path = new HashMap<>();
74+
75+
switch (randomIntBetween(0, 6)) {
76+
case 0 -> {
77+
// empty source
78+
}
79+
case 1 -> {
80+
source.put("some", null);
81+
}
82+
case 2 -> {
83+
some.put("map", null);
84+
source.put("some", some);
85+
}
86+
case 3 -> {
87+
some.put("map", map);
88+
source.put("some", some);
89+
}
90+
case 4 -> {
91+
map.put("path", null);
92+
some.put("map", map);
93+
source.put("some", some);
94+
}
95+
case 5 -> {
96+
map.put("path", path);
97+
some.put("map", map);
98+
source.put("some", some);
99+
}
100+
case 6 -> {
101+
map.put("path", "foobar");
102+
some.put("map", map);
103+
source.put("some", some);
104+
}
105+
}
106+
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source);
107+
Map<String, Object> config = new HashMap<>();
108+
config.put("field", "some.map.path");
109+
config.put("ignore_missing", true);
110+
Processor processor = new RemoveProcessor.Factory(TestTemplateService.instance()).create(null, null, null, config, null);
111+
processor.execute(document);
112+
assertThat(document.hasField("some.map.path"), is(false));
67113
}
68114

69115
public void testKeepFields() throws Exception {
@@ -76,24 +122,24 @@ public void testKeepFields() throws Exception {
76122
source.put("age", 55);
77123
source.put("address", address);
78124

79-
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), source);
125+
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source);
80126

81127
List<TemplateScript.Factory> fieldsToKeep = List.of(
82128
new TestTemplateService.MockTemplateScript.Factory("name"),
83129
new TestTemplateService.MockTemplateScript.Factory("address.street")
84130
);
85131

86132
Processor processor = new RemoveProcessor(randomAlphaOfLength(10), null, new ArrayList<>(), fieldsToKeep, false);
87-
processor.execute(ingestDocument);
88-
assertTrue(ingestDocument.hasField("name"));
89-
assertTrue(ingestDocument.hasField("address"));
90-
assertTrue(ingestDocument.hasField("address.street"));
91-
assertFalse(ingestDocument.hasField("age"));
92-
assertFalse(ingestDocument.hasField("address.number"));
93-
assertTrue(ingestDocument.hasField("_index"));
94-
assertTrue(ingestDocument.hasField("_version"));
95-
assertTrue(ingestDocument.hasField("_id"));
96-
assertTrue(ingestDocument.hasField("_version_type"));
133+
processor.execute(document);
134+
assertTrue(document.hasField("name"));
135+
assertTrue(document.hasField("address"));
136+
assertTrue(document.hasField("address.street"));
137+
assertFalse(document.hasField("age"));
138+
assertFalse(document.hasField("address.number"));
139+
assertTrue(document.hasField("_index"));
140+
assertTrue(document.hasField("_version"));
141+
assertTrue(document.hasField("_id"));
142+
assertTrue(document.hasField("_version_type"));
97143
}
98144

99145
public void testShouldKeep(String a, String b) {
@@ -105,55 +151,37 @@ public void testShouldKeep(String a, String b) {
105151
source.put("name", "eric clapton");
106152
source.put("address", address);
107153

108-
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), source);
154+
IngestDocument document = RandomDocumentPicks.randomIngestDocument(random(), source);
109155

110-
assertTrue(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("name")), ingestDocument));
156+
assertTrue(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("name")), document));
111157

112-
assertTrue(RemoveProcessor.shouldKeep("age", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), ingestDocument));
158+
assertTrue(RemoveProcessor.shouldKeep("age", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), document));
113159

114-
assertFalse(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), ingestDocument));
160+
assertFalse(RemoveProcessor.shouldKeep("name", List.of(new TestTemplateService.MockTemplateScript.Factory("age")), document));
115161

116162
assertTrue(
117-
RemoveProcessor.shouldKeep(
118-
"address",
119-
List.of(new TestTemplateService.MockTemplateScript.Factory("address.street")),
120-
ingestDocument
121-
)
163+
RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address.street")), document)
122164
);
123165

124166
assertTrue(
125-
RemoveProcessor.shouldKeep(
126-
"address",
127-
List.of(new TestTemplateService.MockTemplateScript.Factory("address.number")),
128-
ingestDocument
129-
)
167+
RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address.number")), document)
130168
);
131169

132170
assertTrue(
133-
RemoveProcessor.shouldKeep(
134-
"address.street",
135-
List.of(new TestTemplateService.MockTemplateScript.Factory("address")),
136-
ingestDocument
137-
)
171+
RemoveProcessor.shouldKeep("address.street", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), document)
138172
);
139173

140174
assertTrue(
141-
RemoveProcessor.shouldKeep(
142-
"address.number",
143-
List.of(new TestTemplateService.MockTemplateScript.Factory("address")),
144-
ingestDocument
145-
)
175+
RemoveProcessor.shouldKeep("address.number", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), document)
146176
);
147177

148-
assertTrue(
149-
RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), ingestDocument)
150-
);
178+
assertTrue(RemoveProcessor.shouldKeep("address", List.of(new TestTemplateService.MockTemplateScript.Factory("address")), document));
151179

152180
assertFalse(
153181
RemoveProcessor.shouldKeep(
154182
"address.street",
155183
List.of(new TestTemplateService.MockTemplateScript.Factory("address.number")),
156-
ingestDocument
184+
document
157185
)
158186
);
159187
}

‎server/src/main/java/org/elasticsearch/ingest/IngestDocument.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ public void removeField(String path, boolean ignoreMissing) {
345345
}
346346

347347
String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1];
348-
if (context == null) {
348+
if (context == null && ignoreMissing == false) {
349349
throw new IllegalArgumentException(Errors.cannotRemove(path, leafKey, null));
350350
} else if (context instanceof IngestCtxMap map) { // optimization: handle IngestCtxMap separately from Map
351351
if (map.containsKey(leafKey)) {

‎server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -860,8 +860,14 @@ public void testRemoveFieldIgnoreMissing() {
860860

861861
// if ignoreMissing is false, we throw an exception for values that aren't found
862862
IllegalArgumentException e;
863-
e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
864-
assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]"));
863+
if (randomBoolean()) {
864+
document.setFieldValue("fizz.some", (Object) null);
865+
e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
866+
assertThat(e.getMessage(), is("cannot remove [nonsense] from null as part of path [fizz.some.nonsense]"));
867+
} else {
868+
e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
869+
assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]"));
870+
}
865871

866872
// but no exception is thrown if ignoreMissing is true
867873
document.removeField("fizz.some.nonsense", true);

0 commit comments

Comments
 (0)