From ee2ba13cce21382ea81ab204ab9cb71e30cc4580 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 14 Jun 2016 17:04:58 +0200 Subject: [PATCH] Register Highlighter instances instead of classes (#18859) This change detaches highlighter registration from Guice. It's just a small step into the right direction. --- .../elasticsearch/search/Highlighters.java | 63 +++++++++++++++++ .../elasticsearch/search/SearchModule.java | 24 ++----- .../search/highlight/HighlightPhase.java | 1 + .../search/highlight/Highlighters.java | 69 ------------------- .../search/SearchModuleTests.java | 24 ++++--- .../highlight/CustomHighlighterPlugin.java | 2 +- 6 files changed, 85 insertions(+), 98 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/search/Highlighters.java delete mode 100644 core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java diff --git a/core/src/main/java/org/elasticsearch/search/Highlighters.java b/core/src/main/java/org/elasticsearch/search/Highlighters.java new file mode 100644 index 00000000000..38fed839d6e --- /dev/null +++ b/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 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 null 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 + "]"); + } + } +} diff --git a/core/src/main/java/org/elasticsearch/search/SearchModule.java b/core/src/main/java/org/elasticsearch/search/SearchModule.java index 8afda85c58f..2f626a82496 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/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> scoreFunctionParserRegistry = new ParseFieldRegistry<>("score_function"); private final IndicesQueriesRegistry queryParserRegistry = new IndicesQueriesRegistry(); private final ParseFieldRegistry 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 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> 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() { diff --git a/core/src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java b/core/src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java index 92011fd77e7..701cd588892 100644 --- a/core/src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java +++ b/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; diff --git a/core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java b/core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java deleted file mode 100644 index 30b8d15d93d..00000000000 --- a/core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java +++ /dev/null @@ -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 { - - private static final String FVH = "fvh"; - private static final String PLAIN = "plain"; - private static final String POSTINGS = "postings"; - - private final Map parsers; - - public Highlighters(){ - this(Collections.emptyMap()); - } - - private Highlighters(Map 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 parsers) { - this(addBuiltIns(settings, parsers)); - } - - private static Map addBuiltIns(Settings settings, Map parsers) { - Map 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); - } -} diff --git a/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java b/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java index 384e181c899..2cfb249f533 100644 --- a/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java +++ b/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() { diff --git a/core/src/test/java/org/elasticsearch/search/highlight/CustomHighlighterPlugin.java b/core/src/test/java/org/elasticsearch/search/highlight/CustomHighlighterPlugin.java index 80e39a2a6dd..b6c42b73677 100644 --- a/core/src/test/java/org/elasticsearch/search/highlight/CustomHighlighterPlugin.java +++ b/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()); } }