Skip to content

Commit bd0c079

Browse files
joegallojoegallo
authored
Fix append processor ignore_empty_values edge case (#136649) (#136659)
1 parent 4f05a9c commit bd0c079

File tree

4 files changed

+259
-33
lines changed

4 files changed

+259
-33
lines changed

‎docs/changelog/136649.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 136649
2+
summary: Fix append processor `ignore_empty_values` edge case
3+
area: Ingest Node
4+
type: bug
5+
issues: []

‎modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/380_append_edge_cases.yml‎

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
---
2+
setup:
3+
- requires:
4+
cluster_features: [ "ingest.append.ignore_empty_values_fix" ]
5+
reason: "These tests all verify that a bug in ignore_empty_values has been fixed"
6+
- do:
7+
ingest.put_pipeline:
8+
id: "test-pipeline-1"
9+
body: >
10+
{
11+
"processors": [
12+
{
13+
"append": {
14+
"if": "ctx?.templating != true",
15+
"field": "foo",
16+
"copy_from": "bar",
17+
"allow_duplicates": false,
18+
"ignore_empty_values": true
19+
}
20+
},
21+
{
22+
"append": {
23+
"if": "ctx?.templating == true",
24+
"field": "foo",
25+
"value": "{{bar}}",
26+
"allow_duplicates": true,
27+
"ignore_empty_values": true
28+
}
29+
},
30+
{
31+
"remove": {
32+
"field": "bar",
33+
"ignore_missing": true
34+
}
35+
},
36+
{
37+
"script": {
38+
"source": "def foo = ctx.foo; ctx.foo_is_null = (foo == null);"
39+
}
40+
}
41+
]
42+
}
43+
- do:
44+
indices.create:
45+
index: "test-some-index"
46+
47+
---
48+
teardown:
49+
- do:
50+
indices.delete:
51+
index: "test-some-index"
52+
ignore_unavailable: true
53+
- do:
54+
ingest.delete_pipeline:
55+
id: "test-pipeline-1"
56+
ignore: 404
57+
58+
---
59+
"scalar values will become a list if necessary":
60+
- do:
61+
index:
62+
index: test-some-index
63+
id: 1
64+
pipeline: test-pipeline-1
65+
body: >
66+
{
67+
"foo": "2",
68+
"bar": "3",
69+
"templating": false
70+
}
71+
72+
- do:
73+
get:
74+
index: test-some-index
75+
id: "1"
76+
- match: { _source.foo: ["2", "3"] }
77+
78+
- do:
79+
index:
80+
index: test-some-index
81+
id: 2
82+
pipeline: test-pipeline-1
83+
body: >
84+
{
85+
"foo": "2",
86+
"bar": "2",
87+
"templating": true
88+
}
89+
90+
- do:
91+
get:
92+
index: test-some-index
93+
id: "2"
94+
- match: { _source.foo: ["2", "2"] }
95+
96+
- do:
97+
index:
98+
index: test-some-index
99+
id: 3
100+
pipeline: test-pipeline-1
101+
body: >
102+
{
103+
"foo": "2",
104+
"bar": "3",
105+
"templating": true
106+
}
107+
108+
- do:
109+
get:
110+
index: test-some-index
111+
id: "3"
112+
- match: { _source.foo: ["2", "3"] }
113+
114+
---
115+
"if a value doesn't change because of ignoring duplicates or empty values, it doesn't become a list":
116+
- do:
117+
index:
118+
index: test-some-index
119+
id: 1
120+
pipeline: test-pipeline-1
121+
body: >
122+
{
123+
"foo": "2",
124+
"bar": "2",
125+
"templating": false
126+
}
127+
128+
- do:
129+
get:
130+
index: test-some-index
131+
id: "1"
132+
- match: { _source.foo: "2" }
133+
134+
- do:
135+
index:
136+
index: test-some-index
137+
id: 2
138+
pipeline: test-pipeline-1
139+
body: >
140+
{
141+
"foo": "2",
142+
"bar": "",
143+
"templating": false
144+
}
145+
146+
- do:
147+
get:
148+
index: test-some-index
149+
id: "2"
150+
- match: { _source.foo: "2" }
151+
152+
- do:
153+
index:
154+
index: test-some-index
155+
id: 3
156+
pipeline: test-pipeline-1
157+
body: >
158+
{
159+
"foo": "2",
160+
"bar": "",
161+
"templating": true
162+
}
163+
164+
- do:
165+
get:
166+
index: test-some-index
167+
id: "3"
168+
- match: { _source.foo: "2" }
169+
170+
---
171+
"an absent value isn't added in the absence of change":
172+
- do:
173+
index:
174+
index: test-some-index
175+
id: 1
176+
pipeline: test-pipeline-1
177+
body: >
178+
{
179+
"bar": "",
180+
"templating": false
181+
}
182+
183+
- do:
184+
get:
185+
index: test-some-index
186+
id: "1"
187+
- match: { _source.foo_is_null: true }
188+
189+
- do:
190+
index:
191+
index: test-some-index
192+
id: 2
193+
pipeline: test-pipeline-1
194+
body: >
195+
{
196+
"bar": "",
197+
"templating": true
198+
}
199+
200+
- do:
201+
get:
202+
index: test-some-index
203+
id: "2"
204+
- match: { _source.foo_is_null: true }
205+
206+
- do:
207+
index:
208+
index: test-some-index
209+
id: 3
210+
pipeline: test-pipeline-1
211+
body: >
212+
{
213+
"templating": false
214+
}
215+
216+
- do:
217+
get:
218+
index: test-some-index
219+
id: "3"
220+
- match: { _source.foo_is_null: true }
221+
222+
- do:
223+
index:
224+
index: test-some-index
225+
id: 4
226+
pipeline: test-pipeline-1
227+
body: >
228+
{
229+
"templating": true
230+
}
231+
232+
- do:
233+
get:
234+
index: test-some-index
235+
id: "4"
236+
- match: { _source.foo_is_null: true }

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

Lines changed: 11 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -879,7 +879,9 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
879879
if (object == NOT_FOUND) {
880880
List<Object> list = new ArrayList<>();
881881
appendValues(list, value, allowDuplicates, ignoreEmptyValues);
882-
map.put(leafKey, list);
882+
if (list.isEmpty() == false) {
883+
map.put(leafKey, list);
884+
}
883885
} else {
884886
Object list = appendValues(object, value, allowDuplicates, ignoreEmptyValues);
885887
if (list != object) {
@@ -897,7 +899,9 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
897899
if (object == NOT_FOUND) {
898900
List<Object> list = new ArrayList<>();
899901
appendValues(list, value, allowDuplicates, ignoreEmptyValues);
900-
map.put(leafKey, list);
902+
if (list.isEmpty() == false) {
903+
map.put(leafKey, list);
904+
}
901905
} else {
902906
Object list = appendValues(object, value, allowDuplicates, ignoreEmptyValues);
903907
if (list != object) {
@@ -949,49 +953,24 @@ private static Object appendValues(Object maybeList, Object value, boolean allow
949953
list = new ArrayList<>();
950954
list.add(maybeList);
951955
}
952-
if (allowDuplicates) {
953-
innerAppendValues(list, value, ignoreEmptyValues);
954-
return list;
955-
} else {
956-
// if no values were appended due to duplication, return the original object so the ingest document remains unmodified
957-
return innerAppendValuesWithoutDuplicates(list, value, ignoreEmptyValues) ? list : maybeList;
958-
}
959-
}
960-
961-
// helper method for use in appendValues above, please do not call this directly except from that method
962-
private static void innerAppendValues(List<Object> list, Object value, boolean ignoreEmptyValues) {
963-
if (value instanceof List<?> l) {
964-
if (ignoreEmptyValues) {
965-
l.forEach((v) -> {
966-
if (valueNotEmpty(v)) {
967-
list.add(v);
968-
}
969-
});
970-
} else {
971-
list.addAll(l);
972-
}
973-
} else if (ignoreEmptyValues == false || valueNotEmpty(value)) {
974-
list.add(value);
975-
}
976-
}
977956

978-
// helper method for use in appendValues above, please do not call this directly except from that method
979-
private static boolean innerAppendValuesWithoutDuplicates(List<Object> list, Object value, boolean ignoreEmptyValues) {
980957
boolean valuesWereAppended = false;
981958
if (value instanceof List<?> valueList) {
982959
for (Object val : valueList) {
983-
if (list.contains(val) == false && (ignoreEmptyValues == false || valueNotEmpty(val))) {
960+
if ((allowDuplicates || list.contains(val) == false) && (ignoreEmptyValues == false || valueNotEmpty(val))) {
984961
list.add(val);
985962
valuesWereAppended = true;
986963
}
987964
}
988965
} else {
989-
if (list.contains(value) == false && (ignoreEmptyValues == false || valueNotEmpty(value))) {
966+
if ((allowDuplicates || list.contains(value) == false) && (ignoreEmptyValues == false || valueNotEmpty(value))) {
990967
list.add(value);
991968
valuesWereAppended = true;
992969
}
993970
}
994-
return valuesWereAppended;
971+
972+
// if no values were appended due to duplication/empties, return the original object so the ingest document remains unmodified
973+
return valuesWereAppended ? list : maybeList;
995974
}
996975

997976
private static boolean valueNotEmpty(Object value) {

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ public class IngestFeatures implements FeatureSpecification {
1818
private static final NodeFeature SIMULATE_INGEST_400_ON_FAILURE = new NodeFeature("simulate.ingest.400_on_failure", true);
1919
private static final NodeFeature INGEST_APPEND_COPY_FROM = new NodeFeature("ingest.append.copy_from", true);
2020
private static final NodeFeature INGEST_APPEND_IGNORE_EMPTY_VALUES = new NodeFeature("ingest.append.ignore_empty_values", true);
21+
private static final NodeFeature INGEST_APPEND_IGNORE_EMPTY_VALUES_FIX = new NodeFeature("ingest.append.ignore_empty_values_fix", true);
2122

2223
@Override
2324
public Set<NodeFeature> getFeatures() {
@@ -26,6 +27,11 @@ public Set<NodeFeature> getFeatures() {
2627

2728
@Override
2829
public Set<NodeFeature> getTestFeatures() {
29-
return Set.of(SIMULATE_INGEST_400_ON_FAILURE, INGEST_APPEND_COPY_FROM, INGEST_APPEND_IGNORE_EMPTY_VALUES);
30+
return Set.of(
31+
SIMULATE_INGEST_400_ON_FAILURE,
32+
INGEST_APPEND_COPY_FROM,
33+
INGEST_APPEND_IGNORE_EMPTY_VALUES,
34+
INGEST_APPEND_IGNORE_EMPTY_VALUES_FIX
35+
);
3036
}
3137
}

0 commit comments

Comments
 (0)