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

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

@@ -43,6 +43,7 @@ import org.elasticsearch.search.internal.SearchContext;
 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 @@ public class PercentilesAggregationBuilder extends LeafOnly<ValuesSource.Numeric
                 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 @@ public class PercentilesAggregationBuilder extends LeafOnly<ValuesSource.Numeric
     }
 
     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 class PercentilesAggregationBuilder extends LeafOnly<ValuesSource.Numeric
         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 @@ public class PercentilesAggregationBuilder extends LeafOnly<ValuesSource.Numeric
     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 @@ public class PercentilesTests extends BaseAggregationTestCase<PercentilesAggrega
         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());
+    }
 }