Skip to content

Commit cd1ed90

Browse files
authored
Fix bug in BucketMetrics path traversal (#30632)
When processing a top-level sibling pipeline, we destructively sublist the path by assigning back onto the same variable. But if aggs are specified such: A. Multi-bucket agg in the first entry of our internal list B. Regular agg as the immediate child of the multi-bucket in A C. Regular agg with the same name as B at the top level, listed as the second entry in our internal list D. Finally, a pipeline agg with the path down to B We'll get class cast exception. The first agg will sublist the path from [A,B] to [B], and then when we loop around to check agg C, the sublisted path [B] matches the name of C and it fails. The fix is simple: we just need to store the sublist in a new object so that the old path remains valid for the rest of the aggs in the loop Closes #30608
1 parent 95ad9ab commit cd1ed90

File tree

2 files changed

+146
-2
lines changed

2 files changed

+146
-2
lines changed

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregator.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,11 @@ public final InternalAggregation doReduce(Aggregations aggregations, ReduceConte
7979
List<String> bucketsPath = AggregationPath.parse(bucketsPaths()[0]).getPathElementsAsStringList();
8080
for (Aggregation aggregation : aggregations) {
8181
if (aggregation.getName().equals(bucketsPath.get(0))) {
82-
bucketsPath = bucketsPath.subList(1, bucketsPath.size());
82+
List<String> sublistedPath = bucketsPath.subList(1, bucketsPath.size());
8383
InternalMultiBucketAggregation<?, ?> multiBucketsAgg = (InternalMultiBucketAggregation<?, ?>) aggregation;
8484
List<? extends InternalMultiBucketAggregation.InternalBucket> buckets = multiBucketsAgg.getBuckets();
8585
for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) {
86-
Double bucketValue = BucketHelpers.resolveBucketValue(multiBucketsAgg, bucket, bucketsPath, gapPolicy);
86+
Double bucketValue = BucketHelpers.resolveBucketValue(multiBucketsAgg, bucket, sublistedPath, gapPolicy);
8787
if (bucketValue != null && !Double.isNaN(bucketValue)) {
8888
collectBucketValue(bucket.getKeyAsString(), bucketValue);
8989
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.search.aggregations.pipeline.bucketmetrics.avg;
21+
22+
import org.apache.lucene.document.Document;
23+
import org.apache.lucene.document.SortedNumericDocValuesField;
24+
import org.apache.lucene.index.DirectoryReader;
25+
import org.apache.lucene.index.IndexReader;
26+
import org.apache.lucene.index.RandomIndexWriter;
27+
import org.apache.lucene.search.IndexSearcher;
28+
import org.apache.lucene.search.MatchAllDocsQuery;
29+
import org.apache.lucene.search.Query;
30+
import org.apache.lucene.store.Directory;
31+
import org.elasticsearch.index.mapper.DateFieldMapper;
32+
import org.elasticsearch.index.mapper.MappedFieldType;
33+
import org.elasticsearch.index.mapper.NumberFieldMapper;
34+
import org.elasticsearch.search.aggregations.Aggregation;
35+
import org.elasticsearch.search.aggregations.Aggregations;
36+
import org.elasticsearch.search.aggregations.AggregatorTestCase;
37+
import org.elasticsearch.search.aggregations.InternalAggregation;
38+
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
39+
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
40+
import org.elasticsearch.search.aggregations.bucket.histogram.InternalDateHistogram;
41+
import org.elasticsearch.search.aggregations.metrics.avg.AvgAggregationBuilder;
42+
import org.elasticsearch.search.aggregations.metrics.avg.InternalAvg;
43+
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
44+
45+
import java.io.IOException;
46+
import java.util.ArrayList;
47+
import java.util.Arrays;
48+
import java.util.Collections;
49+
import java.util.List;
50+
51+
52+
public class AvgBucketAggregatorTests extends AggregatorTestCase {
53+
private static final String DATE_FIELD = "date";
54+
private static final String VALUE_FIELD = "value";
55+
56+
private static final List<String> dataset = Arrays.asList(
57+
"2010-03-12T01:07:45",
58+
"2010-04-27T03:43:34",
59+
"2012-05-18T04:11:00",
60+
"2013-05-29T05:11:31",
61+
"2013-10-31T08:24:05",
62+
"2015-02-13T13:09:32",
63+
"2015-06-24T13:47:43",
64+
"2015-11-13T16:14:34",
65+
"2016-03-04T17:09:50",
66+
"2017-12-12T22:55:46");
67+
68+
/**
69+
* Test for issue #30608. Under the following circumstances:
70+
*
71+
* A. Multi-bucket agg in the first entry of our internal list
72+
* B. Regular agg as the immediate child of the multi-bucket in A
73+
* C. Regular agg with the same name as B at the top level, listed as the second entry in our internal list
74+
* D. Finally, a pipeline agg with the path down to B
75+
*
76+
* BucketMetrics reduction would throw a class cast exception due to bad subpathing. This test ensures
77+
* it is fixed.
78+
*
79+
* Note: we have this test inside of the `avg_bucket` package so that we can get access to the package-private
80+
* `doReduce()` needed for testing this
81+
*/
82+
public void testSameAggNames() throws IOException {
83+
Query query = new MatchAllDocsQuery();
84+
85+
AvgAggregationBuilder avgBuilder = new AvgAggregationBuilder("foo").field(VALUE_FIELD);
86+
DateHistogramAggregationBuilder histo = new DateHistogramAggregationBuilder("histo")
87+
.dateHistogramInterval(DateHistogramInterval.YEAR)
88+
.field(DATE_FIELD)
89+
.subAggregation(new AvgAggregationBuilder("foo").field(VALUE_FIELD));
90+
91+
AvgBucketPipelineAggregationBuilder avgBucketBuilder
92+
= new AvgBucketPipelineAggregationBuilder("the_avg_bucket", "histo>foo");
93+
94+
try (Directory directory = newDirectory()) {
95+
try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
96+
Document document = new Document();
97+
for (String date : dataset) {
98+
if (frequently()) {
99+
indexWriter.commit();
100+
}
101+
102+
document.add(new SortedNumericDocValuesField(DATE_FIELD, asLong(date)));
103+
document.add(new SortedNumericDocValuesField(VALUE_FIELD, randomInt()));
104+
indexWriter.addDocument(document);
105+
document.clear();
106+
}
107+
}
108+
109+
InternalAvg avgResult;
110+
InternalDateHistogram histogramResult;
111+
try (IndexReader indexReader = DirectoryReader.open(directory)) {
112+
IndexSearcher indexSearcher = newSearcher(indexReader, true, true);
113+
114+
DateFieldMapper.Builder builder = new DateFieldMapper.Builder("histo");
115+
DateFieldMapper.DateFieldType fieldType = builder.fieldType();
116+
fieldType.setHasDocValues(true);
117+
fieldType.setName(DATE_FIELD);
118+
119+
MappedFieldType valueFieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
120+
valueFieldType.setName(VALUE_FIELD);
121+
valueFieldType.setHasDocValues(true);
122+
123+
avgResult = searchAndReduce(indexSearcher, query, avgBuilder, 10000, new MappedFieldType[]{fieldType, valueFieldType});
124+
histogramResult = searchAndReduce(indexSearcher, query, histo, 10000, new MappedFieldType[]{fieldType, valueFieldType});
125+
}
126+
127+
// Finally, reduce the pipeline agg
128+
PipelineAggregator avgBucketAgg = avgBucketBuilder.createInternal(Collections.emptyMap());
129+
List<Aggregation> reducedAggs = new ArrayList<>(2);
130+
131+
// Histo has to go first to exercise the bug
132+
reducedAggs.add(histogramResult);
133+
reducedAggs.add(avgResult);
134+
Aggregations aggregations = new Aggregations(reducedAggs);
135+
InternalAggregation pipelineResult = ((AvgBucketPipelineAggregator)avgBucketAgg).doReduce(aggregations, null);
136+
assertNotNull(pipelineResult);
137+
}
138+
}
139+
140+
141+
private static long asLong(String dateTime) {
142+
return DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parser().parseDateTime(dateTime).getMillis();
143+
}
144+
}

0 commit comments

Comments
 (0)