Merge pull request #17547: Add shuffling xContent to aggregation tests

This adds shuffling of xContent similar to #17521 to the aggregation and pipeline aggregation base test.
The additional shuffling uncovered that some aggregation builders internally store some properties in a 
way that made the equals() testing fail when the xContent is shuffled.
For TopHitsAggregatorBuilder, the internal scriptFields parameter was changed to a set because the order 
they appear in the xContent should not matter. For FiltersAggregatorBuilder, the internal list of KeyedFilters 
is sorted by key now. As a side effect, the keys in the aggregation response are now not always in the same 
order as the filters in the query, but sorted by key as well (unless they are anonymous).

core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregatorBuilder.java

@@ -25,14 +25,15 @@ import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.query.EmptyQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.AggregatorBuilder;
-import org.elasticsearch.search.aggregations.AggregatorFactory;
 import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
+import org.elasticsearch.search.aggregations.AggregatorFactory;
 import org.elasticsearch.search.aggregations.bucket.filters.FiltersAggregator.KeyedFilter;
 import org.elasticsearch.search.aggregations.support.AggregationContext;
 
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 
@@ -57,7 +58,9 @@ public class FiltersAggregatorBuilder extends AggregatorBuilder<FiltersAggregato
 
     private FiltersAggregatorBuilder(String name, List<KeyedFilter> filters) {
         super(name, InternalFilters.TYPE);
-        this.filters = filters;
+        // internally we want to have a fixed order of filters, regardless of the order of the filters in the request
+        this.filters = new ArrayList<>(filters);
+        Collections.sort(this.filters, (KeyedFilter kf1, KeyedFilter kf2) -> kf1.key().compareTo(kf2.key()));
         this.keyed = true;
     }
 
@@ -92,6 +95,13 @@ public class FiltersAggregatorBuilder extends AggregatorBuilder<FiltersAggregato
         return otherBucket;
     }
 
+    /**
+     * Get the filters. This will be an unmodifiable list
+     */
+    public List<KeyedFilter> filters() {
+        return Collections.unmodifiableList(this.filters);
+    }
+
     /**
      * Set the key to use for the bucket for documents not matching any
      * filter.

core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorBuilder.java

@@ -42,8 +42,10 @@ import org.elasticsearch.search.sort.SortOrder;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 
 public class TopHitsAggregatorBuilder extends AggregatorBuilder<TopHitsAggregatorBuilder> {
 
@@ -58,7 +60,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder<TopHitsAggregato
     private HighlightBuilder highlightBuilder;
     private List<String> fieldNames;
     private List<String> fieldDataFields;
-    private List<ScriptField> scriptFields;
+    private Set<ScriptField> scriptFields;
     private FetchSourceContext fetchSourceContext;
 
     public TopHitsAggregatorBuilder(String name) {
@@ -378,7 +380,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder<TopHitsAggregato
             throw new IllegalArgumentException("scriptField [script] must not be null: [" + name + "]");
         }
         if (scriptFields == null) {
-            scriptFields = new ArrayList<>();
+            scriptFields = new HashSet<>();
         }
         scriptFields.add(new ScriptField(name, script, ignoreFailure));
         return this;
@@ -389,7 +391,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder<TopHitsAggregato
             throw new IllegalArgumentException("[scriptFields] must not be null: [" + name + "]");
         }
         if (this.scriptFields == null) {
-            this.scriptFields = new ArrayList<>();
+            this.scriptFields = new HashSet<>();
         }
         this.scriptFields.addAll(scriptFields);
         return this;
@@ -398,7 +400,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder<TopHitsAggregato
     /**
      * Gets the script fields.
      */
-    public List<ScriptField> scriptFields() {
+    public Set<ScriptField> scriptFields() {
         return scriptFields;
     }
 
@@ -541,7 +543,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder<TopHitsAggregato
         factory.highlightBuilder = in.readOptionalWriteable(HighlightBuilder::new);
         if (in.readBoolean()) {
             int size = in.readVInt();
-            List<ScriptField> scriptFields = new ArrayList<>(size);
+            Set<ScriptField> scriptFields = new HashSet<>(size);
             for (int i = 0; i < size; i++) {
                 scriptFields.add(ScriptField.PROTOTYPE.readFrom(in));
             }

core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java

@@ -42,6 +42,7 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 
 public class TopHitsAggregatorFactory extends AggregatorFactory<TopHitsAggregatorFactory> {
 
@@ -54,12 +55,12 @@ public class TopHitsAggregatorFactory extends AggregatorFactory<TopHitsAggregato
     private final HighlightBuilder highlightBuilder;
     private final List<String> fieldNames;
     private final List<String> fieldDataFields;
-    private final List<ScriptField> scriptFields;
+    private final Set<ScriptField> scriptFields;
     private final FetchSourceContext fetchSourceContext;
 
     public TopHitsAggregatorFactory(String name, Type type, int from, int size, boolean explain, boolean version, boolean trackScores,
             List<SortBuilder<?>> sorts, HighlightBuilder highlightBuilder, List<String> fieldNames, List<String> fieldDataFields,
-            List<ScriptField> scriptFields, FetchSourceContext fetchSourceContext, AggregationContext context, AggregatorFactory<?> parent,
+            Set<ScriptField> scriptFields, FetchSourceContext fetchSourceContext, AggregationContext context, AggregatorFactory<?> parent,
             AggregatorFactories.Builder subFactories, Map<String, Object> metaData) throws IOException {
         super(name, type, context, parent, subFactories, metaData);
         this.from = from;

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregatorBuilder.java

@@ -39,9 +39,9 @@ import java.util.Objects;
 public abstract class PipelineAggregatorBuilder<PAB extends PipelineAggregatorBuilder<PAB>> extends ToXContentToBytes
         implements NamedWriteable<PipelineAggregatorBuilder<PAB>>, ToXContent {
 
-    protected String name;
+    protected final String name;
-    protected String type;
+    protected final String type;
-    protected String[] bucketsPaths;
+    protected final String[] bucketsPaths;
     protected Map<String, Object> metaData;
 
     /**

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptParser.java

@@ -27,6 +27,7 @@ import org.elasticsearch.script.Script;
 import org.elasticsearch.script.Script.ScriptField;
 import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
+
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregatorBuilder.java

@@ -24,9 +24,9 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.Script.ScriptField;
+import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilder;
-import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
 import org.elasticsearch.search.aggregations.support.format.ValueFormat;
 import org.elasticsearch.search.aggregations.support.format.ValueFormatter;
 
@@ -34,8 +34,9 @@ import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Map.Entry;
+import java.util.Objects;
+import java.util.TreeMap;
 
 public class BucketScriptPipelineAggregatorBuilder extends PipelineAggregatorBuilder<BucketScriptPipelineAggregatorBuilder> {
 
@@ -48,7 +49,8 @@ public class BucketScriptPipelineAggregatorBuilder extends PipelineAggregatorBui
     private GapPolicy gapPolicy = GapPolicy.SKIP;
 
     public BucketScriptPipelineAggregatorBuilder(String name, Map<String, String> bucketsPathsMap, Script script) {
-        super(name, BucketScriptPipelineAggregator.TYPE.name(), bucketsPathsMap.values().toArray(new String[bucketsPathsMap.size()]));
+        super(name, BucketScriptPipelineAggregator.TYPE.name(), new TreeMap<>(bucketsPathsMap).values()
+                .toArray(new String[bucketsPathsMap.size()]));
         this.bucketsPathsMap = bucketsPathsMap;
         this.script = script;
     }

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorParser.java

@@ -27,6 +27,7 @@ import org.elasticsearch.script.Script;
 import org.elasticsearch.script.Script.ScriptField;
 import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
+
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregatorBuilder.java

@@ -24,17 +24,18 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.Script.ScriptField;
+import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilder;
-import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
 import org.elasticsearch.search.aggregations.pipeline.bucketscript.BucketScriptParser;
 
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Map.Entry;
+import java.util.Objects;
+import java.util.TreeMap;
 
 public class BucketSelectorPipelineAggregatorBuilder extends PipelineAggregatorBuilder<BucketSelectorPipelineAggregatorBuilder> {
 
@@ -43,10 +44,11 @@ public class BucketSelectorPipelineAggregatorBuilder extends PipelineAggregatorB
 
     private Script script;
     private GapPolicy gapPolicy = GapPolicy.SKIP;
-    private Map<String, String> bucketsPathsMap;
+    private final Map<String, String> bucketsPathsMap;
 
     public BucketSelectorPipelineAggregatorBuilder(String name, Map<String, String> bucketsPathsMap, Script script) {
-        super(name, BucketSelectorPipelineAggregator.TYPE.name(), bucketsPathsMap.values().toArray(new String[bucketsPathsMap.size()]));
+        super(name, BucketSelectorPipelineAggregator.TYPE.name(), new TreeMap<>(bucketsPathsMap).values()
+                .toArray(new String[bucketsPathsMap.size()]));
         this.bucketsPathsMap = bucketsPathsMap;
         this.script = script;
     }

core/src/test/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java

@@ -36,8 +36,11 @@ import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.settings.SettingsModule;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.EnvironmentModule;
 import org.elasticsearch.index.Index;
@@ -215,8 +218,13 @@ public abstract class BaseAggregationTestCase<AB extends AggregatorBuilder<AB>>
     public void testFromXContent() throws IOException {
         AB testAgg = createTestAggregatorBuilder();
         AggregatorFactories.Builder factoriesBuilder = AggregatorFactories.builder().addAggregator(testAgg);
-        String contentString = factoriesBuilder.toString();
+        XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
-        XContentParser parser = XContentFactory.xContent(contentString).createParser(contentString);
+        if (randomBoolean()) {
+            builder.prettyPrint();
+        }
+        factoriesBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        XContentBuilder shuffled = shuffleXContent(builder, Collections.emptySet());
+        XContentParser parser = XContentFactory.xContent(shuffled.bytes()).createParser(shuffled.bytes());
         QueryParseContext parseContext = new QueryParseContext(queriesRegistry);
         parseContext.reset(parser);
         parseContext.parseFieldMatcher(parseFieldMatcher);

core/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java

@@ -36,8 +36,11 @@ import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.settings.SettingsModule;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.EnvironmentModule;
 import org.elasticsearch.index.Index;
@@ -216,9 +219,14 @@ public abstract class BasePipelineAggregationTestCase<AF extends PipelineAggrega
     public void testFromXContent() throws IOException {
         AF testAgg = createTestAggregatorFactory();
         AggregatorFactories.Builder factoriesBuilder = AggregatorFactories.builder().skipResolveOrder().addPipelineAggregator(testAgg);
-        String contentString = factoriesBuilder.toString();
+        logger.info("Content string: {}", factoriesBuilder);
-        logger.info("Content string: {}", contentString);
+        XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
-        XContentParser parser = XContentFactory.xContent(contentString).createParser(contentString);
+        if (randomBoolean()) {
+            builder.prettyPrint();
+        }
+        factoriesBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        XContentBuilder shuffled = shuffleXContent(builder, Collections.emptySet());
+        XContentParser parser = XContentFactory.xContent(shuffled.bytes()).createParser(shuffled.bytes());
         QueryParseContext parseContext = new QueryParseContext(queriesRegistry);
         parseContext.reset(parser);
         parseContext.parseFieldMatcher(parseFieldMatcher);

core/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersIT.java

@@ -34,7 +34,9 @@ import org.elasticsearch.test.ESIntegTestCase;
 import org.hamcrest.Matchers;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 
@@ -112,7 +114,8 @@ public class FiltersIT extends ESIntegTestCase {
 
     public void testSimple() throws Exception {
         SearchResponse response = client().prepareSearch("idx").addAggregation(
-                filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), new KeyedFilter("tag2", termQuery("tag", "tag2"))))
+                filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")),
+                        new KeyedFilter("tag2", termQuery("tag", "tag2")))))
                 .execute().actionGet();
 
         assertSearchResponse(response);
@@ -137,7 +140,8 @@ public class FiltersIT extends ESIntegTestCase {
     public void testEmptyFilterDeclarations() throws Exception {
         QueryBuilder<?> emptyFilter = new BoolQueryBuilder();
         SearchResponse response = client().prepareSearch("idx")
-                .addAggregation(filters("tags", new KeyedFilter("all", emptyFilter), new KeyedFilter("tag1", termQuery("tag", "tag1"))))
+                .addAggregation(filters("tags", randomOrder(new KeyedFilter("all", emptyFilter),
+                        new KeyedFilter("tag1", termQuery("tag", "tag1")))))
                 .execute().actionGet();
 
         assertSearchResponse(response);
@@ -154,8 +158,8 @@ public class FiltersIT extends ESIntegTestCase {
 
     public void testWithSubAggregation() throws Exception {
         SearchResponse response = client().prepareSearch("idx")
-                .addAggregation(filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")),
+                .addAggregation(filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")),
-                        new KeyedFilter("tag2", termQuery("tag", "tag2"))).subAggregation(avg("avg_value").field("value")))
+                        new KeyedFilter("tag2", termQuery("tag", "tag2")))).subAggregation(avg("avg_value").field("value")))
                 .execute().actionGet();
 
         assertSearchResponse(response);
@@ -254,9 +258,9 @@ public class FiltersIT extends ESIntegTestCase {
         try {
             client().prepareSearch("idx")
                     .addAggregation(
-                    filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), new KeyedFilter("tag2", termQuery("tag", "tag2")))
+                            filters("tags",
-                                    .subAggregation(avg("avg_value"))
+                            randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")),
-                    )
+                                    new KeyedFilter("tag2", termQuery("tag", "tag2")))).subAggregation(avg("avg_value")))
                     .execute().actionGet();
 
             fail("expected execution to fail - an attempt to have a context based numeric sub-aggregation, but there is not value source" +
@@ -314,8 +318,8 @@ public class FiltersIT extends ESIntegTestCase {
 
     public void testOtherBucket() throws Exception {
         SearchResponse response = client().prepareSearch("idx").addAggregation(
-                filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), new KeyedFilter("tag2", termQuery("tag", "tag2")))
+                filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")),
-                        .otherBucket(true))
+                        new KeyedFilter("tag2", termQuery("tag", "tag2")))).otherBucket(true))
                 .execute().actionGet();
 
         assertSearchResponse(response);
@@ -341,8 +345,8 @@ public class FiltersIT extends ESIntegTestCase {
 
     public void testOtherNamedBucket() throws Exception {
         SearchResponse response = client().prepareSearch("idx")
-                .addAggregation(filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")),
+                .addAggregation(filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")),
-                        new KeyedFilter("tag2", termQuery("tag", "tag2"))).otherBucket(true).otherBucketKey("foobar"))
+                        new KeyedFilter("tag2", termQuery("tag", "tag2")))).otherBucket(true).otherBucketKey("foobar"))
                 .execute().actionGet();
 
         assertSearchResponse(response);
@@ -397,8 +401,8 @@ public class FiltersIT extends ESIntegTestCase {
 
     public void testOtherWithSubAggregation() throws Exception {
         SearchResponse response = client().prepareSearch("idx")
-                .addAggregation(filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")),
+                .addAggregation(filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")),
-                        new KeyedFilter("tag2", termQuery("tag", "tag2"))).otherBucket(true)
+                        new KeyedFilter("tag2", termQuery("tag", "tag2")))).otherBucket(true)
                                 .subAggregation(avg("avg_value").field("value")))
                 .execute().actionGet();
 
@@ -485,4 +489,8 @@ public class FiltersIT extends ESIntegTestCase {
         assertThat(other.getDocCount(), is(0L));
     }
 
+    private static KeyedFilter[] randomOrder(KeyedFilter... filters) {
+        Collections.shuffle(Arrays.asList(filters), random());
+        return filters;
+    }
 }

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

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.search.aggregations.metrics;
 
+import org.elasticsearch.index.query.MatchNoneQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryBuilders;
 import org.elasticsearch.search.aggregations.BaseAggregationTestCase;
@@ -61,4 +62,20 @@ public class FiltersTests extends BaseAggregationTestCase<FiltersAggregatorBuild
         return factory;
     }
 
+    /**
+     * Test that when passing in keyed filters as list or array, the list stored internally is sorted by key
+     * Also check the list passed in is not modified by this but rather copied
+     */
+    public void testFiltersSortedByKey() {
+        KeyedFilter[] original = new KeyedFilter[]{new KeyedFilter("bbb", new MatchNoneQueryBuilder()),
+                new KeyedFilter("aaa", new MatchNoneQueryBuilder())};
+        FiltersAggregatorBuilder builder;
+        builder = new FiltersAggregatorBuilder("my-agg", original);
+        assertEquals("aaa", builder.filters().get(0).key());
+        assertEquals("bbb", builder.filters().get(1).key());
+        // original should be unchanged
+        assertEquals("bbb", original[0].key());
+        assertEquals("aaa", original[1].key());
+    }
+
 }