Aggregations: Fixes Filter and FiltersAggregation to work with empty …

…query This fix ensures the filter and filters aggregation will not throw a NPE when `{}` is passed in as a filter. Instead `{}` is interpreted as a MatchAllDocsQuery. Closes #17518
elastic · Apr 5, 2016 · 65a5366 · 65a5366
1 parent 7037670
commit 65a5366
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 7 deletions.
diff --git a/...ain/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorBuilder.java b/...ain/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorBuilder.java
@@ -23,6 +23,7 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.query.EmptyQueryBuilder;
+import org.elasticsearch.index.query.MatchAllQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.AggregatorBuilder;
 import org.elasticsearch.search.aggregations.AggregatorFactories;
@@ -51,7 +52,11 @@ public FilterAggregatorBuilder(String name, QueryBuilder<?> filter) {
         if (filter == null) {
             throw new IllegalArgumentException("[filter] must not be null: [" + name + "]");
         }
-        this.filter = filter;
+        if (filter instanceof EmptyQueryBuilder) {
+            this.filter = new MatchAllQueryBuilder();
+        } else {
+            this.filter = filter;
+        }
     }
 
     @Override

diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FilterParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FilterParser.java
@@ -20,7 +20,6 @@
 
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.index.query.MatchAllQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryParseContext;
 import org.elasticsearch.search.aggregations.Aggregator;
@@ -45,9 +44,7 @@ public FilterAggregatorBuilder parse(String aggregationName, XContentParser pars
             throw new ParsingException(null, "filter cannot be null in filter aggregation [{}]", aggregationName);
         }
 
-        FilterAggregatorBuilder factory = new FilterAggregatorBuilder(aggregationName,
-                filter == null ? new MatchAllQueryBuilder() : filter);
-        return factory;
+        return new FilterAggregatorBuilder(aggregationName, filter);
     }
 
     @Override

diff --git a/...src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java b/...src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java
@@ -30,6 +30,7 @@
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.query.EmptyQueryBuilder;
+import org.elasticsearch.index.query.MatchAllQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.Aggregator;
 import org.elasticsearch.search.aggregations.AggregatorFactories;
@@ -70,7 +71,11 @@ public KeyedFilter(String key, QueryBuilder<?> filter) {
                 throw new IllegalArgumentException("[filter] must not be null");
             }
             this.key = key;
-            this.filter = filter;
+            if (filter instanceof EmptyQueryBuilder) {
+                this.filter = new MatchAllQueryBuilder();
+            } else {
+                this.filter = filter;
+            }
         }
 
         public String key() {

diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FilterIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FilterIT.java
@@ -23,6 +23,7 @@
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.query.BoolQueryBuilder;
+import org.elasticsearch.index.query.EmptyQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.bucket.filter.Filter;
 import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
@@ -108,7 +109,18 @@ public void testSimple() throws Exception {
     // See NullPointer issue when filters are empty:
     // https://github.com/elastic/elasticsearch/issues/8438
     public void testEmptyFilterDeclarations() throws Exception {
-        QueryBuilder emptyFilter = new BoolQueryBuilder();
+        QueryBuilder<?> emptyFilter = new BoolQueryBuilder();
+        SearchResponse response = client().prepareSearch("idx").addAggregation(filter("tag1", emptyFilter)).execute().actionGet();
+
+        assertSearchResponse(response);
+
+        Filter filter = response.getAggregations().get("tag1");
+        assertThat(filter, notNullValue());
+        assertThat(filter.getDocCount(), equalTo((long) numDocs));
+    }
+
+    public void testEmptyFilter() throws Exception {
+        QueryBuilder<?> emptyFilter = new EmptyQueryBuilder();
         SearchResponse response = client().prepareSearch("idx").addAggregation(filter("tag1", emptyFilter)).execute().actionGet();
 
         assertSearchResponse(response);

diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersIT.java
@@ -24,6 +24,7 @@
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.query.BoolQueryBuilder;
+import org.elasticsearch.index.query.EmptyQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.bucket.filters.Filters;
 import org.elasticsearch.search.aggregations.bucket.filters.FiltersAggregator.KeyedFilter;
@@ -201,6 +202,32 @@ public void testWithSubAggregation() throws Exception {
         assertThat((double) propertiesCounts[1], equalTo((double) sum / numTag2Docs));
     }
 
+    public void testEmptyFilter() throws Exception {
+        QueryBuilder<?> emptyFilter = new EmptyQueryBuilder();
+        SearchResponse response = client().prepareSearch("idx").addAggregation(filters("tag1", emptyFilter)).execute().actionGet();
+
+        assertSearchResponse(response);
+
+        Filters filter = response.getAggregations().get("tag1");
+        assertThat(filter, notNullValue());
+        assertThat(filter.getBuckets().size(), equalTo(1));
+        assertThat(filter.getBuckets().get(0).getDocCount(), equalTo((long) numDocs));
+    }
+
+    public void testEmptyKeyedFilter() throws Exception {
+        QueryBuilder<?> emptyFilter = new EmptyQueryBuilder();
+        SearchResponse response = client().prepareSearch("idx").addAggregation(filters("tag1", new KeyedFilter("foo", emptyFilter)))
+                .execute().actionGet();
+
+        assertSearchResponse(response);
+
+        Filters filter = response.getAggregations().get("tag1");
+        assertThat(filter, notNullValue());
+        assertThat(filter.getBuckets().size(), equalTo(1));
+        assertThat(filter.getBuckets().get(0).getKey(), equalTo("foo"));
+        assertThat(filter.getBuckets().get(0).getDocCount(), equalTo((long) numDocs));
+    }
+
     public void testAsSubAggregation() {
         SearchResponse response = client().prepareSearch("idx")
                 .addAggregation(