elastic#19855 Throw exception when maxBounds greater than minBounds

Throw exception when maxBounds greater than minBounds
areek · Aug 8, 2016 · bf0e42a · bf0e42a
2 parents 180eff1 + 4735e0a
commit bf0e42a
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 15 deletions.
diff --git a/...a/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/...a/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java
@@ -47,8 +47,8 @@ public class HistogramAggregationBuilder
 
     private double interval;
     private double offset = 0;
-    private double minBound = Double.MAX_VALUE;
-    private double maxBound = Double.MIN_VALUE;
+    private double minBound = Double.POSITIVE_INFINITY;
+    private double maxBound = Double.NEGATIVE_INFINITY;
     private InternalOrder order = (InternalOrder) Histogram.Order.KEY_ASC;
     private boolean keyed = false;
     private long minDocCount = 0;
@@ -122,17 +122,24 @@ public double maxBound() {
         return maxBound;
     }
 
-    /** Set extended bounds on this builder: buckets between {@code minBound}
-     *  and {@code maxBound} will be created even if no documents fell into
-     *  these buckets. It is possible to create half-open bounds by providing
-     *  {@link Double#POSITIVE_INFINITY} as a {@code minBound} or 
-     *  {@link Double#NEGATIVE_INFINITY} as a {@code maxBound}. */
+    /**
+     * Set extended bounds on this builder: buckets between {@code minBound} and
+     * {@code maxBound} will be created even if no documents fell into these
+     * buckets.
+     *
+     * @throws IllegalArgumentException
+     *             if maxBound is less that minBound, or if either of the bounds
+     *             are not finite.
+     */
     public HistogramAggregationBuilder extendedBounds(double minBound, double maxBound) {
-        if (minBound == Double.NEGATIVE_INFINITY) {
-            throw new IllegalArgumentException("minBound must not be -Infinity, got: " + minBound);
+        if (Double.isFinite(minBound) == false) {
+            throw new IllegalArgumentException("minBound must be finite, got: " + minBound);
         }
-        if (maxBound == Double.POSITIVE_INFINITY) {
-            throw new IllegalArgumentException("maxBound must not be +Infinity, got: " + maxBound);
+        if (Double.isFinite(maxBound) == false) {
+            throw new IllegalArgumentException("maxBound must be finite, got: " + maxBound);
+        }
+        if (maxBound < minBound) {
+            throw new IllegalArgumentException("maxBound [" + maxBound + "] must be greater than minBound [" + minBound + "]");
         }
         this.minBound = minBound;
         this.maxBound = maxBound;

diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramIT.java
@@ -860,7 +860,7 @@ public void testSingleValuedFieldWithExtendedBounds() throws Exception {
                 return;
             }
 
-        } catch (Exception e) {
+        } catch (IllegalArgumentException e) {
             if (invalidBoundsError) {
                 // expected
                 return;
@@ -886,7 +886,6 @@ public void testSingleValuedFieldWithExtendedBounds() throws Exception {
         }
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/19833")
     public void testEmptyWithExtendedBounds() throws Exception {
         int lastDataBucketKey = (numValueBuckets - 1) * interval;
 
@@ -938,7 +937,7 @@ public void testEmptyWithExtendedBounds() throws Exception {
                 return;
             }
 
-        } catch (Exception e) {
+        } catch (IllegalArgumentException e) {
             if (invalidBoundsError) {
                 // expected
                 return;

diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java
@@ -23,6 +23,9 @@
 import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Order;
 
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.startsWith;
+
 public class HistogramTests extends BaseAggregationTestCase<HistogramAggregationBuilder> {
 
     @Override
@@ -31,7 +34,9 @@ protected HistogramAggregationBuilder createTestAggregatorBuilder() {
         factory.field(INT_FIELD_NAME);
         factory.interval(randomDouble() * 1000);
         if (randomBoolean()) {
-            factory.extendedBounds(randomDouble(), randomDouble());
+            double minBound = randomDouble();
+            double maxBound = randomDoubleBetween(minBound, 1, true);
+            factory.extendedBounds(minBound, maxBound);
         }
         if (randomBoolean()) {
             factory.format("###.##");
@@ -74,4 +79,27 @@ protected HistogramAggregationBuilder createTestAggregatorBuilder() {
         return factory;
     }
 
+    public void testInvalidBounds() {
+        HistogramAggregationBuilder factory = new HistogramAggregationBuilder("foo");
+        factory.field(INT_FIELD_NAME);
+        factory.interval(randomDouble() * 1000);
+
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(Double.NaN, 1.0); });
+        assertThat(ex.getMessage(), startsWith("minBound must be finite, got: "));
+        ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(Double.POSITIVE_INFINITY, 1.0); });
+        assertThat(ex.getMessage(), startsWith("minBound must be finite, got: "));
+        ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(Double.NEGATIVE_INFINITY, 1.0); });
+        assertThat(ex.getMessage(), startsWith("minBound must be finite, got: "));
+
+        ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.0, Double.NaN); });
+        assertThat(ex.getMessage(), startsWith("maxBound must be finite, got: "));
+        ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.0, Double.POSITIVE_INFINITY); });
+        assertThat(ex.getMessage(), startsWith("maxBound must be finite, got: "));
+        ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.0, Double.NEGATIVE_INFINITY); });
+        assertThat(ex.getMessage(), startsWith("maxBound must be finite, got: "));
+
+        ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.5, 0.4); });
+        assertThat(ex.getMessage(), equalTo("maxBound [0.4] must be greater than minBound [0.5]"));
+    }
+
 }