decouple registration of SearchExtParsers from sub fetch phases

Search section supports an ext section that is used to provide additional config needed from plugins. It is now tied to sub fetch phases because it is the only section that may need additional config, but there is no reason for the two to be tightly coupled.

It is now possible to register a searchExtParser independently from a sub fetch phase. All a search ext parser does is parsing some ext section of a search request, whose parsed resulting object is stored in the search context for later retrieval.

core/src/main/java/org/elasticsearch/plugins/SearchPlugin.java

@@ -30,6 +30,7 @@ import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryParser;
 import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilder;
 import org.elasticsearch.index.query.functionscore.ScoreFunctionParser;
+import org.elasticsearch.search.SearchExtParser;
 import org.elasticsearch.search.aggregations.Aggregation;
 import org.elasticsearch.search.aggregations.AggregationBuilder;
 import org.elasticsearch.search.aggregations.Aggregator;
@@ -82,6 +83,12 @@ public interface SearchPlugin {
     default List<FetchSubPhase> getFetchSubPhases(FetchPhaseConstructionContext context) {
         return emptyList();
     }
+    /**
+     * The new {@link SearchExtParser}s defined by this plugin.
+     */
+    default List<SearchExtParser> getSearchExtParsers() {
+        return emptyList();
+    }
     /**
      * Get the {@link Highlighter}s defined by this plugin.
      */

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

@@ -0,0 +1,32 @@
+/*
+ * 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;
+
+import org.elasticsearch.common.xcontent.ParseFieldRegistry;
+
+/**
+ * Extensions to ParseFieldRegistry to make Guice happy.
+ */
+public class SearchExtParserRegistry extends ParseFieldRegistry<SearchExtParser> {
+
+    SearchExtParserRegistry() {
+        super("ext");
+    }
+}

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

@@ -306,6 +306,7 @@ public class SearchModule extends AbstractModule {
             "moving_avg_model");
 
     private final List<FetchSubPhase> fetchSubPhases = new ArrayList<>();
+    private final SearchExtParserRegistry searchExtParserRegistry = new SearchExtParserRegistry();
 
     private final Settings settings;
     private final List<Entry> namedWriteables = new ArrayList<>();
@@ -326,6 +327,7 @@ public class SearchModule extends AbstractModule {
         registerAggregations(plugins);
         registerPipelineAggregations(plugins);
         registerFetchSubPhases(plugins);
+        registerSearchExtParsers(plugins);
         registerShapes();
         searchRequestParsers = new SearchRequestParsers(queryParserRegistry, aggregatorParsers, getSuggesters());
     }
@@ -380,6 +382,7 @@ public class SearchModule extends AbstractModule {
         if (false == transportClient) {
             bind(IndicesQueriesRegistry.class).toInstance(queryParserRegistry);
             bind(SearchRequestParsers.class).toInstance(searchRequestParsers);
+            bind(SearchExtParserRegistry.class).toInstance(searchExtParserRegistry);
             configureSearch();
         }
     }
@@ -725,6 +728,14 @@ public class SearchModule extends AbstractModule {
         registerFromPlugin(plugins, p -> p.getFetchSubPhases(context), this::registerFetchSubPhase);
     }
 
+    private void registerSearchExtParsers(List<SearchPlugin> plugins) {
+        registerFromPlugin(plugins, SearchPlugin::getSearchExtParsers, this::registerSearchExtParser);
+    }
+
+    private void registerSearchExtParser(SearchExtParser searchExtParser) {
+        searchExtParserRegistry.register(searchExtParser, searchExtParser.getName());
+    }
+
     private void registerFetchSubPhase(FetchSubPhase subPhase) {
         Class<?> subPhaseClass = subPhase.getClass();
         if (fetchSubPhases.stream().anyMatch(p -> p.getClass().equals(subPhaseClass))) {

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

@@ -100,7 +100,6 @@ import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicLong;
 
-import static java.util.Collections.unmodifiableMap;
 import static org.elasticsearch.common.unit.TimeValue.timeValueMillis;
 import static org.elasticsearch.common.unit.TimeValue.timeValueMinutes;
 
@@ -143,13 +142,14 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
 
     private final ConcurrentMapLong<SearchContext> activeContexts = ConcurrentCollections.newConcurrentMapLongWithAggressiveConcurrency();
 
-    private final Map<String, SearchExtParser> fetchPhaseParsers;
+    private final SearchExtParserRegistry searchExtParserRegistry;
 
     private final ParseFieldMatcher parseFieldMatcher;
 
     @Inject
     public SearchService(Settings settings, ClusterSettings clusterSettings, ClusterService clusterService, IndicesService indicesService,
-                         ThreadPool threadPool, ScriptService scriptService, BigArrays bigArrays, FetchPhase fetchPhase) {
+                         ThreadPool threadPool, ScriptService scriptService, BigArrays bigArrays, FetchPhase fetchPhase,
+                         SearchExtParserRegistry searchExtParserRegistry) {
         super(settings);
         this.parseFieldMatcher = new ParseFieldMatcher(settings);
         this.threadPool = threadPool;
@@ -163,10 +163,10 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
         TimeValue keepAliveInterval = KEEPALIVE_INTERVAL_SETTING.get(settings);
         this.defaultKeepAlive = DEFAULT_KEEPALIVE_SETTING.get(settings).millis();
 
-        this.fetchPhaseParsers = unmodifiableMap(fetchPhase.parsers());
-
         this.keepAliveReaper = threadPool.scheduleWithFixedDelay(new Reaper(), keepAliveInterval, Names.SAME);
 
+        this.searchExtParserRegistry = searchExtParserRegistry;
+
         defaultSearchTimeout = DEFAULT_SEARCH_TIMEOUT_SETTING.get(settings);
         clusterSettings.addSettingsUpdateConsumer(DEFAULT_SEARCH_TIMEOUT_SETTING, this::setDefaultSearchTimeout);
     }
@@ -762,19 +762,10 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
                     if (token == XContentParser.Token.FIELD_NAME) {
                         currentFieldName = extParser.currentName();
                     } else {
-                        SearchExtParser searchExtParser = this.fetchPhaseParsers.get(currentFieldName);
+                        SearchExtParser searchExtParser = this.searchExtParserRegistry.lookup(currentFieldName, parseFieldMatcher,
-                        if (searchExtParser == null) {
-                            if (currentFieldName != null && currentFieldName.equals("suggest")) {
-                                throw new SearchParseException(context,
-                                    "suggest is not supported in [ext], please use SearchSourceBuilder#suggest(SuggestBuilder) instead",
                                 extParser.getTokenLocation());
-                            }
+                        Object searchExtBuilder = searchExtParser.parse(extParser);
-                            throw new SearchParseException(context, "Unknown element [" + currentFieldName + "] in [ext]",
+                        context.putSearchExtBuilder(currentFieldName, searchExtBuilder);
-                                    extParser.getTokenLocation());
-                        } else {
-                            Object fetchSubPhaseBuilder = searchExtParser.parse(extParser);
-                            context.putFetchSubPhaseBuilder(currentFieldName, fetchSubPhaseBuilder);
-                        }
                     }
                 }
             } catch (Exception e) {

core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

@@ -41,7 +41,6 @@ import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.ObjectMapper;
 import org.elasticsearch.index.mapper.SourceFieldMapper;
-import org.elasticsearch.search.SearchExtParser;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.SearchHitField;
 import org.elasticsearch.search.SearchPhase;
@@ -62,7 +61,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
-import static java.util.Collections.unmodifiableMap;
 import static org.elasticsearch.common.xcontent.XContentFactory.contentBuilder;
 
 /**
@@ -78,16 +76,6 @@ public class FetchPhase implements SearchPhase {
         this.fetchSubPhases[fetchSubPhases.size()] = new InnerHitsFetchSubPhase(this);
     }
 
-    public Map<String, ? extends SearchExtParser> parsers() {
-        Map<String, SearchExtParser> parsers = new HashMap<>();
-        for (FetchSubPhase fetchSubPhase : fetchSubPhases) {
-            if (fetchSubPhase.parser() != null) {
-                parsers.put(fetchSubPhase.parser().getName(), fetchSubPhase.parser());
-            }
-        }
-        return unmodifiableMap(parsers);
-    }
-
     @Override
     public void preProcess(SearchContext context) {
     }

core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java

@@ -22,7 +22,6 @@ import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.LeafReader;
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.search.IndexSearcher;
-import org.elasticsearch.search.SearchExtParser;
 import org.elasticsearch.search.internal.InternalSearchHit;
 import org.elasticsearch.search.internal.SearchContext;
 
@@ -77,13 +76,6 @@ public interface FetchSubPhase {
 
     }
 
-    /**
-     * Returns the parser for the optional config to be put in the ext section of the search request
-     */
-    default SearchExtParser parser() {
-        return null;
-    }
-
     /**
      * Executes the hit level phase, with a reader and doc id (note, its a low level reader, and the matching doc).
      */

core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java

@@ -145,7 +145,7 @@ public class DefaultSearchContext extends SearchContext {
     private volatile long lastAccessTime = -1;
     private Profilers profilers;
 
-    private final Map<String, Object> subPhaseBuilders = new HashMap<>();
+    private final Map<String, Object> searchExtBuilders = new HashMap<>();
     private final Map<Class<?>, Collector> queryCollectors = new HashMap<>();
     private final QueryShardContext queryShardContext;
     private FetchPhase fetchPhase;
@@ -385,13 +385,13 @@ public class DefaultSearchContext extends SearchContext {
     }
 
     @Override
-    public Object getFetchSubPhaseBuilder(String name) {
+    public Object getSearchExtBuilder(String name) {
-        return subPhaseBuilders.get(name);
+        return searchExtBuilders.get(name);
     }
 
     @Override
-    public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) {
+    public void putSearchExtBuilder(String name, Object searchExtBuilder) {
-        subPhaseBuilders.put(name, fetchSubPhaseBuilder);
+        searchExtBuilders.put(name, searchExtBuilder);
     }
 
     @Override

core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java

@@ -510,13 +510,13 @@ public abstract class FilteredSearchContext extends SearchContext {
     }
 
     @Override
-    public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) {
+    public void putSearchExtBuilder(String name, Object fetchSubPhaseBuilder) {
-        in.putFetchSubPhaseBuilder(name, fetchSubPhaseBuilder);
+        in.putSearchExtBuilder(name, fetchSubPhaseBuilder);
     }
 
     @Override
-    public Object getFetchSubPhaseBuilder(String name) {
+    public Object getSearchExtBuilder(String name) {
-        return in.getFetchSubPhaseBuilder(name);
+        return in.getSearchExtBuilder(name);
     }
 
     @Override

core/src/main/java/org/elasticsearch/search/internal/SearchContext.java

@@ -184,9 +184,9 @@ public abstract class SearchContext extends AbstractRefCounted implements Releas
 
     public abstract SearchContext aggregations(SearchContextAggregations aggregations);
 
-    public abstract void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder);
+    public abstract void putSearchExtBuilder(String name, Object fetchSubPhaseBuilder);
 
-    public abstract Object getFetchSubPhaseBuilder(String name);
+    public abstract Object getSearchExtBuilder(String name);
 
     public abstract SearchContextHighlight highlight();
 

core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java

@@ -103,19 +103,19 @@ public class FetchSubPhasePluginIT extends ESIntegTestCase {
         public List<FetchSubPhase> getFetchSubPhases(FetchPhaseConstructionContext context) {
             return singletonList(new TermVectorsFetchSubPhase());
         }
+
+        @Override
+        public List<SearchExtParser> getSearchExtParsers() {
+            return Collections.singletonList(TermVectorsFetchParser.INSTANCE);
+        }
     }
 
     public static final class TermVectorsFetchSubPhase implements FetchSubPhase {
         private static final String NAME = "term_vectors_fetch";
 
-        @Override
-        public SearchExtParser parser() {
-            return TermVectorsFetchParser.INSTANCE;
-        }
-
         @Override
         public void hitExecute(SearchContext context, HitContext hitContext) {
-            TermVectorsFetchBuilder fetchSubPhaseBuilder = (TermVectorsFetchBuilder)context.getFetchSubPhaseBuilder(NAME);
+            TermVectorsFetchBuilder fetchSubPhaseBuilder = (TermVectorsFetchBuilder)context.getSearchExtBuilder(NAME);
             if (fetchSubPhaseBuilder == null) {
                 return;
             }

test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java

@@ -72,8 +72,9 @@ public class MockSearchService extends SearchService {
     @Inject
     public MockSearchService(Settings settings, ClusterSettings clusterSettings, ClusterService clusterService,
             IndicesService indicesService, ThreadPool threadPool, ScriptService scriptService,
-            BigArrays bigArrays, FetchPhase fetchPhase) {
+            BigArrays bigArrays, FetchPhase fetchPhase, SearchExtParserRegistry searchExtParserRegistry) {
-        super(settings, clusterSettings, clusterService, indicesService, threadPool, scriptService, bigArrays, fetchPhase);
+        super(settings, clusterSettings, clusterService, indicesService, threadPool, scriptService,
+                bigArrays, fetchPhase, searchExtParserRegistry);
     }
 
     @Override

test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java

@@ -89,7 +89,7 @@ public class TestSearchContext extends SearchContext {
     private SearchContextAggregations aggregations;
 
     private final long originNanoTime = System.nanoTime();
-    private final Map<String, Object> subPhaseContexts = new HashMap<>();
+    private final Map<String, Object> searchExtBuilders = new HashMap<>();
 
     public TestSearchContext(ThreadPool threadPool, BigArrays bigArrays, ScriptService scriptService, IndexService indexService) {
         super(ParseFieldMatcher.STRICT);
@@ -196,13 +196,13 @@ public class TestSearchContext extends SearchContext {
     }
 
     @Override
-    public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) {
+    public void putSearchExtBuilder(String name, Object searchExtBuilders) {
-        subPhaseContexts.put(name, fetchSubPhaseBuilder);
+        this.searchExtBuilders.put(name, searchExtBuilders);
     }
 
     @Override
-    public Object getFetchSubPhaseBuilder(String name) {
+    public Object getSearchExtBuilder(String name) {
-        return subPhaseContexts.get(name);
+        return searchExtBuilders.get(name);
     }
 
     @Override