Skip to content

Commit

Permalink
Parser throws NullPointerException when Filter aggregation clause is …
Browse files Browse the repository at this point in the history
…empty.

Added Junit test that recreates the error and fixed FilterParser to default to using a MatchAllDocsFilter if the requested filter clause is left empty.
Also added fix and test for the Filters (with an "s") aggregation.

Closes #8438
  • Loading branch information
markharwood committed Nov 20, 2014
1 parent 6c4bb90 commit f2e7364
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 3 deletions.
Expand Up @@ -18,6 +18,7 @@
*/
package org.elasticsearch.search.aggregations.bucket.filter;

import org.elasticsearch.common.lucene.search.MatchAllDocsFilter;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.ParsedFilter;
import org.elasticsearch.search.aggregations.Aggregator;
Expand All @@ -39,7 +40,8 @@ public String type() {
@Override
public AggregatorFactory parse(String aggregationName, XContentParser parser, SearchContext context) throws IOException {
ParsedFilter filter = context.queryParserService().parseInnerFilter(parser);
return new FilterAggregator.Factory(aggregationName, filter.filter());

return new FilterAggregator.Factory(aggregationName, filter == null ? new MatchAllDocsFilter() : filter.filter());
}

}
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.search.aggregations.bucket.filters;

import org.elasticsearch.common.lucene.search.MatchAllDocsFilter;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.ParsedFilter;
import org.elasticsearch.search.SearchParseException;
Expand Down Expand Up @@ -60,7 +61,7 @@ public AggregatorFactory parse(String aggregationName, XContentParser parser, Se
key = parser.currentName();
} else {
ParsedFilter filter = context.queryParserService().parseInnerFilter(parser);
filters.add(new FiltersAggregator.KeyedFilter(key, filter.filter()));
filters.add(new FiltersAggregator.KeyedFilter(key, filter == null ? new MatchAllDocsFilter() : filter.filter()));
}
}
} else {
Expand All @@ -72,7 +73,8 @@ public AggregatorFactory parse(String aggregationName, XContentParser parser, Se
int idx = 0;
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
ParsedFilter filter = context.queryParserService().parseInnerFilter(parser);
filters.add(new FiltersAggregator.KeyedFilter(String.valueOf(idx), filter.filter()));
filters.add(new FiltersAggregator.KeyedFilter(String.valueOf(idx), filter == null ? new MatchAllDocsFilter()
: filter.filter()));
idx++;
}
} else {
Expand Down
Expand Up @@ -21,6 +21,8 @@
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.index.query.AndFilterBuilder;
import org.elasticsearch.index.query.FilterBuilder;
import org.elasticsearch.search.aggregations.bucket.filter.Filter;
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
import org.elasticsearch.search.aggregations.metrics.avg.Avg;
Expand Down Expand Up @@ -97,6 +99,20 @@ public void simple() throws Exception {
assertThat(filter.getDocCount(), equalTo((long) numTag1Docs));
}

// See NullPointer issue when filters are empty:
// https://github.com/elasticsearch/elasticsearch/issues/8438
@Test
public void emptyFilterDeclarations() throws Exception {
FilterBuilder emptyFilter = new AndFilterBuilder();
SearchResponse response = client().prepareSearch("idx").addAggregation(filter("tag1").filter(emptyFilter)).execute().actionGet();

assertSearchResponse(response);

Filter filter = response.getAggregations().get("tag1");
assertThat(filter, notNullValue());
assertThat(filter.getDocCount(), equalTo((long) numDocs));
}

@Test
public void withSubAggregation() throws Exception {
SearchResponse response = client().prepareSearch("idx")
Expand Down
Expand Up @@ -22,6 +22,8 @@
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.index.query.AndFilterBuilder;
import org.elasticsearch.index.query.FilterBuilder;
import org.elasticsearch.search.aggregations.bucket.filters.Filters;
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
import org.elasticsearch.search.aggregations.metrics.avg.Avg;
Expand Down Expand Up @@ -112,6 +114,27 @@ public void simple() throws Exception {
assertThat(bucket.getDocCount(), equalTo((long) numTag2Docs));
}

// See NullPointer issue when filters are empty:
// https://github.com/elasticsearch/elasticsearch/issues/8438
@Test
public void emptyFilterDeclarations() throws Exception {
FilterBuilder emptyFilter = new AndFilterBuilder();
SearchResponse response = client().prepareSearch("idx")
.addAggregation(filters("tags").filter("all", emptyFilter).filter("tag1", termFilter("tag", "tag1"))).execute()
.actionGet();

assertSearchResponse(response);

Filters filters = response.getAggregations().get("tags");
assertThat(filters, notNullValue());
Filters.Bucket allBucket = filters.getBucketByKey("all");
assertThat(allBucket.getDocCount(), equalTo((long) numDocs));

Filters.Bucket bucket = filters.getBucketByKey("tag1");
assertThat(bucket, Matchers.notNullValue());
assertThat(bucket.getDocCount(), equalTo((long) numTag1Docs));
}

@Test
public void withSubAggregation() throws Exception {
SearchResponse response = client().prepareSearch("idx")
Expand Down

0 comments on commit f2e7364

Please sign in to comment.