From b58bbb9e48a4a86cd5cd2fb6aaab8ddb4e58901e Mon Sep 17 00:00:00 2001
From: Dimitris Athanasiou <dimitris@elastic.co>
Date: Mon, 19 Dec 2016 09:52:07 +0000
Subject: [PATCH] Allow setting aggs after parsing them elsewhere (#22238)

This commit exposes public getters for the aggregations in
AggregatorFactories.Builder. The reason is that it allows to
parse the aggregation object from elsewhere (e.g. a plugin) and then
be able to get the aggregation builders in order to set them in
a SearchSourceBuilder.

The alternative would have been to expose a setter for the
AggregatorFactories.Builder object. But that would be making
the API a bit trappy.
---
 .../aggregations/AggregatorFactories.java     | 11 ++---
 .../AggregatorFactoriesTests.java             | 45 +++++++++++++++++++
 2 files changed, 51 insertions(+), 5 deletions(-)
 create mode 100644 core/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java

diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java b/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java
index 200232930a9a3..b0f52ffb13039 100644
--- a/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java
+++ b/core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java
@@ -32,6 +32,7 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -255,7 +256,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
                             } else {
                                 // Check the non-pipeline sub-aggregator
                                 // factories
-                                AggregationBuilder[] subBuilders = aggBuilder.factoriesBuilder.getAggregatorFactories();
+                                List<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
                                 boolean foundSubBuilder = false;
                                 for (AggregationBuilder subBuilder : subBuilders) {
                                     if (aggName.equals(subBuilder.name)) {
@@ -297,12 +298,12 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
             }
         }
 
-        AggregationBuilder[] getAggregatorFactories() {
-            return this.aggregationBuilders.toArray(new AggregationBuilder[this.aggregationBuilders.size()]);
+        public List<AggregationBuilder> getAggregatorFactories() {
+            return Collections.unmodifiableList(aggregationBuilders);
         }
 
-        List<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
-            return this.pipelineAggregatorBuilders;
+        public List<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
+            return Collections.unmodifiableList(pipelineAggregatorBuilders);
         }
 
         public int count() {
diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java
new file mode 100644
index 0000000000000..1822b1e22e9fb
--- /dev/null
+++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java
@@ -0,0 +1,45 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.search.aggregations;
+
+import org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilders;
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.List;
+
+import static org.hamcrest.Matchers.equalTo;
+
+public class AggregatorFactoriesTests extends ESTestCase {
+
+    public void testGetAggregatorFactories_returnsUnmodifiableList() {
+        AggregatorFactories.Builder builder = new AggregatorFactories.Builder().addAggregator(AggregationBuilders.avg("foo"));
+        List<AggregationBuilder> aggregatorFactories = builder.getAggregatorFactories();
+        assertThat(aggregatorFactories.size(), equalTo(1));
+        expectThrows(UnsupportedOperationException.class, () -> aggregatorFactories.add(AggregationBuilders.avg("bar")));
+    }
+
+    public void testGetPipelineAggregatorFactories_returnsUnmodifiableList() {
+        AggregatorFactories.Builder builder = new AggregatorFactories.Builder().addPipelineAggregator(
+            PipelineAggregatorBuilders.avgBucket("foo", "path1"));
+        List<PipelineAggregationBuilder> pipelineAggregatorFactories = builder.getPipelineAggregatorFactories();
+        assertThat(pipelineAggregatorFactories.size(), equalTo(1));
+        expectThrows(UnsupportedOperationException.class,
+            () -> pipelineAggregatorFactories.add(PipelineAggregatorBuilders.avgBucket("bar", "path2")));
+    }
+}