From 764efd5b680b509f43e3e919dd34af29c7a09a8b Mon Sep 17 00:00:00 2001 From: "George P. Stathis" Date: Wed, 10 Feb 2016 23:16:45 -0500 Subject: [PATCH 1/5] Issue #16594: prevents built-in similarities from being redefined, allows users to define a "default" similarity type or falls back to "classic" if none is defined. --- .../index/similarity/SimilarityService.java | 5 ++- .../similarity/SimilarityIT.java | 43 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java index e950ebda1b3..f5557b6ad4d 100644 --- a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java +++ b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java @@ -63,6 +63,8 @@ public final class SimilarityService extends AbstractIndexComponent { Map similaritySettings = this.indexSettings.getSettings().getGroups(IndexModule.SIMILARITY_SETTINGS_PREFIX); for (Map.Entry entry : similaritySettings.entrySet()) { String name = entry.getKey(); + if(BUILT_IN.containsKey(name)) + throw new IllegalArgumentException("Cannot redefine built-in Similarity [" + name + "]"); Settings settings = entry.getValue(); String typeName = settings.get("type"); if (typeName == null) { @@ -78,7 +80,8 @@ public final class SimilarityService extends AbstractIndexComponent { } addSimilarities(similaritySettings, providers, DEFAULTS); this.similarities = providers; - defaultSimilarity = providers.get(SimilarityService.DEFAULT_SIMILARITY).get(); + defaultSimilarity = (providers.get("default") != null) ? providers.get("default").get() + : providers.get(SimilarityService.DEFAULT_SIMILARITY).get(); // Expert users can configure the base type as being different to default, but out-of-box we use default. baseSimilarity = (providers.get("base") != null) ? providers.get("base").get() : defaultSimilarity; diff --git a/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java b/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java index f6fa1fc621f..badae75ae44 100644 --- a/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java +++ b/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java @@ -72,4 +72,47 @@ public class SimilarityIT extends ESIntegTestCase { assertThat(bm25Score, not(equalTo(defaultScore))); } + + // Tests #16594 + public void testCustomBM25SimilarityAsDefault() throws Exception { + try { + client().admin().indices().prepareDelete("test").execute().actionGet(); + } catch (Exception e) { + // ignore + } + + client().admin().indices().prepareCreate("test") + .addMapping("type1", jsonBuilder().startObject() + .startObject("type1") + .startObject("properties") + .startObject("field1") + .field("type", "string") + .endObject() + .startObject("field2") + .field("similarity", "custom") + .field("type", "string") + .endObject() + .endObject() + .endObject()) + .setSettings(Settings.settingsBuilder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + .put("similarity.default.type", "BM25") + .put("similarity.custom.type", "classic") + ).execute().actionGet(); + + client().prepareIndex("test", "type1", "1").setSource("field1", "the quick brown fox jumped over the lazy dog", + "field2", "the quick brown fox jumped over the lazy dog") + .setRefresh(true).execute().actionGet(); + + SearchResponse bm25SearchResponse = client().prepareSearch().setQuery(matchQuery("field1", "quick brown fox")).execute().actionGet(); + assertThat(bm25SearchResponse.getHits().totalHits(), equalTo(1L)); + float bm25Score = bm25SearchResponse.getHits().hits()[0].score(); + + SearchResponse defaultSearchResponse = client().prepareSearch().setQuery(matchQuery("field2", "quick brown fox")).execute().actionGet(); + assertThat(defaultSearchResponse.getHits().totalHits(), equalTo(1L)); + float defaultScore = defaultSearchResponse.getHits().hits()[0].score(); + + assertThat(bm25Score, not(equalTo(defaultScore))); + } } From f953d34cba0fdba7a59a92ae3ff76fce0c64c1b2 Mon Sep 17 00:00:00 2001 From: "George P. Stathis" Date: Thu, 25 Feb 2016 01:35:16 -0500 Subject: [PATCH 2/5] Adds unit tests for #16594 and removes prior integration tests. Throws exception on redefining built-in similarities only for indices created on or after v3. --- .../index/similarity/SimilarityService.java | 9 +++- .../similarity/SimilarityServiceTests.java | 49 +++++++++++++++++++ .../similarity/SimilarityIT.java | 43 ---------------- 3 files changed, 57 insertions(+), 44 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java diff --git a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java index f5557b6ad4d..7ad7dfad3a0 100644 --- a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java +++ b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java @@ -21,6 +21,7 @@ package org.elasticsearch.index.similarity; import org.apache.lucene.search.similarities.PerFieldSimilarityWrapper; import org.apache.lucene.search.similarities.Similarity; +import org.elasticsearch.Version; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.IndexModule; @@ -63,8 +64,10 @@ public final class SimilarityService extends AbstractIndexComponent { Map similaritySettings = this.indexSettings.getSettings().getGroups(IndexModule.SIMILARITY_SETTINGS_PREFIX); for (Map.Entry entry : similaritySettings.entrySet()) { String name = entry.getKey(); - if(BUILT_IN.containsKey(name)) + // Starting with v3.0 indices, it should no longer be possible to redefine built-in similarities + if(BUILT_IN.containsKey(name) && indexSettings.getIndexVersionCreated().onOrAfter(Version.V_3_0_0)) { throw new IllegalArgumentException("Cannot redefine built-in Similarity [" + name + "]"); + } Settings settings = entry.getValue(); String typeName = settings.get("type"); if (typeName == null) { @@ -109,6 +112,10 @@ public final class SimilarityService extends AbstractIndexComponent { return similarities.get(name); } + public SimilarityProvider getDefaultSimilarity() { + return similarities.get("default"); + } + static class PerFieldSimilarity extends PerFieldSimilarityWrapper { private final Similarity defaultSimilarity; diff --git a/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java b/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java new file mode 100644 index 00000000000..0ec7dc5d64d --- /dev/null +++ b/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java @@ -0,0 +1,49 @@ +/* + * 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.index.similarity; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.IndexSettingsModule; + +import java.util.Collections; + +public class SimilarityServiceTests extends ESTestCase { + + // Tests #16594 + public void testOverrideBuiltInSimilarity() { + Settings settings = Settings.builder().put("index.similarity.BM25.type", "classic").build(); + IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", settings); + try { + new SimilarityService(indexSettings, Collections.emptyMap()); + fail("can't override bm25"); + } catch (IllegalArgumentException ex) { + assertEquals(ex.getMessage(), "Cannot redefine built-in Similarity [BM25]"); + } + } + + // Tests #16594 + public void testDefaultSimilarity() { + Settings settings = Settings.builder().put("index.similarity.default.type", "BM25").build(); + IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", settings); + SimilarityService service = new SimilarityService(indexSettings, Collections.emptyMap()); + assertTrue(service.getDefaultSimilarity() instanceof BM25SimilarityProvider); + } +} diff --git a/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java b/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java index badae75ae44..f6fa1fc621f 100644 --- a/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java +++ b/core/src/test/java/org/elasticsearch/similarity/SimilarityIT.java @@ -72,47 +72,4 @@ public class SimilarityIT extends ESIntegTestCase { assertThat(bm25Score, not(equalTo(defaultScore))); } - - // Tests #16594 - public void testCustomBM25SimilarityAsDefault() throws Exception { - try { - client().admin().indices().prepareDelete("test").execute().actionGet(); - } catch (Exception e) { - // ignore - } - - client().admin().indices().prepareCreate("test") - .addMapping("type1", jsonBuilder().startObject() - .startObject("type1") - .startObject("properties") - .startObject("field1") - .field("type", "string") - .endObject() - .startObject("field2") - .field("similarity", "custom") - .field("type", "string") - .endObject() - .endObject() - .endObject()) - .setSettings(Settings.settingsBuilder() - .put("index.number_of_shards", 1) - .put("index.number_of_replicas", 0) - .put("similarity.default.type", "BM25") - .put("similarity.custom.type", "classic") - ).execute().actionGet(); - - client().prepareIndex("test", "type1", "1").setSource("field1", "the quick brown fox jumped over the lazy dog", - "field2", "the quick brown fox jumped over the lazy dog") - .setRefresh(true).execute().actionGet(); - - SearchResponse bm25SearchResponse = client().prepareSearch().setQuery(matchQuery("field1", "quick brown fox")).execute().actionGet(); - assertThat(bm25SearchResponse.getHits().totalHits(), equalTo(1L)); - float bm25Score = bm25SearchResponse.getHits().hits()[0].score(); - - SearchResponse defaultSearchResponse = client().prepareSearch().setQuery(matchQuery("field2", "quick brown fox")).execute().actionGet(); - assertThat(defaultSearchResponse.getHits().totalHits(), equalTo(1L)); - float defaultScore = defaultSearchResponse.getHits().hits()[0].score(); - - assertThat(bm25Score, not(equalTo(defaultScore))); - } } From a70df69af45be60a0b6eaeb16a86ef848d505eaf Mon Sep 17 00:00:00 2001 From: "George P. Stathis" Date: Sat, 27 Feb 2016 18:56:53 -0500 Subject: [PATCH 3/5] Allow pre v3 indices to overwrite built-in similarities. --- .../index/similarity/SimilarityService.java | 13 +++++++++++-- .../index/similarity/SimilarityServiceTests.java | 13 +++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java index 7ad7dfad3a0..cdeaacb9f28 100644 --- a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java +++ b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java @@ -81,7 +81,13 @@ public final class SimilarityService extends AbstractIndexComponent { } providers.put(name, factory.apply(name, settings)); } - addSimilarities(similaritySettings, providers, DEFAULTS); + for (Map.Entry entry : addSimilarities(similaritySettings, DEFAULTS).entrySet()) { + // Avoid overwriting custom providers for indices older that v3.0 + if (providers.containsKey(entry.getKey()) && indexSettings.getIndexVersionCreated().before(Version.V_3_0_0)) { + continue; + } + providers.put(entry.getKey(), entry.getValue()); + } this.similarities = providers; defaultSimilarity = (providers.get("default") != null) ? providers.get("default").get() : providers.get(SimilarityService.DEFAULT_SIMILARITY).get(); @@ -96,7 +102,9 @@ public final class SimilarityService extends AbstractIndexComponent { defaultSimilarity; } - private void addSimilarities(Map similaritySettings, Map providers, Map> similarities) { + private Map addSimilarities(Map similaritySettings, + Map> similarities) { + Map providers = new HashMap<>(similarities.size()); for (Map.Entry> entry : similarities.entrySet()) { String name = entry.getKey(); BiFunction factory = entry.getValue(); @@ -106,6 +114,7 @@ public final class SimilarityService extends AbstractIndexComponent { } providers.put(name, factory.apply(name, settings)); } + return providers; } public SimilarityProvider getSimilarity(String name) { diff --git a/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java b/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java index 0ec7dc5d64d..edb337fd4e6 100644 --- a/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java +++ b/core/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java @@ -18,6 +18,8 @@ */ package org.elasticsearch.index.similarity; +import org.elasticsearch.Version; +import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.test.ESTestCase; @@ -39,6 +41,17 @@ public class SimilarityServiceTests extends ESTestCase { } } + // Pre v3 indices could override built-in similarities + public void testOverrideBuiltInSimilarityPreV3() { + Settings settings = Settings.builder() + .put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_2_0_0) + .put("index.similarity.BM25.type", "classic") + .build(); + IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", settings); + SimilarityService service = new SimilarityService(indexSettings, Collections.emptyMap()); + assertTrue(service.getSimilarity("BM25") instanceof ClassicSimilarityProvider); + } + // Tests #16594 public void testDefaultSimilarity() { Settings settings = Settings.builder().put("index.similarity.default.type", "BM25").build(); From f8d2400ee697520d6defdacdd7a29104d791224d Mon Sep 17 00:00:00 2001 From: "George P. Stathis" Date: Sat, 27 Feb 2016 18:57:29 -0500 Subject: [PATCH 4/5] First pass at validating similarities insite the Settings infrastructure. --- .../common/settings/IndexScopedSettings.java | 12 ++++++++++-- .../org/elasticsearch/common/settings/Setting.java | 7 ++++++- .../common/settings/ScopedSettingsTests.java | 7 +++++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java index 157bbfbd5b9..4d550e53dac 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java @@ -35,6 +35,7 @@ import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.percolator.PercolatorQueriesRegistry; +import org.elasticsearch.index.similarity.SimilarityService; import org.elasticsearch.index.store.FsDirectoryService; import org.elasticsearch.index.store.IndexStore; import org.elasticsearch.index.store.Store; @@ -133,8 +134,15 @@ public final class IndexScopedSettings extends AbstractScopedSettings { FsDirectoryService.INDEX_LOCK_FACTOR_SETTING, EngineConfig.INDEX_CODEC_SETTING, IndexWarmer.INDEX_NORMS_LOADING_SETTING, - // this sucks but we can't really validate all the analyzers/similarity in here - Setting.groupSetting("index.similarity.", false, Setting.Scope.INDEX), // this allows similarity settings to be passed + // validate that built-in similarities don't get redefined + Setting.groupSetting("index.similarity.", false, Setting.Scope.INDEX, (s) -> { + boolean valid = true; + String similarityName = s.substring(0, s.indexOf(".")); + if(SimilarityService.BUILT_IN.keySet().contains(similarityName)) { + throw new IllegalArgumentException("Cannot redefine built-in Similarity [" + similarityName + "]"); + } + return valid; + }), // this allows similarity settings to be passed Setting.groupSetting("index.analysis.", false, Setting.Scope.INDEX) // this allows analysis settings to be passed ))); diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index 7f64c011133..a6c86edde77 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -40,6 +40,7 @@ import java.util.Objects; import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.Predicate; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -486,6 +487,10 @@ public class Setting extends ToXContentToBytes { } public static Setting groupSetting(String key, boolean dynamic, Scope scope) { + return groupSetting(key, dynamic, scope, (s) -> true); + } + + public static Setting groupSetting(String key, boolean dynamic, Scope scope, Predicate settingsValidator) { if (key.endsWith(".") == false) { throw new IllegalArgumentException("key must end with a '.'"); } @@ -498,7 +503,7 @@ public class Setting extends ToXContentToBytes { @Override public Settings get(Settings settings) { - return settings.getByPrefix(key); + return settings.getByPrefix(key).filter(settingsValidator); } @Override diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 58f5cde65ce..fa5a018aa9b 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -213,6 +213,13 @@ public class ScopedSettingsTests extends ESTestCase { } catch (IllegalArgumentException e) { assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage()); } + + try { + settings.validate("index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build()); + fail(); + } catch (IllegalArgumentException e) { + assertEquals("Cannot redefine built-in Similarity [classic]", e.getMessage()); + } } From 198a79edf574543b4f7e7568265c36ddbe53f72d Mon Sep 17 00:00:00 2001 From: "George P. Stathis" Date: Fri, 4 Mar 2016 19:32:44 -0500 Subject: [PATCH 5/5] Fix spacing, assert key contains period --- .../common/settings/IndexScopedSettings.java | 13 +++++++------ .../org/elasticsearch/common/settings/Setting.java | 2 +- .../index/similarity/SimilarityService.java | 4 ++-- .../common/settings/ScopedSettingsTests.java | 6 +++--- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java index 4d550e53dac..94e736cadec 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java @@ -136,12 +136,13 @@ public final class IndexScopedSettings extends AbstractScopedSettings { IndexWarmer.INDEX_NORMS_LOADING_SETTING, // validate that built-in similarities don't get redefined Setting.groupSetting("index.similarity.", false, Setting.Scope.INDEX, (s) -> { - boolean valid = true; - String similarityName = s.substring(0, s.indexOf(".")); - if(SimilarityService.BUILT_IN.keySet().contains(similarityName)) { - throw new IllegalArgumentException("Cannot redefine built-in Similarity [" + similarityName + "]"); - } - return valid; + boolean valid = true; + assert(s.indexOf(".") > 1); + String similarityName = s.substring(0, s.indexOf(".")); + if(SimilarityService.BUILT_IN.keySet().contains(similarityName)) { + throw new IllegalArgumentException("Cannot redefine built-in Similarity [" + similarityName + "]"); + } + return valid; }), // this allows similarity settings to be passed Setting.groupSetting("index.analysis.", false, Setting.Scope.INDEX) // this allows analysis settings to be passed diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index a6c86edde77..0ce70f89ce7 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -487,7 +487,7 @@ public class Setting extends ToXContentToBytes { } public static Setting groupSetting(String key, boolean dynamic, Scope scope) { - return groupSetting(key, dynamic, scope, (s) -> true); + return groupSetting(key, dynamic, scope, (s) -> true); } public static Setting groupSetting(String key, boolean dynamic, Scope scope, Predicate settingsValidator) { diff --git a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java index cdeaacb9f28..49307af079c 100644 --- a/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java +++ b/core/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java @@ -114,7 +114,7 @@ public final class SimilarityService extends AbstractIndexComponent { } providers.put(name, factory.apply(name, settings)); } - return providers; + return providers; } public SimilarityProvider getSimilarity(String name) { @@ -122,7 +122,7 @@ public final class SimilarityService extends AbstractIndexComponent { } public SimilarityProvider getDefaultSimilarity() { - return similarities.get("default"); + return similarities.get("default"); } static class PerFieldSimilarity extends PerFieldSimilarityWrapper { diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index fa5a018aa9b..ec64ebb6566 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -215,10 +215,10 @@ public class ScopedSettingsTests extends ESTestCase { } try { - settings.validate("index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build()); - fail(); + settings.validate("index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build()); + fail(); } catch (IllegalArgumentException e) { - assertEquals("Cannot redefine built-in Similarity [classic]", e.getMessage()); + assertEquals("Cannot redefine built-in Similarity [classic]", e.getMessage()); } }