Fix iteration of empty percentiles throwing Null Pointer Exception (#…

…96668) (#96691) When creating an empty aggregation we set the state to null. Later on this is used by the iterator and leads to null pointer exceptions. Here we just use an empty iterator in case there is no perceniles.
elastic · Jun 8, 2023 · 0e0ddd4 · 0e0ddd4
1 parent 63a493c
commit 0e0ddd4
Show file tree

Hide file tree

Showing 12 changed files with 93 additions and 13 deletions.
diff --git a/docs/changelog/96668.yaml b/docs/changelog/96668.yaml
@@ -0,0 +1,6 @@
+pr: 96668
+summary: Fix iteration of empty percentiles throwing Null Pointer Exception
+area: Aggregations
+type: bug
+issues:
+ - 96626
diff --git a/...in/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java b/...in/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java
@@ -21,13 +21,16 @@
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.zip.DataFormatException;
 
 abstract class AbstractInternalHDRPercentiles extends InternalNumericMetricsAggregation.MultiValue {
 
+    protected static final Iterator<Percentile> EMPTY_ITERATOR = Collections.emptyIterator();
     private static final DoubleHistogram EMPTY_HISTOGRAM_THREE_DIGITS = new DoubleHistogram(3);
     private static final DoubleHistogram EMPTY_HISTOGRAM_ZERO_DIGITS = new EmptyDoubleHdrHistogram();
 

diff --git a/...ava/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java b/...ava/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java
@@ -19,12 +19,16 @@
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 
 abstract class AbstractInternalTDigestPercentiles extends InternalNumericMetricsAggregation.MultiValue {
 
+    protected static final Iterator<Percentile> EMPTY_ITERATOR = Collections.emptyIterator();
+
     // NOTE: using compression = 1.0 empty histograms will track just about 5 centroids.
     // This reduces the amount of data to serialize and deserialize.
     private static final TDigestState EMPTY_HISTOGRAM = new EmptyTDigestState();

diff --git a/...c/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java b/...c/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java
@@ -14,6 +14,7 @@
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Objects;
 
 public class InternalHDRPercentileRanks extends AbstractInternalHDRPercentiles implements PercentileRanks {
     public static final String NAME = "hdr_percentile_ranks";
@@ -43,6 +44,9 @@ public String getWriteableName() {
 
     @Override
     public Iterator<Percentile> iterator() {
+        if (state == null) {
+            return EMPTY_ITERATOR;
+        }
         return new Iter(keys, state);
     }
 
@@ -93,7 +97,7 @@ public static class Iter implements Iterator<Percentile> {
 
         public Iter(double[] values, DoubleHistogram state) {
             this.values = values;
-            this.state = state;
+            this.state = Objects.requireNonNull(state);
             i = 0;
         }
 

diff --git a/...r/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java b/...r/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java
@@ -14,6 +14,7 @@
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Objects;
 
 public class InternalHDRPercentiles extends AbstractInternalHDRPercentiles implements Percentiles {
     public static final String NAME = "hdr_percentiles";
@@ -43,6 +44,9 @@ public String getWriteableName() {
 
     @Override
     public Iterator<Percentile> iterator() {
+        if (state == null) {
+            return EMPTY_ITERATOR;
+        }
         return new Iter(keys, state);
     }
 
@@ -83,7 +87,7 @@ public static class Iter implements Iterator<Percentile> {
 
         public Iter(double[] percents, DoubleHistogram state) {
             this.percents = percents;
-            this.state = state;
+            this.state = Objects.requireNonNull(state);
             i = 0;
         }
 

diff --git a/...in/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java b/...in/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java
@@ -13,6 +13,7 @@
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Objects;
 
 public class InternalTDigestPercentileRanks extends AbstractInternalTDigestPercentiles implements PercentileRanks {
     public static final String NAME = "tdigest_percentile_ranks";
@@ -42,6 +43,9 @@ public String getWriteableName() {
 
     @Override
     public Iterator<Percentile> iterator() {
+        if (state == null) {
+            return EMPTY_ITERATOR;
+        }
         return new Iter(keys, state);
     }
 
@@ -89,7 +93,7 @@ public static class Iter implements Iterator<Percentile> {
 
         public Iter(double[] values, TDigestState state) {
             this.values = values;
-            this.state = state;
+            this.state = Objects.requireNonNull(state);
             i = 0;
         }
 

diff --git a/...c/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java b/...c/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java
@@ -13,6 +13,7 @@
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Objects;
 
 public class InternalTDigestPercentiles extends AbstractInternalTDigestPercentiles implements Percentiles {
     public static final String NAME = "tdigest_percentiles";
@@ -42,6 +43,9 @@ public String getWriteableName() {
 
     @Override
     public Iterator<Percentile> iterator() {
+        if (state == null) {
+            return EMPTY_ITERATOR;
+        }
         return new Iter(keys, state);
     }
 
@@ -79,7 +83,7 @@ public static class Iter implements Iterator<Percentile> {
 
         public Iter(double[] percents, TDigestState state) {
             this.percents = percents;
-            this.state = state;
+            this.state = Objects.requireNonNull(state);
             i = 0;
         }
 

diff --git a/.../test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java b/.../test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java
@@ -24,9 +24,11 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
 
+import static java.util.Collections.emptyMap;
 import static java.util.stream.Collectors.toList;
 import static org.hamcrest.Matchers.equalTo;
 import static org.mockito.Mockito.mock;
@@ -55,7 +57,7 @@ private T createTestInstance(String name, Map<String, Object> metadata, boolean
         for (int i = 0; i < numValues; ++i) {
             values[i] = randomDouble();
         }
-        return createTestInstance(name, metadata, keyed, format, percents, values);
+        return createTestInstance(name, metadata, keyed, format, percents, values, false);
     }
 
     protected abstract T createTestInstance(
@@ -64,7 +66,8 @@ protected abstract T createTestInstance(
         boolean keyed,
         DocValueFormat format,
         double[] percents,
-        double[] values
+        double[] values,
+        boolean empty
     );
 
     protected abstract Class<? extends ParsedPercentiles> implementationClass();
@@ -104,7 +107,7 @@ public void testEmptyRanksXContent() throws IOException {
         boolean keyed = randomBoolean();
         DocValueFormat docValueFormat = randomNumericDocValueFormat();
 
-        T agg = createTestInstance("test", Collections.emptyMap(), keyed, docValueFormat, percents, new double[0]);
+        T agg = createTestInstance("test", Collections.emptyMap(), keyed, docValueFormat, percents, new double[0], false);
 
         for (Percentile percentile : agg) {
             Double value = percentile.getValue();
@@ -147,4 +150,26 @@ public void testEmptyRanksXContent() throws IOException {
 
         assertThat(Strings.toString(builder), equalTo(expected));
     }
+
+    public void testEmptyIterator() {
+        final double[] percents = randomPercents(false);
+
+        final Iterable<?> aggregation = createTestInstance(
+            "test",
+            emptyMap(),
+            false,
+            randomNumericDocValueFormat(),
+            percents,
+            new double[] {},
+            true
+        );
+
+        for (var ignored : aggregation) {
+            fail("empty expected");
+        }
+
+        final Iterator<?> it = aggregation.iterator();
+        assertFalse(it.hasNext());
+        expectThrows(NoSuchElementException.class, it::next);
+    }
 }
diff --git a/.../java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java b/.../java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java
@@ -30,9 +30,13 @@ protected InternalHDRPercentileRanks createTestInstance(
         boolean keyed,
         DocValueFormat format,
         double[] percents,
-        double[] values
+        double[] values,
+        boolean empty
     ) {
 
+        if (empty) {
+            return new InternalHDRPercentileRanks(name, percents, null, keyed, format, metadata);
+        }
         final DoubleHistogram state = new DoubleHistogram(3);
         Arrays.stream(values).forEach(state::recordValue);
 

diff --git a/.../test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java b/.../test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java
@@ -31,9 +31,14 @@ protected InternalHDRPercentiles createTestInstance(
         boolean keyed,
         DocValueFormat format,
         double[] percents,
-        double[] values
+        double[] values,
+        boolean empty
     ) {
 
+        if (empty) {
+            return new InternalHDRPercentiles(name, percents, null, keyed, format, metadata);
+        }
+
         final DoubleHistogram state = new DoubleHistogram(3);
         Arrays.stream(values).forEach(state::recordValue);
 
@@ -76,7 +81,15 @@ public void testIterator() {
             values[i] = randomDouble();
         }
 
-        InternalHDRPercentiles aggregation = createTestInstance("test", emptyMap(), false, randomNumericDocValueFormat(), percents, values);
+        InternalHDRPercentiles aggregation = createTestInstance(
+            "test",
+            emptyMap(),
+            false,
+            randomNumericDocValueFormat(),
+            percents,
+            values,
+            false
+        );
 
         Iterator<Percentile> iterator = aggregation.iterator();
         Iterator<String> nameIterator = aggregation.valueNames().iterator();

diff --git a/...a/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java b/...a/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java
@@ -29,8 +29,12 @@ protected InternalTDigestPercentileRanks createTestInstance(
         boolean keyed,
         DocValueFormat format,
         double[] percents,
-        double[] values
+        double[] values,
+        boolean empty
     ) {
+        if (empty) {
+            return new InternalTDigestPercentileRanks(name, percents, null, keyed, format, metadata);
+        }
         final TDigestState state = new TDigestState(100);
         Arrays.stream(values).forEach(state::add);
 

diff --git a/...t/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java b/...t/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java
@@ -30,8 +30,12 @@ protected InternalTDigestPercentiles createTestInstance(
         boolean keyed,
         DocValueFormat format,
         double[] percents,
-        double[] values
+        double[] values,
+        boolean empty
     ) {
+        if (empty) {
+            return new InternalTDigestPercentiles(name, percents, null, keyed, format, metadata);
+        }
         final TDigestState state = new TDigestState(100);
         Arrays.stream(values).forEach(state::add);
 
@@ -126,7 +130,8 @@ public void testIterator() {
             false,
             randomNumericDocValueFormat(),
             percents,
-            values
+            values,
+            false
         );
 
         Iterator<Percentile> iterator = aggregation.iterator();