Fix FiltersAggregation NPE when filters is empty (#41459)

polyfractal · polyfractal · commit 072a9bdf55b1 · 2019-05-20T10:04:21.000-04:00
If `keyedFilters` is null it assumes there are unkeyed filters...which
will NPE if the unkeyed filters was actually empty.

This refactors to simplify the filter assignment a bit, adds an empty
check and tidies up some formatting.
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
@@ -251,8 +251,20 @@ setup:
 
 ---
 "Bad params":
+  - skip:
+      version: " - 7.1.99"
+      reason: "empty bodies throws exception starting in 7.2"
+  - do:
+      catch: /\[filters\] cannot be empty/
+      search:
+        rest_total_hits_as_int: true
+        body:
+          aggs:
+            the_filter:
+              filters: {}
 
   - do:
+      catch: /\[filters\] cannot be empty/
       search:
         rest_total_hits_as_int: true
         body:
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java
@@ -40,14 +40,15 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 
 import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
 
 public class FiltersAggregationBuilder extends AbstractAggregationBuilder<FiltersAggregationBuilder>
-        implements MultiBucketAggregationBuilder {
+    implements MultiBucketAggregationBuilder {
     public static final String NAME = "filters";
 
     private static final ParseField FILTERS_FIELD = new ParseField("filters");
@@ -74,7 +75,7 @@ private FiltersAggregationBuilder(String name, List<KeyedFilter> filters, boolea
         this.filters = new ArrayList<>(filters);
         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.filters.sort(Comparator.comparing(KeyedFilter::key));
             this.keyed = true;
         } else {
             this.keyed = false;
@@ -220,9 +221,9 @@ protected AggregationBuilder doRewrite(QueryRewriteContext queryShardContext) th
 
     @Override
     protected AggregatorFactory<?> doBuild(SearchContext context, AggregatorFactory<?> parent, Builder subFactoriesBuilder)
-            throws IOException {
+        throws IOException {
         return new FiltersAggregatorFactory(name, filters, keyed, otherBucket, otherBucketKey, context, parent,
-                subFactoriesBuilder, metaData);
+            subFactoriesBuilder, metaData);
     }
 
     @Override
@@ -248,15 +249,15 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param
     }
 
     public static FiltersAggregationBuilder parse(String aggregationName, XContentParser parser)
-            throws IOException {
+        throws IOException {
 
-        List<FiltersAggregator.KeyedFilter> keyedFilters = null;
-        List<QueryBuilder> nonKeyedFilters = null;
+        List<FiltersAggregator.KeyedFilter> filters = new ArrayList<>();
 
-        XContentParser.Token token = null;
+        XContentParser.Token token;
         String currentFieldName = null;
         String otherBucketKey = null;
         Boolean otherBucket = null;
+        boolean keyed = false;
         while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
             if (token == XContentParser.Token.FIELD_NAME) {
                 currentFieldName = parser.currentName();
@@ -265,61 +266,61 @@ public static FiltersAggregationBuilder parse(String aggregationName, XContentPa
                     otherBucket = parser.booleanValue();
                 } else {
                     throw new ParsingException(parser.getTokenLocation(),
-                            "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
+                        "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
                 }
             } else if (token == XContentParser.Token.VALUE_STRING) {
                 if (OTHER_BUCKET_KEY_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
                     otherBucketKey = parser.text();
                 } else {
                     throw new ParsingException(parser.getTokenLocation(),
-                            "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
+                        "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
                 }
             } else if (token == XContentParser.Token.START_OBJECT) {
                 if (FILTERS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                    keyedFilters = new ArrayList<>();
                     String key = null;
                     while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
                         if (token == XContentParser.Token.FIELD_NAME) {
                             key = parser.currentName();
                         } else {
                             QueryBuilder filter = parseInnerQueryBuilder(parser);
-                            keyedFilters.add(new FiltersAggregator.KeyedFilter(key, filter));
+                            filters.add(new FiltersAggregator.KeyedFilter(key, filter));
                         }
                     }
+                    keyed = true;
                 } else {
                     throw new ParsingException(parser.getTokenLocation(),
-                            "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
+                        "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
                 }
             } else if (token == XContentParser.Token.START_ARRAY) {
                 if (FILTERS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                    nonKeyedFilters = new ArrayList<>();
-                    while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
+                    List<QueryBuilder> builders = new ArrayList<>();
+                    while (parser.nextToken() != XContentParser.Token.END_ARRAY) {
                         QueryBuilder filter = parseInnerQueryBuilder(parser);
-                        nonKeyedFilters.add(filter);
+                        builders.add(filter);
+                    }
+                    for (int i = 0; i < builders.size(); i++) {
+                        filters.add(new KeyedFilter(String.valueOf(i), builders.get(i)));
                     }
                 } else {
                     throw new ParsingException(parser.getTokenLocation(),
-                            "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
+                        "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
                 }
             } else {
                 throw new ParsingException(parser.getTokenLocation(),
-                        "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
+                    "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
             }
         }
 
+        if (filters.isEmpty()) {
+            throw new IllegalArgumentException("[" + FILTERS_FIELD + "] cannot be empty.");
+        }
+
+        FiltersAggregationBuilder factory = new FiltersAggregationBuilder(aggregationName, filters, keyed);
+
         if (otherBucket == null && otherBucketKey != null) {
             // automatically enable the other bucket if a key is set, as per the doc
             otherBucket = true;
         }
-
-        FiltersAggregationBuilder factory;
-        if (keyedFilters != null) {
-            factory = new FiltersAggregationBuilder(aggregationName,
-                    keyedFilters.toArray(new FiltersAggregator.KeyedFilter[keyedFilters.size()]));
-        } else {
-            factory = new FiltersAggregationBuilder(aggregationName,
-                    nonKeyedFilters.toArray(new QueryBuilder[nonKeyedFilters.size()]));
-        }
         if (otherBucket != null) {
             factory.otherBucket(otherBucket);
         }
@@ -338,9 +339,9 @@ protected int doHashCode() {
     protected boolean doEquals(Object obj) {
         FiltersAggregationBuilder other = (FiltersAggregationBuilder) obj;
         return Objects.equals(filters, other.filters)
-                && Objects.equals(keyed, other.keyed)
-                && Objects.equals(otherBucket, other.otherBucket)
-                && Objects.equals(otherBucketKey, other.otherBucketKey);
+            && Objects.equals(keyed, other.keyed)
+            && Objects.equals(otherBucket, other.otherBucket)
+            && Objects.equals(otherBucketKey, other.otherBucketKey);
     }
 
     @Override
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java
@@ -92,7 +92,9 @@ public void testFiltersSortedByKey() {
     public void testOtherBucket() throws IOException {
         XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
         builder.startObject();
-        builder.startArray("filters").endArray();
+        builder.startArray("filters")
+            .startObject().startObject("term").field("field", "foo").endObject().endObject()
+            .endArray();
         builder.endObject();
         try (XContentParser parser = createParser(shuffleXContent(builder))) {
             parser.nextToken();
@@ -102,7 +104,9 @@ public void testOtherBucket() throws IOException {
 
             builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
             builder.startObject();
-            builder.startArray("filters").endArray();
+            builder.startArray("filters")
+                .startObject().startObject("term").field("field", "foo").endObject().endObject()
+                .endArray();
             builder.field("other_bucket_key", "some_key");
             builder.endObject();
         }
@@ -114,7 +118,9 @@ public void testOtherBucket() throws IOException {
 
             builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
             builder.startObject();
-            builder.startArray("filters").endArray();
+            builder.startArray("filters")
+                .startObject().startObject("term").field("field", "foo").endObject().endObject()
+                .endArray();
             builder.field("other_bucket", false);
             builder.field("other_bucket_key", "some_key");
             builder.endObject();
@@ -192,4 +198,30 @@ public void testRewritePreservesOtherBucket() throws IOException {
         assertEquals(originalFilters.otherBucket(), rewrittenFilters.otherBucket());
         assertEquals(originalFilters.otherBucketKey(), rewrittenFilters.otherBucketKey());
     }
+
+    public void testEmptyFilters() throws IOException {
+        {
+            XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
+            builder.startObject();
+            builder.startArray("filters").endArray();  // unkeyed array
+            builder.endObject();
+            XContentParser parser = createParser(shuffleXContent(builder));
+            parser.nextToken();
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                () -> FiltersAggregationBuilder.parse("agg_name", parser));
+            assertThat(e.getMessage(), equalTo("[filters] cannot be empty."));
+        }
+
+        {
+            XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
+            builder.startObject();
+            builder.startObject("filters").endObject(); // keyed object
+            builder.endObject();
+            XContentParser parser = createParser(shuffleXContent(builder));
+            parser.nextToken();
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                () -> FiltersAggregationBuilder.parse("agg_name", parser));
+            assertThat(e.getMessage(), equalTo("[filters] cannot be empty."));
+        }
+    }
 }
