Reject multiple methods in percentiles aggregation (#26163)

Currently the `percentiles` aggregation allows specifying both possible methods
in the query DSL, but only the later one is used. This changes it to rejecting
such requests with an error. Setting the method multiple times via the java API
still works (and the last one wins).

Closes #26095

Author: cbuescher

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

@@ -43,6 +43,7 @@
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Objects;
+import java.util.function.Consumer;
 
 public class PercentilesAggregationBuilder extends LeafOnly<ValuesSource.Numeric, PercentilesAggregationBuilder> {
     public static final String NAME = Percentiles.TYPE_NAME;
@@ -76,7 +77,7 @@ private static class HDROptions {
                 NUMBER_SIGNIFICANT_DIGITS_FIELD);
     }
 
-    private static final ObjectParser<PercentilesAggregationBuilder, Void> PARSER;
+    private static final ObjectParser<InternalBuilder, Void> PARSER;
     static {
         PARSER = new ObjectParser<>(PercentilesAggregationBuilder.NAME);
         ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false);
@@ -103,7 +104,26 @@ private static class HDROptions {
     }
 
     public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException {
-        return PARSER.parse(parser, new PercentilesAggregationBuilder(aggregationName), null);
+        InternalBuilder internal = PARSER.parse(parser, new InternalBuilder(aggregationName), null);
+        // we need to return a PercentilesAggregationBuilder for equality checks to work
+        PercentilesAggregationBuilder returnedAgg = new PercentilesAggregationBuilder(internal.name);
+        setIfNotNull(returnedAgg::valueType, internal.valueType());
+        setIfNotNull(returnedAgg::format, internal.format());
+        setIfNotNull(returnedAgg::missing, internal.missing());
+        setIfNotNull(returnedAgg::field, internal.field());
+        setIfNotNull(returnedAgg::script, internal.script());
+        setIfNotNull(returnedAgg::method, internal.method());
+        setIfNotNull(returnedAgg::percentiles, internal.percentiles());
+        returnedAgg.keyed(internal.keyed());
+        returnedAgg.compression(internal.compression());
+        returnedAgg.numberOfSignificantValueDigits(internal.numberOfSignificantValueDigits());
+        return returnedAgg;
+    }
+
+    private static <T> void setIfNotNull(Consumer<T> consumer, T value) {
+        if (value != null) {
+            consumer.accept(value);
+        }
     }
 
     private double[] percents = DEFAULT_PERCENTS;
@@ -144,6 +164,9 @@ public PercentilesAggregationBuilder percentiles(double... percents) {
         if (percents == null) {
             throw new IllegalArgumentException("[percents] must not be null: [" + name + "]");
         }
+        if (percents.length == 0) {
+            throw new IllegalArgumentException("[percents] must not be empty: [" + name + "]");
+        }
         double[] sortedPercents = Arrays.copyOf(percents, percents.length);
         Arrays.sort(sortedPercents);
         this.percents = sortedPercents;
@@ -293,4 +316,29 @@ protected int innerHashCode() {
     public String getType() {
         return NAME;
     }
+
+    /**
+     * Private specialization of this builder that should only be used by the parser, this enables us to
+     * overwrite {@link #method()} to check that it is not defined twice in xContent and throw
+     * an error, while the Java API should allow to overwrite the method
+     */
+    private static class InternalBuilder extends PercentilesAggregationBuilder {
+
+        private boolean setOnce = false;
+
+        private InternalBuilder(String name) {
+            super(name);
+        }
+
+        @Override
+        public InternalBuilder method(PercentilesMethod method) {
+            if (setOnce == false) {
+                super.method(method);
+                setOnce = true;
+                return this;
+            } else {
+                throw new IllegalStateException("Only one percentiles method should be declared.");
+            }
+        }
+    }
 }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java

@@ -19,9 +19,14 @@
 
 package org.elasticsearch.search.aggregations.metrics;
 
+import org.elasticsearch.common.ParsingException;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.search.aggregations.BaseAggregationTestCase;
 import org.elasticsearch.search.aggregations.metrics.percentiles.PercentilesAggregationBuilder;
 
+import java.io.IOException;
+
 public class PercentilesTests extends BaseAggregationTestCase<PercentilesAggregationBuilder> {
 
     @Override
@@ -55,4 +60,37 @@ protected PercentilesAggregationBuilder createTestAggregatorBuilder() {
         return factory;
     }
 
+    public void testNullOrEmptyPercentilesThrows() throws IOException {
+        PercentilesAggregationBuilder builder = new PercentilesAggregationBuilder("testAgg");
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(null));
+        assertEquals("[percents] must not be null: [testAgg]", ex.getMessage());
+
+        ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(new double[0]));
+        assertEquals("[percents] must not be empty: [testAgg]", ex.getMessage());
+    }
+
+    public void testExceptionMultipleMethods() throws IOException {
+        final String illegalAgg = "{\n" +
+            "       \"percentiles\": {\n" +
+            "           \"field\": \"load_time\",\n" +
+            "           \"percents\": [99],\n" +
+            "           \"tdigest\": {\n" +
+            "               \"compression\": 200\n" +
+            "           },\n" +
+            "           \"hdr\": {\n" +
+            "               \"number_of_significant_value_digits\": 3\n" +
+            "           }\n" +
+            "   }\n" +
+            "}";
+        XContentParser parser = createParser(JsonXContent.jsonXContent, illegalAgg);
+        assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
+        assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
+        ParsingException e = expectThrows(ParsingException.class,
+                () -> PercentilesAggregationBuilder.parse("myPercentiles", parser));
+        assertEquals(
+                "ParsingException[[percentiles] failed to parse field [hdr]]; "
+                + "nested: IllegalStateException[Only one percentiles method should be declared.];; "
+                + "java.lang.IllegalStateException: Only one percentiles method should be declared.",
+                e.getDetailedMessage());
+    }
 }