From c597d8d56b67dc5c898f383b75ea9bc96b944b30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 12 Feb 2015 16:05:46 +0100 Subject: [PATCH] Aggregations: Prevent negative intervals in date_histogram Negative settings for interval in date_histogram could lead to OOM errors in conjunction with min_doc_count=0. This fix raises exceptions in the histogram builder and the TimeZoneRounding classes so that the query fails before this can happen. Closes #9634 Closes #9690 --- .../common/rounding/TimeZoneRounding.java | 9 +++++++++ .../bucket/histogram/HistogramParser.java | 2 +- .../bucket/DateHistogramTests.java | 19 +++++++++++++++++-- .../aggregations/bucket/HistogramTests.java | 16 ++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java b/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java index 6f251f0f73b..38eb6fa1121 100644 --- a/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java +++ b/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.rounding; +import org.elasticsearch.ElasticsearchIllegalArgumentException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.TimeValue; @@ -62,6 +63,8 @@ public abstract class TimeZoneRounding extends Rounding { public Builder(TimeValue interval) { this.unit = null; + if (interval.millis() < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval.millis(); } @@ -303,6 +306,8 @@ public abstract class TimeZoneRounding extends Rounding { } UTCIntervalTimeZoneRounding(long interval) { + if (interval < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval; } @@ -350,6 +355,8 @@ public abstract class TimeZoneRounding extends Rounding { } TimeIntervalTimeZoneRounding(long interval, DateTimeZone preTz, DateTimeZone postTz) { + if (interval < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval; this.preTz = preTz; this.postTz = postTz; @@ -408,6 +415,8 @@ public abstract class TimeZoneRounding extends Rounding { } DayIntervalTimeZoneRounding(long interval, DateTimeZone preTz, DateTimeZone postTz) { + if (interval < 1) + throw new ElasticsearchIllegalArgumentException("Zero or negative time interval not supported"); this.interval = interval; this.preTz = preTz; this.postTz = postTz; diff --git a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java index b336c43a63c..f316237d734 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java +++ b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramParser.java @@ -115,7 +115,7 @@ public class HistogramParser implements Aggregator.Parser { } } - if (interval < 0) { + if (interval < 1) { throw new SearchParseException(context, "Missing required field [interval] for histogram aggregation [" + aggregationName + "]"); } diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java index daa59db67c3..f16ccbce960 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramTests.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.joda.Joda; import org.elasticsearch.common.settings.ImmutableSettings; @@ -43,6 +44,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.TimeUnit; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; @@ -52,8 +54,7 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.max; import static org.elasticsearch.search.aggregations.AggregationBuilders.stats; import static org.elasticsearch.search.aggregations.AggregationBuilders.sum; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; import static org.hamcrest.core.IsNull.notNullValue; /** @@ -1348,4 +1349,18 @@ public class DateHistogramTests extends ElasticsearchIntegrationTest { assertThat(((DateTime) bucket.getKey()), equalTo(key)); assertThat(bucket.getDocCount(), equalTo(3l)); } + + /** + * see issue #9634, negative interval in date_histogram should raise exception + */ + public void testExeptionOnNegativerInterval() { + try { + client().prepareSearch("idx") + .addAggregation(dateHistogram("histo").field("date").interval(-TimeUnit.DAYS.toMillis(1)).minDocCount(0)).execute() + .actionGet(); + fail(); + } catch (SearchPhaseExecutionException e) { + assertThat(e.getMessage(), containsString("IllegalArgumentException")); + } + } } diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java index 00a19e828d6..964bc6c9cd7 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java @@ -20,7 +20,9 @@ package org.elasticsearch.search.aggregations.bucket; import com.carrotsearch.hppc.LongOpenHashSet; +import org.apache.tools.ant.filters.TokenFilter.ContainsString; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.search.aggregations.Aggregator.SubAggCollectionMode; import org.elasticsearch.search.aggregations.bucket.filter.Filter; @@ -49,6 +51,7 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.sum; 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.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; @@ -992,4 +995,17 @@ public class HistogramTests extends ElasticsearchIntegrationTest { } } + /** + * see issue #9634, negative interval in histogram should raise exception + */ + public void testExeptionOnNegativerInterval() { + try { + client().prepareSearch("empty_bucket_idx") + .addAggregation(histogram("histo").field(SINGLE_VALUED_FIELD_NAME).interval(-1).minDocCount(0)).execute().actionGet(); + fail(); + } catch (SearchPhaseExecutionException e) { + assertThat(e.getMessage(), containsString("Missing required field [interval]")); + } + } + }