From 0e0ddd4592b48615dd7b783294e539c0bc36c177 Mon Sep 17 00:00:00 2001 From: Salvatore Campagna <93581129+salvatore-campagna@users.noreply.github.com> Date: Thu, 8 Jun 2023 11:00:29 +0200 Subject: [PATCH] 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. --- docs/changelog/96668.yaml | 6 ++++ .../AbstractInternalHDRPercentiles.java | 3 ++ .../AbstractInternalTDigestPercentiles.java | 4 +++ .../metrics/InternalHDRPercentileRanks.java | 6 +++- .../metrics/InternalHDRPercentiles.java | 6 +++- .../InternalTDigestPercentileRanks.java | 6 +++- .../metrics/InternalTDigestPercentiles.java | 6 +++- .../metrics/AbstractPercentilesTestCase.java | 31 +++++++++++++++++-- .../InternalHDRPercentilesRanksTests.java | 6 +++- .../metrics/InternalHDRPercentilesTests.java | 17 ++++++++-- .../InternalTDigestPercentilesRanksTests.java | 6 +++- .../InternalTDigestPercentilesTests.java | 9 ++++-- 12 files changed, 93 insertions(+), 13 deletions(-) create mode 100644 docs/changelog/96668.yaml diff --git a/docs/changelog/96668.yaml b/docs/changelog/96668.yaml new file mode 100644 index 0000000000000..483c0f462743f --- /dev/null +++ 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/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java index 4abe4200e88bc..a1d6800c66b5c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java @@ -21,6 +21,8 @@ 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; @@ -28,6 +30,7 @@ abstract class AbstractInternalHDRPercentiles extends InternalNumericMetricsAggregation.MultiValue { + protected static final Iterator 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/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java index ed0ab17ce1004..4fb10c4b170c7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java +++ b/server/src/main/java/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 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/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java index 7a2bf3f5b820d..640e66c6fc96d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java +++ b/server/src/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 iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -93,7 +97,7 @@ public static class Iter implements Iterator { public Iter(double[] values, DoubleHistogram state) { this.values = values; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java index d8a71d19506e3..1ba4656196c77 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java +++ b/server/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 iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -83,7 +87,7 @@ public static class Iter implements Iterator { public Iter(double[] percents, DoubleHistogram state) { this.percents = percents; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java index c0c79afc494cf..9fc1c5980faf9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java +++ b/server/src/main/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 iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -89,7 +93,7 @@ public static class Iter implements Iterator { public Iter(double[] values, TDigestState state) { this.values = values; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java index bf4f08f126797..f3aea151b1112 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java +++ b/server/src/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 iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -79,7 +83,7 @@ public static class Iter implements Iterator { public Iter(double[] percents, TDigestState state) { this.percents = percents; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java index 6f8b554500433..d5867c8ebefcd 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java +++ b/server/src/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 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 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/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java index 47015201f58f5..ac43929878f47 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java +++ b/server/src/test/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/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java index 6e986f69c1c50..1c9cb88ee3e2b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java +++ b/server/src/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 iterator = aggregation.iterator(); Iterator nameIterator = aggregation.valueNames().iterator(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java index 27d3fafee1319..cdefca8f72eaf 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java +++ b/server/src/test/java/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/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java index 5c8e6aa2700c9..1df9e8152101f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java +++ b/server/src/test/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 iterator = aggregation.iterator();