From fb2fd4e8eeaaf22ae7fd5b17cf73940d1994025b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christoph=20B=C3=BCscher?= <cbuescher@posteo.de>
Date: Tue, 19 Dec 2017 19:56:12 +0100
Subject: [PATCH] Fix preserving FiltersAggregationBuilder#keyed field on
 rewrite (#27900)

Currently FiltersAggregationBuilder#doRewrite creates a new FiltersAggregationBuilder which doesn't correctly copy the original "keyed" field if a non-keyed filter gets rewritten.
This can cause rendering bugs of the output aggregations like the one reported in #27841.

Closes #27841
---
 .../filter/FiltersAggregationBuilder.java     | 23 ++++++++---
 .../aggregations/bucket/FiltersTests.java     | 40 +++++++++++++++++++
 .../search.aggregation/220_filters_bucket.yml | 16 ++++++++
 3 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java
index 7f3f485d270..a2409bb1a70 100644
--- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java
+++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java
@@ -65,15 +65,19 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
      *            the KeyedFilters to use with this aggregation.
      */
     public FiltersAggregationBuilder(String name, KeyedFilter... filters) {
-        this(name, Arrays.asList(filters));
+        this(name, Arrays.asList(filters), true);
     }
 
-    private FiltersAggregationBuilder(String name, List<KeyedFilter> filters) {
+    private FiltersAggregationBuilder(String name, List<KeyedFilter> filters, boolean keyed) {
         super(name);
-        // internally we want to have a fixed order of filters, regardless of the order of the filters in the request
         this.filters = new ArrayList<>(filters);
-        Collections.sort(this.filters, (KeyedFilter kf1, KeyedFilter kf2) -> kf1.key().compareTo(kf2.key()));
-        this.keyed = true;
+        if (keyed) {
+            // internally we want to have a fixed order of filters, regardless of the order of the filters in the request
+            Collections.sort(this.filters, (KeyedFilter kf1, KeyedFilter kf2) -> kf1.key().compareTo(kf2.key()));
+            this.keyed = true;
+        } else {
+            this.keyed = false;
+        }
     }
 
     /**
@@ -152,6 +156,13 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
         return Collections.unmodifiableList(this.filters);
     }
 
+    /**
+     * @return true if this builders filters have a key
+     */
+    public boolean isKeyed() {
+        return this.keyed;
+    }
+
     /**
      * Set the key to use for the bucket for documents not matching any
      * filter.
@@ -184,7 +195,7 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
             }
         }
         if (changed) {
-            return new FiltersAggregationBuilder(getName(), rewrittenFilters);
+            return new FiltersAggregationBuilder(getName(), rewrittenFilters, this.keyed);
         } else {
             return this;
         }
diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java
index 6021d2ff399..7e63bbb6f38 100644
--- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java
+++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java
@@ -23,15 +23,21 @@ import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.index.query.BoolQueryBuilder;
+import org.elasticsearch.index.query.MatchAllQueryBuilder;
 import org.elasticsearch.index.query.MatchNoneQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryBuilders;
+import org.elasticsearch.index.query.QueryRewriteContext;
+import org.elasticsearch.search.aggregations.AggregationBuilder;
 import org.elasticsearch.search.aggregations.BaseAggregationTestCase;
 import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregator.KeyedFilter;
 
 import java.io.IOException;
 
+import static org.hamcrest.Matchers.instanceOf;
+
 public class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuilder> {
 
     @Override
@@ -113,4 +119,38 @@ public class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuil
         // unless the other bucket is explicitly disabled
         assertFalse(filters.otherBucket());
     }
+
+    public void testRewrite() throws IOException {
+        // test non-keyed filter that doesn't rewrite
+        AggregationBuilder original = new FiltersAggregationBuilder("my-agg", new MatchAllQueryBuilder());
+        AggregationBuilder rewritten = original.rewrite(new QueryRewriteContext(xContentRegistry(), null, null, () -> 0L));
+        assertSame(original, rewritten);
+
+        // test non-keyed filter that does rewrite
+        original = new FiltersAggregationBuilder("my-agg", new BoolQueryBuilder());
+        rewritten = original.rewrite(new QueryRewriteContext(xContentRegistry(), null, null, () -> 0L));
+        assertNotSame(original, rewritten);
+        assertThat(rewritten, instanceOf(FiltersAggregationBuilder.class));
+        assertEquals("my-agg", ((FiltersAggregationBuilder) rewritten).getName());
+        assertEquals(1, ((FiltersAggregationBuilder) rewritten).filters().size());
+        assertEquals("0", ((FiltersAggregationBuilder) rewritten).filters().get(0).key());
+        assertThat(((FiltersAggregationBuilder) rewritten).filters().get(0).filter(), instanceOf(MatchAllQueryBuilder.class));
+        assertFalse(((FiltersAggregationBuilder) rewritten).isKeyed());
+
+        // test keyed filter that doesn't rewrite
+        original = new FiltersAggregationBuilder("my-agg", new KeyedFilter("my-filter",  new MatchAllQueryBuilder()));
+        rewritten = original.rewrite(new QueryRewriteContext(xContentRegistry(), null, null, () -> 0L));
+        assertSame(original, rewritten);
+
+        // test non-keyed filter that does rewrite
+        original = new FiltersAggregationBuilder("my-agg", new KeyedFilter("my-filter",  new BoolQueryBuilder()));
+        rewritten = original.rewrite(new QueryRewriteContext(xContentRegistry(), null, null, () -> 0L));
+        assertNotSame(original, rewritten);
+        assertThat(rewritten, instanceOf(FiltersAggregationBuilder.class));
+        assertEquals("my-agg", ((FiltersAggregationBuilder) rewritten).getName());
+        assertEquals(1, ((FiltersAggregationBuilder) rewritten).filters().size());
+        assertEquals("my-filter", ((FiltersAggregationBuilder) rewritten).filters().get(0).key());
+        assertThat(((FiltersAggregationBuilder) rewritten).filters().get(0).filter(), instanceOf(MatchAllQueryBuilder.class));
+        assertTrue(((FiltersAggregationBuilder) rewritten).isKeyed());
+    }
 }
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/220_filters_bucket.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/220_filters_bucket.yml
index 696a420953d..04981164b9f 100644
--- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/220_filters_bucket.yml
+++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/220_filters_bucket.yml
@@ -246,6 +246,22 @@ setup:
   - match: { aggregations.the_filter.buckets.second_filter.doc_count: 1 }
   - match: { aggregations.the_filter.meta.foo: "bar" }
 
+---
+"Single anonymous bool query":
+
+  - do:
+      search:
+        body:
+          aggs:
+            the_filter:
+              filters:
+                filters:
+                  - bool: {}
+
+  - match: { hits.total: 4 }
+  - length: { hits.hits: 4 }
+  - match: { aggregations.the_filter.buckets.0.doc_count: 4 }
+
 ---
 "Bad params":