From 12ddb3d0ae1f1a21f1f91577178ab7f8f1225c6b Mon Sep 17 00:00:00 2001
From: Nik Everett <nik9000@gmail.com>
Date: Wed, 5 Feb 2020 13:38:47 -0500
Subject: [PATCH] Fix a sneaky bug in rare_terms (#51868)

When the `rare_terms` aggregation contained another aggregation it'd
break them. Most of the time. This happened because the process that it
uses to remove buckets that turn out not to be rare was incorrectly
merging results from multiple leaves. This'd cause array index out of
bounds issues. We didn't catch it in the test because the issue doesn't
happen on the very first bucket. And the tests generated data in such a
way that the first bucket always contained the rare terms. Randomizing
the order of the generated data fixed the test so it caught the issue.

Closes #51020
---
 .../search.aggregation/280_rare_terms.yml     | 46 +++++++++++++++++++
 .../terms/AbstractRareTermsAggregator.java    |  7 ++-
 .../bucket/terms/LongRareTermsAggregator.java |  5 +-
 .../terms/StringRareTermsAggregator.java      |  5 +-
 .../terms/RareTermsAggregatorTests.java       |  4 +-
 5 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
index a82caddd9cfd4..04d76a987809d 100644
--- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
+++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
@@ -313,4 +313,50 @@ setup:
   - match: { hits.total.value: 1 }
   - length: { aggregations.long_terms.buckets: 0 }
 
+---
+"sub aggs":
+  - skip:
+      version: " - 7.99.99"
+      reason: Sub aggs fixed in 8.0 (to be backported to 7.6.1)
+
+  - do:
+      index:
+        refresh: true
+        index: test_1
+        id: 1
+        body: { "str" : "abc", "number": 1 }
 
+  - do:
+      index:
+        refresh: true
+        index: test_1
+        id: 2
+        body: { "str": "abc", "number": 2 }
+
+  - do:
+      index:
+        refresh: true
+        index: test_1
+        id: 3
+        body: { "str": "bcd", "number": 3 }
+
+  - do:
+      search:
+        body:
+          size: 0
+          aggs:
+            str_terms:
+              rare_terms:
+                field: str
+                max_doc_count: 1
+              aggs:
+                max_n:
+                  max:
+                    field: number
+
+  - match: { hits.total.value: 3 }
+  - length: { aggregations.str_terms.buckets: 1 }
+  - match: { aggregations.str_terms.buckets.0.key: "bcd" }
+  - is_false: aggregations.str_terms.buckets.0.key_as_string
+  - match: { aggregations.str_terms.buckets.0.doc_count: 1 }
+  - match: { aggregations.str_terms.buckets.0.max_n.value: 3.0 }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java
index 2bbe3c01988df..bacc2ef217004 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java
@@ -50,7 +50,6 @@ public abstract class AbstractRareTermsAggregator<T extends ValuesSource,
     protected final U includeExclude;
 
     MergingBucketsDeferringCollector deferringCollector;
-    LeafBucketCollector subCollectors;
     final SetBackedScalingCuckooFilter filter;
 
     AbstractRareTermsAggregator(String name, AggregatorFactories factories, SearchContext context,
@@ -115,14 +114,14 @@ private String descendsFromNestedAggregator(Aggregator parent) {
         return null;
     }
 
-    protected void doCollect(V val, int docId) throws IOException {
+    protected void doCollect(LeafBucketCollector subCollector, V val, int docId) throws IOException {
         long bucketOrdinal = addValueToOrds(val);
 
         if (bucketOrdinal < 0) { // already seen
             bucketOrdinal = -1 - bucketOrdinal;
-            collectExistingBucket(subCollectors, docId, bucketOrdinal);
+            collectExistingBucket(subCollector, docId, bucketOrdinal);
         } else {
-            collectBucket(subCollectors, docId, bucketOrdinal);
+            collectBucket(subCollector, docId, bucketOrdinal);
         }
     }
 
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java
index b1d294fefdcf6..7c79e2db6b832 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java
@@ -64,9 +64,6 @@ protected SortedNumericDocValues getValues(ValuesSource.Numeric valuesSource, Le
     public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
                                                 final LeafBucketCollector sub) throws IOException {
         final SortedNumericDocValues values = getValues(valuesSource, ctx);
-        if (subCollectors == null) {
-            subCollectors = sub;
-        }
         return new LeafBucketCollectorBase(sub, values) {
 
             @Override
@@ -78,7 +75,7 @@ public void collect(int docId, long owningBucketOrdinal) throws IOException {
                         final long val = values.nextValue();
                         if (previous != val || i == 0) {
                             if ((includeExclude == null) || (includeExclude.accept(val))) {
-                                doCollect(val, docId);
+                                doCollect(sub, val, docId);
                             }
                             previous = val;
                         }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java
index 0c200e96b242c..29bb46140ec4b 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java
@@ -60,9 +60,6 @@ public class StringRareTermsAggregator extends AbstractRareTermsAggregator<Value
     public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
                                                 final LeafBucketCollector sub) throws IOException {
         final SortedBinaryDocValues values = valuesSource.bytesValues(ctx);
-        if (subCollectors == null) {
-            subCollectors = sub;
-        }
         return new LeafBucketCollectorBase(sub, values) {
             final BytesRefBuilder previous = new BytesRefBuilder();
 
@@ -84,7 +81,7 @@ public void collect(int docId, long bucket) throws IOException {
                             continue;
                         }
 
-                        doCollect(bytes, docId);
+                        doCollect(sub, bytes, docId);
                         previous.copyBytes(bytes);
                     }
                 }
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java
index 441ed43d244d2..768ceee77ef9e 100644
--- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java
+++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java
@@ -562,7 +562,9 @@ private void executeTestCase(boolean reduced, Query query, List<Long> dataset,
         try (Directory directory = newDirectory()) {
             try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
                 Document document = new Document();
-                for (Long value : dataset) {
+                List<Long> shuffledDataset = new ArrayList<>(dataset);
+                Collections.shuffle(shuffledDataset, random());
+                for (Long value : shuffledDataset) {
                     if (frequently()) {
                         indexWriter.commit();
                     }