From 8a4d8909a1e6020a8007097eb3d38f27067201f5 Mon Sep 17 00:00:00 2001 From: George Papadrosou Date: Wed, 17 May 2017 12:34:01 +0300 Subject: [PATCH] Fix ArrayIndexOutOfBoundsException when no ranges are specified in the query (#23241) * Fix ArrayIndexOutOfBoundsException in Range Aggregation when no ranges are specified in the query * Revert "Fix ArrayIndexOutOfBoundsException in Range Aggregation when no ranges are specified in the query" This reverts commit ad57d8feb3577a64b37de28c6f3df96a3a49fe93. * Fix range aggregation out of bounds exception when there are no ranges in a range or date_range query * Fix range aggregation out of bounds exception when there are no ranges in the query This fix is applied to range queries, date range queries, ip range queries and geo distance aggregation queries --- .../bucket/range/RangeAggregationBuilder.java | 3 ++ .../date/DateRangeAggregationBuilder.java | 5 ++- .../GeoDistanceAggregationBuilder.java | 3 ++ .../range/ip/IpRangeAggregationBuilder.java | 3 ++ .../aggregations/bucket/DateRangeIT.java | 15 ++++++++ .../aggregations/bucket/GeoDistanceIT.java | 15 ++++++++ .../search/aggregations/bucket/IpRangeIT.java | 38 +++++++++++++------ .../search/aggregations/bucket/RangeIT.java | 16 ++++++++ 8 files changed, 86 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java index 5692b34c57f..105dbbc5458 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java @@ -140,6 +140,9 @@ public class RangeAggregationBuilder extends AbstractRangeBuilder parent, Builder subFactoriesBuilder) throws IOException { // We need to call processRanges here so they are parsed before we make the decision of whether to cache the request Range[] ranges = processRanges(context, config); + if (ranges.length == 0) { + throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation"); + } return new RangeAggregatorFactory(name, config, ranges, keyed, rangeFactory, context, parent, subFactoriesBuilder, metaData); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java index de5622299cf..2c686fbb977 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java @@ -283,9 +283,12 @@ public class DateRangeAggregationBuilder extends AbstractRangeBuilder config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - // We need to call processRanges here so they are parsed and we know whether `now` has been used before we make + // We need to call processRanges here so they are parsed and we know whether `now` has been used before we make // the decision of whether to cache the request Range[] ranges = processRanges(context, config); + if (ranges.length == 0) { + throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation"); + } return new DateRangeAggregatorFactory(name, config, ranges, keyed, rangeFactory, context, parent, subFactoriesBuilder, metaData); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java index 9278a0b73b2..1484fae8d44 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java @@ -384,6 +384,9 @@ public class GeoDistanceAggregationBuilder extends ValuesSourceAggregationBuilde ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { Range[] ranges = this.ranges.toArray(new Range[this.range().size()]); + if (ranges.length == 0) { + throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation"); + } return new GeoDistanceRangeAggregatorFactory(name, config, origin, ranges, unit, distanceType, keyed, context, parent, subFactoriesBuilder, metaData); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ip/IpRangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ip/IpRangeAggregationBuilder.java index cb03ef7251a..c530ecbfa99 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ip/IpRangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ip/IpRangeAggregationBuilder.java @@ -369,6 +369,9 @@ public final class IpRangeAggregationBuilder AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { List ranges = new ArrayList<>(); + if(this.ranges.size() == 0){ + throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation"); + } for (Range range : this.ranges) { ranges.add(new BinaryRangeAggregator.Range(range.key, toBytesRef(range.from), toBytesRef(range.to))); } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java index 8a167c0daf0..ce575964977 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.plugins.Plugin; @@ -51,6 +52,7 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.sum; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.core.IsNull.notNullValue; import static org.hamcrest.core.IsNull.nullValue; @@ -866,6 +868,19 @@ public class DateRangeIT extends ESIntegTestCase { assertThat(buckets.get(0).getAggregations().asList().isEmpty(), is(true)); } + public void testNoRangesInQuery() { + try { + client().prepareSearch("idx") + .addAggregation(dateRange("my_date_range_agg").field("value")) + .execute().actionGet(); + fail(); + } catch (SearchPhaseExecutionException spee){ + Throwable rootCause = spee.getCause().getCause(); + assertThat(rootCause, instanceOf(IllegalArgumentException.class)); + assertEquals(rootCause.getMessage(), "No [ranges] specified for the [my_date_range_agg] aggregation"); + } + } + /** * Make sure that a request using a script does not get cached and a request * not using a script does get cached. diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java index d8aab691d2a..032bb8d5914 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.bucket; import org.elasticsearch.Version; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.geo.GeoPoint; @@ -52,6 +53,7 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.histogra import static org.elasticsearch.search.aggregations.AggregationBuilders.terms; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.sameInstance; import static org.hamcrest.core.IsNull.notNullValue; @@ -441,6 +443,19 @@ public class GeoDistanceIT extends ESIntegTestCase { assertThat(buckets.get(0).getDocCount(), equalTo(0L)); } + public void testNoRangesInQuery() { + try { + client().prepareSearch("idx") + .addAggregation(geoDistance("geo_dist", new GeoPoint(52.3760, 4.894))) + .execute().actionGet(); + fail(); + } catch (SearchPhaseExecutionException spee){ + Throwable rootCause = spee.getCause().getCause(); + assertThat(rootCause, instanceOf(IllegalArgumentException.class)); + assertEquals(rootCause.getMessage(), "No [ranges] specified for the [geo_dist] aggregation"); + } + } + public void testMultiValues() throws Exception { SearchResponse response = client().prepareSearch("idx-multi") .addAggregation(geoDistance("amsterdam_rings", new GeoPoint(52.3760, 4.894)) diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java index cc4818963ad..b9bb46501db 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java @@ -18,18 +18,9 @@ */ package org.elasticsearch.search.aggregations.bucket; -import org.elasticsearch.cluster.health.ClusterHealthStatus; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; -import static org.hamcrest.Matchers.containsString; - -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.List; -import java.util.Map; - +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.cluster.health.ClusterHealthStatus; import org.elasticsearch.common.inject.internal.Nullable; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.ScriptPlugin; @@ -42,6 +33,17 @@ import org.elasticsearch.search.aggregations.AggregationBuilders; import org.elasticsearch.search.aggregations.bucket.range.Range; import org.elasticsearch.test.ESIntegTestCase; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; + @ESIntegTestCase.SuiteScopeTestCase public class IpRangeIT extends ESIntegTestCase { @@ -221,6 +223,20 @@ public class IpRangeIT extends ESIntegTestCase { assertThat(e.getMessage(), containsString("[ip_range] does not support scripts")); } + public void testNoRangesInQuery() { + try { + client().prepareSearch("idx").addAggregation( + AggregationBuilders.ipRange("my_range") + .field("ip")) + .execute().actionGet(); + fail(); + } catch (SearchPhaseExecutionException spee){ + Throwable rootCause = spee.getCause().getCause(); + assertThat(rootCause, instanceOf(IllegalArgumentException.class)); + assertEquals(rootCause.getMessage(), "No [ranges] specified for the [my_range] aggregation"); + } + } + public static class DummyScriptPlugin extends Plugin implements ScriptPlugin { @Override public List getNativeScripts() { diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java index b4bb3c819d3..c2a2405098d 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/RangeIT.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.fielddata.ScriptDocValues; @@ -53,6 +54,7 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.terms; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.core.IsNull.notNullValue; import static org.hamcrest.core.IsNull.nullValue; @@ -661,6 +663,20 @@ public class RangeIT extends ESIntegTestCase { assertThat(bucket.getDocCount(), equalTo(0L)); } + public void testNoRangesInQuery() { + try { + client().prepareSearch("idx") + .addAggregation(range("foobar") + .field(SINGLE_VALUED_FIELD_NAME)) + .execute().actionGet(); + fail(); + } catch (SearchPhaseExecutionException spee){ + Throwable rootCause = spee.getCause().getCause(); + assertThat(rootCause, instanceOf(IllegalArgumentException.class)); + assertEquals(rootCause.getMessage(), "No [ranges] specified for the [foobar] aggregation"); + } + } + public void testScriptMultiValued() throws Exception { Script script = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['" + MULTI_VALUED_FIELD_NAME + "'].values", Collections.emptyMap());