Fix NPE when values is omitted on percentile_ranks agg (#26046)

An array of values is required because there is no default (or
reasonable way to set a default).  But validation for values
only happens if it is actually set.  If the values param is omitted
entirely than the agg builder will NPE.

Author: polyfractal

core/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java

@@ -49,7 +49,7 @@ public abstract <T> void declareField(BiConsumer<Value, T> consumer, ContextPars
     /**
      * Declares named objects in the style of aggregations. These are named
      * inside and object like this:
-     * 
+     *
      * <pre>
      * <code>
      * {
@@ -62,7 +62,7 @@ public abstract <T> void declareField(BiConsumer<Value, T> consumer, ContextPars
      * }
      * </code>
      * </pre>
-     * 
+     *
      * Unlike the other version of this method, "ordered" mode (arrays of
      * objects) is not supported.
      *
@@ -82,7 +82,7 @@ public abstract <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer
     /**
      * Declares named objects in the style of highlighting's field element.
      * These are usually named inside and object like this:
-     * 
+     *
      * <pre>
      * <code>
      * {
@@ -96,9 +96,9 @@ public abstract <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer
      * }
      * </code>
      * </pre>
-     * 
+     *
      * but, when order is important, some may be written this way:
-     * 
+     *
      * <pre>
      * <code>
      * {
@@ -112,7 +112,7 @@ public abstract <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer
      * }
      * </code>
      * </pre>
-     * 
+     *
      * This is because json doesn't enforce ordering. Elasticsearch reads it in
      * the order sent but tools that generate json are free to put object
      * members in an unordered Map, jumbling them. Thus, if you care about order
@@ -134,6 +134,8 @@ public abstract <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer
     public abstract <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedObjectParser<T, Context> namedObjectParser,
             Consumer<Value> orderedModeCallback, ParseField parseField);
 
+    public abstract String getName();
+
     public <T> void declareField(BiConsumer<Value, T> consumer, CheckedFunction<XContentParser, T, IOException> parser,
             ParseField parseField, ValueType type) {
         if (parser == null) {

core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java

@@ -295,6 +295,10 @@ public <T> void declareNamedObjects(BiConsumer<Value, List<T>> consumer, NamedOb
         }
     }
 
+    public String getName() {
+        return objectParser.getName();
+    }
+
     private Consumer<Target> wrapOrderedModeCallBack(Consumer<Value> callback) {
         return (target) -> {
             if (target.targetObject != null) {

core/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java

@@ -247,15 +247,15 @@ public static SignificantTermsAggregationBuilder significantTerms(String name) {
         return new SignificantTermsAggregationBuilder(name, null);
     }
 
-    
+
     /**
      * Create a new {@link SignificantTextAggregationBuilder} aggregation with the given name and text field name
      */
     public static SignificantTextAggregationBuilder significantText(String name, String fieldName) {
         return new SignificantTextAggregationBuilder(name, fieldName);
-    }    
-        
-    
+    }
+
+
     /**
      * Create a new {@link DateHistogramAggregationBuilder} aggregation with the given
      * name.
@@ -304,8 +304,8 @@ public static PercentilesAggregationBuilder percentiles(String name) {
     /**
      * Create a new {@link PercentileRanks} aggregation with the given name.
      */
-    public static PercentileRanksAggregationBuilder percentileRanks(String name) {
-        return new PercentileRanksAggregationBuilder(name);
+    public static PercentileRanksAggregationBuilder percentileRanks(String name, double[] values) {
+        return new PercentileRanksAggregationBuilder(name, values);
     }
 
     /**

core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java

@@ -22,6 +22,8 @@
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
+import org.elasticsearch.common.xcontent.ContextParser;
 import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -42,7 +44,11 @@
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.List;
 import java.util.Objects;
+import java.util.function.BiConsumer;
+
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
 
 public class PercentileRanksAggregationBuilder extends LeafOnly<ValuesSource.Numeric, PercentileRanksAggregationBuilder> {
     public static final String NAME = PercentileRanks.TYPE_NAME;
@@ -53,7 +59,7 @@ private static class TDigestOptions {
         Double compression;
     }
 
-    private static final ObjectParser<TDigestOptions, Void> TDIGEST_OPTIONS_PARSER =
+    private static final ObjectParser<TDigestOptions, String> TDIGEST_OPTIONS_PARSER =
             new ObjectParser<>(PercentilesMethod.TDIGEST.getParseField().getPreferredName(), TDigestOptions::new);
     static {
         TDIGEST_OPTIONS_PARSER.declareDouble((opts, compression) -> opts.compression = compression, new ParseField("compression"));
@@ -63,22 +69,22 @@ private static class HDROptions {
         Integer numberOfSigDigits;
     }
 
-    private static final ObjectParser<HDROptions, Void> HDR_OPTIONS_PARSER =
+    private static final ObjectParser<HDROptions, String> HDR_OPTIONS_PARSER =
             new ObjectParser<>(PercentilesMethod.HDR.getParseField().getPreferredName(), HDROptions::new);
     static {
         HDR_OPTIONS_PARSER.declareInt((opts, numberOfSigDigits) -> opts.numberOfSigDigits = numberOfSigDigits,
                 new ParseField("number_of_significant_value_digits"));
     }
 
-    private static final ObjectParser<PercentileRanksAggregationBuilder, Void> PARSER;
+    // The builder requires two parameters for the constructor: aggregation name and values array.  The
+    // agg name is supplied externally via the Parser's context (as a String), while the values array
+    // is parsed from the request and supplied to the ConstructingObjectParser as a ctor argument
+    private static final ConstructingObjectParser<PercentileRanksAggregationBuilder, String> PARSER;
     static {
-        PARSER = new ObjectParser<>(PercentileRanksAggregationBuilder.NAME);
+        PARSER = new ConstructingObjectParser<>(PercentileRanksAggregationBuilder.NAME, false,
+            (a, context) -> new PercentileRanksAggregationBuilder(context, (List) a[0]));
         ValuesSourceParserHelper.declareNumericFields(PARSER, true, false, false);
-
-        PARSER.declareDoubleArray(
-                (b, v) -> b.values(v.stream().mapToDouble(Double::doubleValue).toArray()),
-                VALUES_FIELD);
-
+        PARSER.declareDoubleArray(constructorArg(), VALUES_FIELD);
         PARSER.declareBoolean(PercentileRanksAggregationBuilder::keyed, PercentilesAggregationBuilder.KEYED_FIELD);
 
         PARSER.declareField((b, v) -> {
@@ -97,7 +103,8 @@ private static class HDROptions {
     }
 
     public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException {
-        return PARSER.parse(parser, new PercentileRanksAggregationBuilder(aggregationName), null);
+        // the aggregation name is supplied to the parser as a Context. See note at top of Parser for more details
+        return PARSER.parse(parser, aggregationName);
     }
 
     private double[] values;
@@ -106,8 +113,21 @@ public static AggregationBuilder parse(String aggregationName, XContentParser pa
     private double compression = 100.0;
     private boolean keyed = true;
 
-    public PercentileRanksAggregationBuilder(String name) {
+    private PercentileRanksAggregationBuilder(String name, List<Double> values) {
+        this(name, values.stream().mapToDouble(Double::doubleValue).toArray());
+    }
+
+    public PercentileRanksAggregationBuilder(String name, double[] values) {
         super(name, ValuesSourceType.NUMERIC, ValueType.NUMERIC);
+        if (values == null) {
+            throw new IllegalArgumentException("[values] must not be null: [" + name + "]");
+        }
+        if (values.length == 0) {
+            throw new IllegalArgumentException("[values] must not be an empty array: [" + name + "]");
+        }
+        double[] sortedValues = Arrays.copyOf(values, values.length);
+        Arrays.sort(sortedValues);
+        this.values = sortedValues;
     }
 
     /**
@@ -131,19 +151,6 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
         method.writeTo(out);
     }
 
-    /**
-     * Set the values to compute percentiles from.
-     */
-    public PercentileRanksAggregationBuilder values(double... values) {
-        if (values == null) {
-            throw new IllegalArgumentException("[values] must not be null: [" + name + "]");
-        }
-        double[] sortedValues = Arrays.copyOf(values, values.length);
-        Arrays.sort(sortedValues);
-        this.values = sortedValues;
-        return this;
-    }
-
     /**
      * Get the values to compute percentiles from.
      */

core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java

@@ -21,6 +21,8 @@
 
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParsingException;
+import org.elasticsearch.common.xcontent.AbstractObjectParser;
+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.script.Script;
@@ -31,32 +33,32 @@ public final class ValuesSourceParserHelper {
 
     private ValuesSourceParserHelper() {} // utility class, no instantiation
 
-    public static void declareAnyFields(
-            ObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource, ?>, Void> objectParser,
+    public static <T> void declareAnyFields(
+            AbstractObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource, ?>, T> objectParser,
             boolean scriptable, boolean formattable) {
         declareFields(objectParser, scriptable, formattable, false, null);
     }
 
-    public static void declareNumericFields(
-            ObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource.Numeric, ?>, Void> objectParser,
+    public static <T> void declareNumericFields(
+            AbstractObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource.Numeric, ?>, T> objectParser,
             boolean scriptable, boolean formattable, boolean timezoneAware) {
         declareFields(objectParser, scriptable, formattable, timezoneAware, ValueType.NUMERIC);
     }
 
-    public static void declareBytesFields(
-            ObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource.Bytes, ?>, Void> objectParser,
+    public static <T> void declareBytesFields(
+            AbstractObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource.Bytes, ?>, T> objectParser,
             boolean scriptable, boolean formattable) {
         declareFields(objectParser, scriptable, formattable, false, ValueType.STRING);
     }
 
-    public static void declareGeoFields(
-            ObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource.GeoPoint, ?>, Void> objectParser,
+    public static <T> void declareGeoFields(
+            AbstractObjectParser<? extends ValuesSourceAggregationBuilder<ValuesSource.GeoPoint, ?>, T> objectParser,
             boolean scriptable, boolean formattable) {
         declareFields(objectParser, scriptable, formattable, false, ValueType.GEOPOINT);
     }
 
-    private static <VS extends ValuesSource> void declareFields(
-            ObjectParser<? extends ValuesSourceAggregationBuilder<VS, ?>, Void> objectParser,
+    private static <VS extends ValuesSource, T> void declareFields(
+            AbstractObjectParser<? extends ValuesSourceAggregationBuilder<VS, ?>, T> objectParser,
             boolean scriptable, boolean formattable, boolean timezoneAware, ValueType targetValueType) {
 
 
@@ -99,4 +101,6 @@ private static <VS extends ValuesSource> void declareFields(
         }
     }
 
+
+
 }