Skip to content

Commit

Permalink
Fix NPE when values is omitted on percentile_ranks agg (#26046)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@polyfractal
polyfractal committed Aug 15, 2017
1 parent 3072953 commit 9bd5b83
Show file tree
Hide file tree
Showing 11 changed files with 245 additions and 160 deletions.
14 changes: 8 additions & 6 deletions core/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
* {
Expand All @@ -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.
*
Expand All @@ -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>
* {
Expand All @@ -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>
* {
Expand All @@ -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
Expand All @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/org/elasticsearch/common/xcontent/ConstructingObjectParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 6 additions & 6 deletions core/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}

/**
Expand Down
55 changes: 31 additions & 24 deletions ...sticsearch/search/aggregations/metrics/percentiles/PercentileRanksAggregationBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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"));
Expand All @@ -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) -> {
Expand All @@ -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;
Expand All @@ -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;
}

/**
Expand All @@ -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.
*/
Expand Down
24 changes: 14 additions & 10 deletions ...src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParserHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {


Expand Down Expand Up @@ -99,4 +101,6 @@ private static <VS extends ValuesSource> void declareFields(
}
}



}
Loading

0 comments on commit 9bd5b83

Please sign in to comment.