honeymoose/OpenSearch
1
0
Fork 0
mirror of https://github.com/honeymoose/OpenSearch.git synced 2025-02-17 02:14:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix FiltersAggregation NPE when filters is empty (#41459)

Browse Source 
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.
This commit is contained in:
Zachary Tong 2019-05-20 09:46:37 -04:00
parent dbbdcea128
commit 072a9bdf55
3 changed files with 78 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/220_filters_bucket.yml
View File

@ -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:

61
server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregationBuilder.java
View File

@ -40,6 +40,7 @@ import java.io.IOException;
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;
@ -47,7 +48,7 @@ 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 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
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 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
@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 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
}
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 class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
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 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
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

38
server/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java
View File

@ -92,7 +92,9 @@ public class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuil
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 class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuil
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 class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuil
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 class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuil
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."));
}
}
}