Register Highlighter instances instead of classes (#18859)

This change detaches highlighter registration from Guice. It's just a
small step into the right direction.

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

@@ -0,0 +1,63 @@
+/*
+ * 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.settings.Settings;
+import org.elasticsearch.search.highlight.FastVectorHighlighter;
+import org.elasticsearch.search.highlight.Highlighter;
+import org.elasticsearch.search.highlight.PlainHighlighter;
+import org.elasticsearch.search.highlight.PostingsHighlighter;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * An extensions point and registry for all the highlighters a node supports.
+ */
+public final class Highlighters {
+
+    private final Map<String, Highlighter> parsers = new HashMap<>();
+
+    Highlighters(Settings settings) {
+        registerHighlighter("fvh",  new FastVectorHighlighter(settings));
+        registerHighlighter("plain", new PlainHighlighter());
+        registerHighlighter("postings", new PostingsHighlighter());
+    }
+
+    /**
+     * Returns the highlighter for the given key or <code>null</code> if there is no highlighter registered for that key.
+     */
+    public Highlighter get(String key) {
+        return parsers.get(key);
+    }
+
+    /**
+     * Registers a highlighter for the given key
+     * @param key the key the highlighter should be referenced by in the search request
+     * @param highlighter the highlighter instance
+     */
+    void registerHighlighter(String key, Highlighter highlighter) {
+        if (highlighter == null) {
+            throw new IllegalArgumentException("Can't register null highlighter for key: [" + key + "]");
+        }
+        if (parsers.putIfAbsent(key, highlighter) != null) {
+            throw new IllegalArgumentException("Can't register the same [highlighter] more than once for [" + key + "]");
+        }
+    }
+}

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

@@ -28,7 +28,6 @@ import org.elasticsearch.common.inject.multibindings.Multibinder;
 import org.elasticsearch.common.io.stream.NamedWriteable;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.Writeable;
-import org.elasticsearch.common.lucene.search.function.ScoreFunction;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.ParseFieldRegistry;
@@ -250,7 +249,6 @@ import org.elasticsearch.search.fetch.source.FetchSourceSubPhase;
 import org.elasticsearch.search.fetch.version.VersionFetchSubPhase;
 import org.elasticsearch.search.highlight.HighlightPhase;
 import org.elasticsearch.search.highlight.Highlighter;
-import org.elasticsearch.search.highlight.Highlighters;
 import org.elasticsearch.search.query.QueryPhase;
 import org.elasticsearch.search.rescore.QueryRescorerBuilder;
 import org.elasticsearch.search.rescore.RescoreBuilder;
@@ -270,9 +268,8 @@ import java.util.Set;
  */
 public class SearchModule extends AbstractModule {
 
-    private final Highlighters highlighters = new Highlighters();
+    private final Highlighters highlighters;
     private final Suggesters suggesters;
-
     private final ParseFieldRegistry<ScoreFunctionParser<?>> scoreFunctionParserRegistry = new ParseFieldRegistry<>("score_function");
     private final IndicesQueriesRegistry queryParserRegistry = new IndicesQueriesRegistry();
     private final ParseFieldRegistry<Aggregator.Parser> aggregationParserRegistry = new ParseFieldRegistry<>("aggregation");
@@ -298,7 +295,7 @@ public class SearchModule extends AbstractModule {
         this.settings = settings;
         this.namedWriteableRegistry = namedWriteableRegistry;
         suggesters = new Suggesters(namedWriteableRegistry);
-
+        highlighters = new Highlighters(settings);
         registerBuiltinScoreFunctionParsers();
         registerBuiltinQueryParsers();
         registerBuiltinRescorers();
@@ -308,8 +305,8 @@ public class SearchModule extends AbstractModule {
         registerBuiltinMovingAverageModels();
     }
 
-    public void registerHighlighter(String key, Class<? extends Highlighter> clazz) {
-        highlighters.registerExtension(key, clazz);
+    public void registerHighlighter(String key, Highlighter highligher) {
+        highlighters.registerHighlighter(key, highligher);
     }
 
     public void registerSuggester(String key, Suggester<?> suggester) {
@@ -331,13 +328,6 @@ public class SearchModule extends AbstractModule {
         namedWriteableRegistry.register(ScoreFunctionBuilder.class, functionName.getPreferredName(), reader);
     }
 
-    /**
-     * Fetch the registry of {@linkplain ScoreFunction}s. This is public so extensions can access the score functions.
-     */
-    public ParseFieldRegistry<ScoreFunctionParser<?>> getScoreFunctionParserRegistry() {
-        return scoreFunctionParserRegistry;
-    }
-
     /**
      * Register a new ValueFormat.
      */
@@ -441,10 +431,6 @@ public class SearchModule extends AbstractModule {
         namedWriteableRegistry.register(PipelineAggregatorBuilder.class, aggregationName.getPreferredName(), reader);
     }
 
-    public AggregatorParsers getAggregatorParsers() {
-        return aggregatorParsers;
-    }
-
     @Override
     protected void configure() {
         bind(IndicesQueriesRegistry.class).toInstance(queryParserRegistry);
@@ -473,7 +459,7 @@ public class SearchModule extends AbstractModule {
     }
 
     protected void configureHighlighters() {
-       highlighters.bind(binder());
+       binder().bind(Highlighters.class).toInstance(highlighters);
     }
 
     protected void configureAggs() {

core/src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java

@@ -30,6 +30,7 @@ import org.elasticsearch.index.mapper.core.KeywordFieldMapper;
 import org.elasticsearch.index.mapper.core.StringFieldMapper;
 import org.elasticsearch.index.mapper.core.TextFieldMapper;
 import org.elasticsearch.index.mapper.internal.SourceFieldMapper;
+import org.elasticsearch.search.Highlighters;
 import org.elasticsearch.search.SearchParseElement;
 import org.elasticsearch.search.fetch.FetchSubPhase;
 import org.elasticsearch.search.internal.InternalSearchHit;

core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java

@@ -1,69 +0,0 @@
-/*
- * 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.highlight;
-
-import org.elasticsearch.common.inject.Inject;
-import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.util.ExtensionPoint;
-
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-
-/**
- * An extensions point and registry for all the highlighters a node supports.
- */
-public class Highlighters extends ExtensionPoint.ClassMap<Highlighter> {
-
-    private static final String FVH = "fvh";
-    private static final String PLAIN = "plain";
-    private static final String POSTINGS = "postings";
-
-    private final Map<String, Highlighter> parsers;
-
-    public Highlighters(){
-        this(Collections.emptyMap());
-    }
-
-    private Highlighters(Map<String, Highlighter> parsers) {
-        super("highlighter", Highlighter.class, new HashSet<>(Arrays.asList(FVH, PLAIN, POSTINGS)),
-                Highlighters.class);
-        this.parsers = Collections.unmodifiableMap(parsers);
-    }
-
-    @Inject
-    public Highlighters(Settings settings, Map<String, Highlighter> parsers) {
-        this(addBuiltIns(settings, parsers));
-    }
-
-    private static Map<String, Highlighter> addBuiltIns(Settings settings, Map<String, Highlighter> parsers) {
-        Map<String, Highlighter> map = new HashMap<>();
-        map.put(FVH,  new FastVectorHighlighter(settings));
-        map.put(PLAIN, new PlainHighlighter());
-        map.put(POSTINGS, new PostingsHighlighter());
-        map.putAll(parsers);
-        return map;
-    }
-
-    public Highlighter get(String type) {
-        return parsers.get(type);
-    }
-}

core/src/test/java/org/elasticsearch/search/SearchModuleTests.java

@@ -29,8 +29,9 @@ import org.elasticsearch.index.query.QueryParser;
 import org.elasticsearch.index.query.TermQueryBuilder;
 import org.elasticsearch.indices.query.IndicesQueriesRegistry;
 import org.elasticsearch.search.highlight.CustomHighlighter;
-import org.elasticsearch.search.highlight.Highlighter;
+import org.elasticsearch.search.highlight.FastVectorHighlighter;
 import org.elasticsearch.search.highlight.PlainHighlighter;
+import org.elasticsearch.search.highlight.PostingsHighlighter;
 import org.elasticsearch.search.suggest.CustomSuggester;
 import org.elasticsearch.search.suggest.phrase.PhraseSuggester;
 
@@ -48,7 +49,7 @@ public class SearchModuleTests extends ModuleTestCase {
    public void testDoubleRegister() {
        SearchModule module = new SearchModule(Settings.EMPTY, new NamedWriteableRegistry());
        try {
-           module.registerHighlighter("fvh", PlainHighlighter.class);
+           module.registerHighlighter("fvh", new PlainHighlighter());
        } catch (IllegalArgumentException e) {
            assertEquals(e.getMessage(), "Can't register the same [highlighter] more than once for [fvh]");
        }
@@ -70,13 +71,18 @@ public class SearchModuleTests extends ModuleTestCase {
 
     public void testRegisterHighlighter() {
         SearchModule module = new SearchModule(Settings.EMPTY, new NamedWriteableRegistry());
-        module.registerHighlighter("custom", CustomHighlighter.class);
-        try {
-            module.registerHighlighter("custom", CustomHighlighter.class);
-        } catch (IllegalArgumentException e) {
-            assertEquals(e.getMessage(), "Can't register the same [highlighter] more than once for [custom]");
-        }
-        assertMapMultiBinding(module, Highlighter.class, CustomHighlighter.class);
+        module.registerHighlighter("custom",  new CustomHighlighter());
+        IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
+            () -> module.registerHighlighter("custom", new CustomHighlighter()));
+        assertEquals("Can't register the same [highlighter] more than once for [custom]", exception.getMessage());
+
+        exception = expectThrows(IllegalArgumentException.class,
+            () -> module.registerHighlighter("custom", null));
+        assertEquals("Can't register null highlighter for key: [custom]", exception.getMessage());
+        assertInstanceBinding(module, Highlighters.class, (h) -> h.get("custom").getClass() == CustomHighlighter.class
+        && h.get("fvh").getClass() == FastVectorHighlighter.class
+        && h.get("plain").getClass() == PlainHighlighter.class
+        && h.get("postings").getClass() == PostingsHighlighter.class);
     }
 
     public void testRegisterQueryParserDuplicate() {

core/src/test/java/org/elasticsearch/search/highlight/CustomHighlighterPlugin.java

@@ -35,6 +35,6 @@ public class CustomHighlighterPlugin extends Plugin {
     }
 
     public void onModule(SearchModule highlightModule) {
-        highlightModule.registerHighlighter("test-custom", CustomHighlighter.class);
+        highlightModule.registerHighlighter("test-custom", new CustomHighlighter());
     }
 }