Fix AggregationFactories.Builder equality and hash regarding order (#34005)

Fixes the equals and hash function to ignore the order of aggregations to ensure equality after serialization
and deserialization. This ensures storing configs with aggregation works properly.

This also addresses a potential issue in caching when the same query contains aggregations but in 
different order. 1st it will not hit in the cache, 2nd cache objects which shall be equal might end up twice in 
the cache.

Author: Hendrik Muhs

server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java

@@ -28,7 +28,7 @@
 import org.elasticsearch.search.internal.SearchContext;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 
 /**
@@ -79,12 +79,12 @@ public String getName() {
     public abstract AggregationBuilder subAggregation(PipelineAggregationBuilder aggregation);
 
     /** Return the configured set of subaggregations **/
-    public List<AggregationBuilder> getSubAggregations() {
+    public Collection<AggregationBuilder> getSubAggregations() {
         return factoriesBuilder.getAggregatorFactories();
     }
 
     /** Return the configured set of pipeline aggregations **/
-    public List<PipelineAggregationBuilder> getPipelineAggregations() {
+    public Collection<PipelineAggregationBuilder> getPipelineAggregations() {
         return factoriesBuilder.getPipelineAggregatorFactories();
     }
 

server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java

@@ -38,9 +38,11 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -237,8 +239,11 @@ public int countPipelineAggregators() {
 
     public static class Builder implements Writeable, ToXContentObject {
         private final Set<String> names = new HashSet<>();
-        private final List<AggregationBuilder> aggregationBuilders = new ArrayList<>();
-        private final List<PipelineAggregationBuilder> pipelineAggregatorBuilders = new ArrayList<>();
+
+        // Using LinkedHashSets to preserve the order of insertion, that makes the results
+        // ordered nicely, although technically order does not matter
+        private final Collection<AggregationBuilder> aggregationBuilders = new LinkedHashSet<>();
+        private final Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders = new LinkedHashSet<>();
         private boolean skipResolveOrder;
 
         /**
@@ -322,29 +327,32 @@ public AggregatorFactories build(SearchContext context, AggregatorFactory<?> par
                         parent);
             }
             AggregatorFactory<?>[] aggFactories = new AggregatorFactory<?>[aggregationBuilders.size()];
-            for (int i = 0; i < aggregationBuilders.size(); i++) {
-                aggFactories[i] = aggregationBuilders.get(i).build(context, parent);
+
+            int i = 0;
+            for (AggregationBuilder agg : aggregationBuilders) {
+                aggFactories[i] = agg.build(context, parent);
+                ++i;
             }
             return new AggregatorFactories(aggFactories, orderedpipelineAggregators);
         }
 
         private List<PipelineAggregationBuilder> resolvePipelineAggregatorOrder(
-                List<PipelineAggregationBuilder> pipelineAggregatorBuilders, List<AggregationBuilder> aggBuilders,
+                Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders, Collection<AggregationBuilder> aggregationBuilders,
                 AggregatorFactory<?> parent) {
             Map<String, PipelineAggregationBuilder> pipelineAggregatorBuildersMap = new HashMap<>();
             for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders) {
                 pipelineAggregatorBuildersMap.put(builder.getName(), builder);
             }
             Map<String, AggregationBuilder> aggBuildersMap = new HashMap<>();
-            for (AggregationBuilder aggBuilder : aggBuilders) {
+            for (AggregationBuilder aggBuilder : aggregationBuilders) {
                 aggBuildersMap.put(aggBuilder.name, aggBuilder);
             }
             List<PipelineAggregationBuilder> orderedPipelineAggregatorrs = new LinkedList<>();
             List<PipelineAggregationBuilder> unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders);
-            Set<PipelineAggregationBuilder> temporarilyMarked = new HashSet<>();
+            Collection<PipelineAggregationBuilder> temporarilyMarked = new HashSet<>();
             while (!unmarkedBuilders.isEmpty()) {
                 PipelineAggregationBuilder builder = unmarkedBuilders.get(0);
-                builder.validate(parent, aggBuilders, pipelineAggregatorBuilders);
+                builder.validate(parent, aggregationBuilders, pipelineAggregatorBuilders);
                 resolvePipelineAggregatorOrder(aggBuildersMap, pipelineAggregatorBuildersMap, orderedPipelineAggregatorrs, unmarkedBuilders,
                         temporarilyMarked, builder);
             }
@@ -354,7 +362,7 @@ private List<PipelineAggregationBuilder> resolvePipelineAggregatorOrder(
         private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggBuildersMap,
                 Map<String, PipelineAggregationBuilder> pipelineAggregatorBuildersMap,
                 List<PipelineAggregationBuilder> orderedPipelineAggregators, List<PipelineAggregationBuilder> unmarkedBuilders,
-                Set<PipelineAggregationBuilder> temporarilyMarked, PipelineAggregationBuilder builder) {
+                Collection<PipelineAggregationBuilder> temporarilyMarked, PipelineAggregationBuilder builder) {
             if (temporarilyMarked.contains(builder)) {
                 throw new IllegalArgumentException("Cyclical dependency found with pipeline aggregator [" + builder.getName() + "]");
             } else if (unmarkedBuilders.contains(builder)) {
@@ -375,7 +383,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
                             } else {
                                 // Check the non-pipeline sub-aggregator
                                 // factories
-                                List<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
+                                Collection<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
                                 boolean foundSubBuilder = false;
                                 for (AggregationBuilder subBuilder : subBuilders) {
                                     if (aggName.equals(subBuilder.name)) {
@@ -386,7 +394,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
                                 }
                                 // Check the pipeline sub-aggregator factories
                                 if (!foundSubBuilder && (i == bucketsPathElements.size() - 1)) {
-                                    List<PipelineAggregationBuilder> subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders;
+                                    Collection<PipelineAggregationBuilder> subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders;
                                     for (PipelineAggregationBuilder subFactory : subPipelineBuilders) {
                                         if (aggName.equals(subFactory.getName())) {
                                             foundSubBuilder = true;
@@ -417,12 +425,12 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
             }
         }
 
-        public List<AggregationBuilder> getAggregatorFactories() {
-            return Collections.unmodifiableList(aggregationBuilders);
+        public Collection<AggregationBuilder> getAggregatorFactories() {
+            return Collections.unmodifiableCollection(aggregationBuilders);
         }
 
-        public List<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
-            return Collections.unmodifiableList(pipelineAggregatorBuilders);
+        public Collection<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
+            return Collections.unmodifiableCollection(pipelineAggregatorBuilders);
         }
 
         public int count() {
@@ -463,6 +471,7 @@ public boolean equals(Object obj) {
             if (getClass() != obj.getClass())
                 return false;
             Builder other = (Builder) obj;
+
             if (!Objects.equals(aggregationBuilders, other.aggregationBuilders))
                 return false;
             if (!Objects.equals(pipelineAggregatorBuilders, other.pipelineAggregatorBuilders))

server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java

@@ -25,7 +25,7 @@
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 
 /**
@@ -68,8 +68,8 @@ public final String[] getBucketsPaths() {
      * Internal: Validates the state of this factory (makes sure the factory is properly
      * configured)
      */
-    protected abstract void validate(AggregatorFactory<?> parent, List<AggregationBuilder> factories,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories);
+    protected abstract void validate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggregationBuilders,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders);
 
     /**
      * Creates the pipeline aggregator

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java

@@ -28,7 +28,7 @@
 
 import java.io.IOException;
 import java.util.Arrays;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 
@@ -81,8 +81,8 @@ public String type() {
      * configured)
      */
     @Override
-    public final void validate(AggregatorFactory<?> parent, List<AggregationBuilder> factories,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
+    public final void validate(AggregatorFactory<?> parent, Collection<AggregationBuilder> factories,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
         doValidate(parent, factories, pipelineAggregatorFactories);
     }
 
@@ -99,8 +99,8 @@ public final PipelineAggregator create() throws IOException {
         return aggregator;
     }
 
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> factories,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> factories,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
     }
 
     @SuppressWarnings("unchecked")

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregationBuilder.java

@@ -32,7 +32,7 @@
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -109,8 +109,8 @@ public GapPolicy gapPolicy() {
     protected abstract PipelineAggregator createInternal(Map<String, Object> metaData) throws IOException;
 
     @Override
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggBuilders,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggBuilders,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
         if (bucketsPaths.length != 1) {
             throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName()
                     + " must contain a single entry for aggregation [" + name + "]");

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregationBuilder.java

@@ -35,7 +35,7 @@
 
 import java.io.IOException;
 import java.util.Arrays;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 
@@ -95,8 +95,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
     }
 
     @Override
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
         super.doValidate(parent, aggFactories, pipelineAggregatorFactories);
 
         for (Double p : percents) {

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketPipelineAggregationBuilder.java

@@ -29,7 +29,7 @@
 import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.BucketMetricsPipelineAggregationBuilder;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 
@@ -82,8 +82,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
     }
 
     @Override
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggBuilders,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggBuilders,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
         super.doValidate(parent, aggBuilders, pipelineAggregatorFactories);
 
         if (sigma < 0.0 ) {

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketsort/BucketSortPipelineAggregationBuilder.java

@@ -38,6 +38,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
@@ -145,8 +146,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
     }
 
     @Override
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
-                           List<PipelineAggregationBuilder> pipelineAggregatoractories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
+                           Collection<PipelineAggregationBuilder> pipelineAggregatoractories) {
         if (sorts.isEmpty() && size == null && from == 0) {
             throw new IllegalStateException("[" + name + "] is configured to perform nothing. Please set either of "
                     + Arrays.asList(SearchSourceBuilder.SORT_FIELD.getPreferredName(), SIZE.getPreferredName(), FROM.getPreferredName())

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregationBuilder.java

@@ -36,6 +36,7 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -97,8 +98,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
     }
 
     @Override
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
-            List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
+            Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
         if (bucketsPaths.length != 1) {
             throw new IllegalStateException(BUCKETS_PATH.getPreferredName()
                     + " must contain a single entry for aggregation [" + name + "]");

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregationBuilder.java

@@ -42,6 +42,7 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -156,8 +157,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
     }
 
     @Override
-    public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
-            List<PipelineAggregationBuilder> pipelineAggregatoractories) {
+    public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
+            Collection<PipelineAggregationBuilder> pipelineAggregatoractories) {
         if (bucketsPaths.length != 1) {
             throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName()
                     + " must contain a single entry for aggregation [" + name + "]");