Skip to content

Commit a24f2ad

Browse files
joegallojoegallo
authored
Fix allow_duplicates edge case bug in append processor (#134319)
1 parent 7c957c3 commit a24f2ad

File tree

3 files changed

+190
-6
lines changed

3 files changed

+190
-6
lines changed

‎docs/changelog/134319.yaml‎

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

‎modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/360_append_allow_duplicates.yml‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
---
2+
setup:
3+
- requires:
4+
cluster_features: [ "ingest.append.copy_from" ]
5+
reason: "The copy_from option of the append processor is new"
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": "dest",
16+
"copy_from": "src",
17+
"allow_duplicates": false
18+
}
19+
},
20+
{
21+
"append": {
22+
"if": "ctx?.templating == true",
23+
"field": "dest",
24+
"value": ["{{three}}", "{{three}}", "{{three}}"],
25+
"allow_duplicates": false
26+
}
27+
},
28+
{
29+
"foreach": {
30+
"description": "templating results in strings, so convert them if necessary",
31+
"field": "dest",
32+
"processor": {
33+
"convert": {
34+
"field": "_ingest._value",
35+
"type": "long"
36+
}
37+
}
38+
}
39+
},
40+
{
41+
"remove": {
42+
"description": "we only care about the dest, so remove everything else",
43+
"keep": ["dest"]
44+
}
45+
}
46+
]
47+
}
48+
- do:
49+
indices.create:
50+
index: "test-some-index"
51+
52+
---
53+
teardown:
54+
- do:
55+
indices.delete:
56+
index: "test-some-index"
57+
ignore_unavailable: true
58+
- do:
59+
ingest.delete_pipeline:
60+
id: "test-pipeline-1"
61+
ignore: 404
62+
63+
---
64+
"allow_duplicates (false) removes duplicates with a present array and copy_from":
65+
- do:
66+
index:
67+
index: test-some-index
68+
id: 1
69+
pipeline: test-pipeline-1
70+
body: >
71+
{
72+
"src": [3, 3, 3],
73+
"dest": [3]
74+
}
75+
76+
- do:
77+
get:
78+
index: test-some-index
79+
id: "1"
80+
- match: { _source.dest: [3] }
81+
82+
---
83+
"allow_duplicates (false) removes duplicates with an empty array and copy_from":
84+
- do:
85+
index:
86+
index: test-some-index
87+
id: 1
88+
pipeline: test-pipeline-1
89+
body: >
90+
{
91+
"src": [3, 3, 3],
92+
"dest": []
93+
}
94+
95+
- do:
96+
get:
97+
index: test-some-index
98+
id: "1"
99+
- match: { _source.dest: [3] }
100+
101+
---
102+
"allow_duplicates (false) removes duplicates with a missing array and copy_from":
103+
- do:
104+
index:
105+
index: test-some-index
106+
id: 1
107+
pipeline: test-pipeline-1
108+
body: >
109+
{
110+
"src": [3, 3, 3]
111+
}
112+
113+
- do:
114+
get:
115+
index: test-some-index
116+
id: "1"
117+
- match: { _source.dest: [3] }
118+
119+
---
120+
"allow_duplicates (false) removes duplicates with a present array and templating":
121+
- do:
122+
index:
123+
index: test-some-index
124+
id: 1
125+
pipeline: test-pipeline-1
126+
body: >
127+
{
128+
"templating": true,
129+
"three": 3,
130+
"dest": ["3"],
131+
"note": "blargh, duplicate removal is based on strings, because of templating"
132+
}
133+
134+
- do:
135+
get:
136+
index: test-some-index
137+
id: "1"
138+
- match: { _source.dest: [3] }
139+
140+
---
141+
"allow_duplicates (false) removes duplicates with an empty array and templating":
142+
- do:
143+
index:
144+
index: test-some-index
145+
id: 1
146+
pipeline: test-pipeline-1
147+
body: >
148+
{
149+
"templating": true,
150+
"three": 3,
151+
"dest": []
152+
}
153+
154+
- do:
155+
get:
156+
index: test-some-index
157+
id: "1"
158+
- match: { _source.dest: [3] }
159+
160+
---
161+
"allow_duplicates (false) removes duplicates with a missing array and templating":
162+
- do:
163+
index:
164+
index: test-some-index
165+
id: 1
166+
pipeline: test-pipeline-1
167+
body: >
168+
{
169+
"templating": true,
170+
"three": 3
171+
}
172+
173+
- do:
174+
get:
175+
index: test-some-index
176+
id: "1"
177+
- match: { _source.dest: [3] }

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,7 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
861861
Object object = map.getOrDefault(leafKey, NOT_FOUND); // getOrDefault is faster than containsKey + get
862862
if (object == NOT_FOUND) {
863863
List<Object> list = new ArrayList<>();
864-
appendValues(list, value);
864+
appendValues(list, value, allowDuplicates);
865865
map.put(leafKey, list);
866866
} else {
867867
Object list = appendValues(object, value, allowDuplicates);
@@ -879,7 +879,7 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
879879
Object object = map.getOrDefault(leafKey, NOT_FOUND); // getOrDefault is faster than containsKey + get
880880
if (object == NOT_FOUND) {
881881
List<Object> list = new ArrayList<>();
882-
appendValues(list, value);
882+
appendValues(list, value, allowDuplicates);
883883
map.put(leafKey, list);
884884
} else {
885885
Object list = appendValues(object, value, allowDuplicates);
@@ -933,23 +933,25 @@ private static Object appendValues(Object maybeList, Object value, boolean allow
933933
list.add(maybeList);
934934
}
935935
if (allowDuplicates) {
936-
appendValues(list, value);
936+
innerAppendValues(list, value);
937937
return list;
938938
} else {
939939
// if no values were appended due to duplication, return the original object so the ingest document remains unmodified
940-
return appendValuesWithoutDuplicates(list, value) ? list : maybeList;
940+
return innerAppendValuesWithoutDuplicates(list, value) ? list : maybeList;
941941
}
942942
}
943943

944-
private static void appendValues(List<Object> list, Object value) {
944+
// helper method for use in appendValues above, please do not call this directly except from that method
945+
private static void innerAppendValues(List<Object> list, Object value) {
945946
if (value instanceof List<?> l) {
946947
list.addAll(l);
947948
} else {
948949
list.add(value);
949950
}
950951
}
951952

952-
private static boolean appendValuesWithoutDuplicates(List<Object> list, Object value) {
953+
// helper method for use in appendValues above, please do not call this directly except from that method
954+
private static boolean innerAppendValuesWithoutDuplicates(List<Object> list, Object value) {
953955
boolean valuesWereAppended = false;
954956
if (value instanceof List<?> valueList) {
955957
for (Object val : valueList) {

0 commit comments

Comments
 (0)