Replace AggregatorParsers with namedObject

core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java

@@ -90,8 +90,7 @@ public static MultiSearchRequest parseRequest(RestRequest restRequest, boolean a
         parseMultiLineRequest(restRequest, multiRequest.indicesOptions(), allowExplicitIndex, (searchRequest, parser) -> {
             try {
                 final QueryParseContext queryParseContext = new QueryParseContext(parser, parseFieldMatcher);
-                searchRequest.source(SearchSourceBuilder.fromXContent(queryParseContext,
-                    searchRequestParsers.aggParsers, searchRequestParsers.suggesters));
+                searchRequest.source(SearchSourceBuilder.fromXContent(queryParseContext, searchRequestParsers.suggesters));
                 multiRequest.add(searchRequest);
             } catch (IOException e) {
                 throw new ElasticsearchParseException("Exception when parsing search request", e);

core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

@@ -93,7 +93,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r
         searchRequest.indices(Strings.splitStringByCommaToArray(request.param("index")));
         if (requestContentParser != null) {
             QueryParseContext context = new QueryParseContext(requestContentParser, parseFieldMatcher);
-            searchRequest.source().parseXContent(context, searchRequestParsers.aggParsers, searchRequestParsers.suggesters);
+            searchRequest.source().parseXContent(context, searchRequestParsers.suggesters);
         }
 
         // do not allow 'query_and_fetch' or 'dfs_query_and_fetch' search types

core/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -94,8 +94,8 @@
 import org.elasticsearch.plugins.SearchPlugin.SearchExtSpec;
 import org.elasticsearch.plugins.SearchPlugin.SearchExtensionSpec;
 import org.elasticsearch.search.aggregations.AggregationBuilder;
-import org.elasticsearch.search.aggregations.Aggregator;
-import org.elasticsearch.search.aggregations.AggregatorParsers;
+import org.elasticsearch.search.aggregations.AggregatorFactories;
+import org.elasticsearch.search.aggregations.BaseAggregationBuilder;
 import org.elasticsearch.search.aggregations.InternalAggregation;
 import org.elasticsearch.search.aggregations.PipelineAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.children.ChildrenAggregationBuilder;
@@ -268,10 +268,6 @@ public class SearchModule {
     private final boolean transportClient;
     private final Map<String, Highlighter> highlighters;
     private final Map<String, Suggester<?>> suggesters;
-    private final ParseFieldRegistry<Aggregator.Parser> aggregationParserRegistry = new ParseFieldRegistry<>("aggregation");
-    private final ParseFieldRegistry<PipelineAggregator.Parser> pipelineAggregationParserRegistry = new ParseFieldRegistry<>(
-            "pipline_aggregation");
-    private final AggregatorParsers aggregatorParsers = new AggregatorParsers(aggregationParserRegistry, pipelineAggregationParserRegistry);
     private final ParseFieldRegistry<SignificanceHeuristicParser> significanceHeuristicParserRegistry = new ParseFieldRegistry<>(
             "significance_heuristic");
     private final ParseFieldRegistry<MovAvgModel.AbstractModelParser> movingAverageModelParserRegistry = new ParseFieldRegistry<>(
@@ -301,7 +297,7 @@ public SearchModule(Settings settings, boolean transportClient, List<SearchPlugi
         registerFetchSubPhases(plugins);
         registerSearchExts(plugins);
         registerShapes();
-        searchRequestParsers = new SearchRequestParsers(aggregatorParsers, getSuggesters());
+        searchRequestParsers = new SearchRequestParsers(getSuggesters());
     }
 
     public List<NamedWriteableRegistry.Entry> getNamedWriteables() {
@@ -341,13 +337,6 @@ public ParseFieldRegistry<MovAvgModel.AbstractModelParser> getMovingAverageModel
         return movingAverageModelParserRegistry;
     }
 
-    /**
-     * Parsers for {@link AggregationBuilder}s and {@link PipelineAggregationBuilder}s.
-     */
-    public AggregatorParsers getAggregatorParsers() {
-        return aggregatorParsers;
-    }
-
     private void registerAggregations(List<SearchPlugin> plugins) {
         registerAggregation(new AggregationSpec(AvgAggregationBuilder.NAME, AvgAggregationBuilder::new, AvgAggregationBuilder::parse)
                 .addResultReader(InternalAvg::new));
@@ -433,7 +422,10 @@ private void registerAggregations(List<SearchPlugin> plugins) {
 
     private void registerAggregation(AggregationSpec spec) {
         if (false == transportClient) {
-            aggregationParserRegistry.register(spec.getParser(), spec.getName());
+            namedXContents.add(new NamedXContentRegistry.Entry(BaseAggregationBuilder.class, spec.getName(), (p, c) -> {
+                AggregatorFactories.AggParseContext context = (AggregatorFactories.AggParseContext) c;
+                return spec.getParser().parse(context.name, context.queryParseContext);
+            }));
         }
         namedWriteables.add(
                 new NamedWriteableRegistry.Entry(AggregationBuilder.class, spec.getName().getPreferredName(), spec.getReader()));
@@ -527,7 +519,10 @@ private void registerPipelineAggregations(List<SearchPlugin> plugins) {
 
     private void registerPipelineAggregation(PipelineAggregationSpec spec) {
         if (false == transportClient) {
-            pipelineAggregationParserRegistry.register(spec.getParser(), spec.getName());
+            namedXContents.add(new NamedXContentRegistry.Entry(BaseAggregationBuilder.class, spec.getName(), (p, c) -> {
+                AggregatorFactories.AggParseContext context = (AggregatorFactories.AggParseContext) c;
+                return spec.getParser().parse(context.name, context.queryParseContext);
+            }));
         }
         namedWriteables.add(
                 new NamedWriteableRegistry.Entry(PipelineAggregationBuilder.class, spec.getName().getPreferredName(), spec.getReader()));

core/src/main/java/org/elasticsearch/search/SearchRequestParsers.java

@@ -19,8 +19,6 @@
 
 package org.elasticsearch.search;
 
-import org.elasticsearch.index.query.QueryParseContext;
-import org.elasticsearch.search.aggregations.AggregatorParsers;
 import org.elasticsearch.search.suggest.Suggesters;
 
 /**
@@ -32,25 +30,14 @@ public class SearchRequestParsers {
     // methods split across RestSearchAction and SearchSourceBuilder should be moved here
     // TODO: make all members private once parsing functions are moved here
 
-    // TODO: AggregatorParsers should be removed and the underlying maps of agg
-    // and pipeline agg parsers should be here
-    /**
-     * Agg and pipeline agg parsers that may be used in search requests.
-     * @see org.elasticsearch.search.builder.SearchSourceBuilder#fromXContent(QueryParseContext, AggregatorParsers,
-     *      Suggesters)
-     */
-    public final AggregatorParsers aggParsers;
-
     // TODO: Suggesters should be removed and the underlying map moved here
     /**
      * Suggesters that may be used in search requests.
-     * @see org.elasticsearch.search.builder.SearchSourceBuilder#fromXContent(QueryParseContext, AggregatorParsers,
-     *      Suggesters)
+     * @see org.elasticsearch.search.builder.SearchSourceBuilder#fromXContent(QueryParseContext, Suggesters)
      */
     public final Suggesters suggesters;
 
-    public SearchRequestParsers(AggregatorParsers aggParsers, Suggesters suggesters) {
-        this.aggParsers = aggParsers;
+    public SearchRequestParsers(Suggesters suggesters) {
         this.suggesters = suggesters;
     }
 }

core/src/main/java/org/elasticsearch/search/aggregations/AbstractAggregationBuilder.java

@@ -117,6 +117,7 @@ public AB setMetaData(Map<String, Object> metaData) {
         return (AB) this;
     }
 
+    @Override
     public String getType() {
         return type.name();
     }

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

@@ -23,6 +23,7 @@
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.io.stream.NamedWriteable;
 import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.index.query.QueryParseContext;
 import org.elasticsearch.search.aggregations.InternalAggregation.Type;
 import org.elasticsearch.search.internal.SearchContext;
 
@@ -34,7 +35,7 @@
  */
 public abstract class AggregationBuilder
     extends ToXContentToBytes
-    implements NamedWriteable, ToXContent {
+    implements NamedWriteable, ToXContent, BaseAggregationBuilder {
 
     protected final String name;
     protected final Type type;
@@ -66,6 +67,7 @@ public String getName() {
     protected abstract AggregatorFactory<?> build(SearchContext context, AggregatorFactory<?> parent) throws IOException;
 
     /** Associate metadata with this {@link AggregationBuilder}. */
+    @Override
     public abstract AggregationBuilder setMetaData(Map<String, Object> metaData);
 
     /** Add a sub aggregation to this builder. */
@@ -77,13 +79,14 @@ public String getName() {
     /**
      * Internal: Registers sub-factories with this factory. The sub-factory will be
      * responsible for the creation of sub-aggregators under the aggregator
-     * created by this factory. This is only for use by {@link AggregatorParsers}.
+     * created by this factory. This is only for use by {@link AggregatorFactories#parseAggregators(QueryParseContext)}.
      *
      * @param subFactories
      *            The sub-factories
      * @return this factory (fluent interface)
      */
-    protected abstract AggregationBuilder subAggregations(AggregatorFactories.Builder subFactories);
+    @Override
+    public abstract AggregationBuilder subAggregations(AggregatorFactories.Builder subFactories);
 
     /** Common xcontent fields shared among aggregator builders */
     public static final class CommonFields extends ParseField.CommonFields {

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

@@ -19,10 +19,13 @@
 package org.elasticsearch.search.aggregations;
 
 import org.elasticsearch.action.support.ToXContentToBytes;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.index.query.QueryParseContext;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 import org.elasticsearch.search.aggregations.support.AggregationPath;
 import org.elasticsearch.search.aggregations.support.AggregationPath.PathElement;
@@ -40,8 +43,126 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 public class AggregatorFactories {
+    public static final Pattern VALID_AGG_NAME = Pattern.compile("[^\\[\\]>]+");
+
+    /**
+     * Parses the aggregation request recursively generating aggregator factories in turn.
+     *
+     * @param parseContext   The parse context.
+     *
+     * @return          The parsed aggregator factories.
+     *
+     * @throws IOException When parsing fails for unknown reasons.
+     */
+    public static AggregatorFactories.Builder parseAggregators(QueryParseContext parseContext) throws IOException {
+        return parseAggregators(parseContext, 0);
+    }
+
+    private static AggregatorFactories.Builder parseAggregators(QueryParseContext parseContext, int level) throws IOException {
+        Matcher validAggMatcher = VALID_AGG_NAME.matcher("");
+        AggregatorFactories.Builder factories = new AggregatorFactories.Builder();
+
+        XContentParser.Token token = null;
+        XContentParser parser = parseContext.parser();
+        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
+            if (token != XContentParser.Token.FIELD_NAME) {
+                throw new ParsingException(parser.getTokenLocation(),
+                        "Unexpected token " + token + " in [aggs]: aggregations definitions must start with the name of the aggregation.");
+            }
+            final String aggregationName = parser.currentName();
+            if (!validAggMatcher.reset(aggregationName).matches()) {
+                throw new ParsingException(parser.getTokenLocation(), "Invalid aggregation name [" + aggregationName
+                        + "]. Aggregation names must be alpha-numeric and can only contain '_' and '-'");
+            }
+
+            token = parser.nextToken();
+            if (token != XContentParser.Token.START_OBJECT) {
+                throw new ParsingException(parser.getTokenLocation(), "Aggregation definition for [" + aggregationName + " starts with a ["
+                        + token + "], expected a [" + XContentParser.Token.START_OBJECT + "].");
+            }
+
+            BaseAggregationBuilder aggBuilder = null;
+            AggregatorFactories.Builder subFactories = null;
+
+            Map<String, Object> metaData = null;
+
+            while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
+                if (token != XContentParser.Token.FIELD_NAME) {
+                    throw new ParsingException(
+                            parser.getTokenLocation(), "Expected [" + XContentParser.Token.FIELD_NAME + "] under a ["
+                            + XContentParser.Token.START_OBJECT + "], but got a [" + token + "] in [" + aggregationName + "]",
+                            parser.getTokenLocation());
+                }
+                final String fieldName = parser.currentName();
+
+                token = parser.nextToken();
+                if (token == XContentParser.Token.START_OBJECT) {
+                    switch (fieldName) {
+                    case "meta":
+                        metaData = parser.map();
+                        break;
+                    case "aggregations":
+                    case "aggs":
+                        if (subFactories != null) {
+                            throw new ParsingException(parser.getTokenLocation(),
+                                    "Found two sub aggregation definitions under [" + aggregationName + "]");
+                        }
+                        subFactories = parseAggregators(parseContext, level + 1);
+                        break;
+                    default:
+                        if (aggBuilder != null) {
+                            throw new ParsingException(parser.getTokenLocation(), "Found two aggregation type definitions in ["
+                                    + aggregationName + "]: [" + aggBuilder.getType() + "] and [" + fieldName + "]");
+                        }
+
+                        aggBuilder = parser.namedObject(BaseAggregationBuilder.class, fieldName,
+                                new AggParseContext(aggregationName, parseContext));
+                    }
+                } else {
+                    throw new ParsingException(parser.getTokenLocation(), "Expected [" + XContentParser.Token.START_OBJECT + "] under ["
+                            + fieldName + "], but got a [" + token + "] in [" + aggregationName + "]");
+                }
+            }
+
+            if (aggBuilder == null) {
+                throw new ParsingException(parser.getTokenLocation(), "Missing definition for aggregation [" + aggregationName + "]",
+                        parser.getTokenLocation());
+            } else {
+                if (metaData != null) {
+                    aggBuilder.setMetaData(metaData);
+                }
+
+                if (subFactories != null) {
+                    aggBuilder.subAggregations(subFactories);
+                }
+
+                if (aggBuilder instanceof AggregationBuilder) {
+                    factories.addAggregator((AggregationBuilder) aggBuilder);
+                } else {
+                    factories.addPipelineAggregator((PipelineAggregationBuilder) aggBuilder);
+                }
+            }
+        }
+
+        return factories;
+    }
+
+    /**
+     * Context to parse and aggregation. This should eventually be removed and replaced with a String.
+     */
+    public static final class AggParseContext {
+        public final String name;
+        public final QueryParseContext queryParseContext;
+
+        public AggParseContext(String name, QueryParseContext queryParseContext) {
+            this.name = name;
+            this.queryParseContext = queryParseContext;
+        }
+    }
 
     public static final AggregatorFactories EMPTY = new AggregatorFactories(null, new AggregatorFactory<?>[0],
             new ArrayList<PipelineAggregationBuilder>());

core/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationBuilder.java

@@ -0,0 +1,46 @@
+/*
+ * 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.common.xcontent.XContentParser;
+import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
+
+import java.util.Map;
+
+/**
+ * Interface shared by {@link AggregationBuilder} and {@link PipelineAggregationBuilder} so they can conveniently share the same namespace
+ * for {@link XContentParser#namedObject(Class, String, Object)}.
+ */
+public interface BaseAggregationBuilder {
+    /**
+     * The name of the type of aggregation built by this builder. 
+     */
+    String getType();
+
+    /**
+     * Set the aggregation's metadata. Returns {@code this} for chaining.
+     */
+    BaseAggregationBuilder setMetaData(Map<String, Object> metaData);
+
+    /**
+     * Set the sub aggregations if this aggregation supports sub aggregations. Returns {@code this} for chaining.
+     */
+    BaseAggregationBuilder subAggregations(Builder subFactories);
+}

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

@@ -20,7 +20,7 @@
 
 import org.elasticsearch.action.support.ToXContentToBytes;
 import org.elasticsearch.common.io.stream.NamedWriteable;
-import org.elasticsearch.search.aggregations.AggregatorFactory;
+import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 
 import java.io.IOException;
@@ -32,7 +32,7 @@
  * specific type.
  */
 public abstract class PipelineAggregationBuilder extends ToXContentToBytes
-        implements NamedWriteable {
+        implements NamedWriteable, BaseAggregationBuilder {
 
     protected final String name;
     protected final String[] bucketsPaths;
@@ -79,6 +79,11 @@ protected abstract void validate(AggregatorFactory<?> parent, AggregatorFactory<
     protected abstract PipelineAggregator create() throws IOException;
 
     /** Associate metadata with this {@link PipelineAggregationBuilder}. */
+    @Override
     public abstract PipelineAggregationBuilder setMetaData(Map<String, Object> metaData);
 
+    @Override
+    public PipelineAggregationBuilder subAggregations(Builder subFactories) {
+        throw new IllegalArgumentException("Aggregation [" + name + "] cannot define sub-aggregations");
+    }
 }