From 6ca36bba15af11c30850a7401ec6e0f552296407 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Thu, 13 Sep 2018 09:21:27 +0200 Subject: [PATCH] Fix field mapping updates with similarity (#33634) This change fixes a bug introduced in 6.3 that prevents fields with an explicit similarity to be updated. It also adds a test that checks this case for similarities but also for analyzers since they could suffer from the same problem. Closes #33611 --- .../index/mapper/MappedFieldType.java | 14 ++-------- .../index/similarity/SimilarityProvider.java | 26 +++++++++++++++++++ .../index/mapper/FieldTypeTestCase.java | 22 ++++++++++++++++ 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java index 4a3fa852e7f..82a601de05e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java @@ -111,17 +111,6 @@ public abstract class MappedFieldType extends FieldType { public boolean equals(Object o) { if (!super.equals(o)) return false; MappedFieldType fieldType = (MappedFieldType) o; - // check similarity first because we need to check the name, and it might be null - // TODO: SimilarityProvider should have equals? - if (similarity == null || fieldType.similarity == null) { - if (similarity != fieldType.similarity) { - return false; - } - } else { - if (Objects.equals(similarity.name(), fieldType.similarity.name()) == false) { - return false; - } - } return boost == fieldType.boost && docValues == fieldType.docValues && @@ -131,7 +120,8 @@ public abstract class MappedFieldType extends FieldType { Objects.equals(searchQuoteAnalyzer(), fieldType.searchQuoteAnalyzer()) && Objects.equals(eagerGlobalOrdinals, fieldType.eagerGlobalOrdinals) && Objects.equals(nullValue, fieldType.nullValue) && - Objects.equals(nullValueAsString, fieldType.nullValueAsString); + Objects.equals(nullValueAsString, fieldType.nullValueAsString) && + Objects.equals(similarity, fieldType.similarity); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/similarity/SimilarityProvider.java b/server/src/main/java/org/elasticsearch/index/similarity/SimilarityProvider.java index fed15b30583..f5a870441d4 100644 --- a/server/src/main/java/org/elasticsearch/index/similarity/SimilarityProvider.java +++ b/server/src/main/java/org/elasticsearch/index/similarity/SimilarityProvider.java @@ -21,6 +21,8 @@ package org.elasticsearch.index.similarity; import org.apache.lucene.search.similarities.Similarity; +import java.util.Objects; + /** * Wrapper around a {@link Similarity} and its name. */ @@ -48,4 +50,28 @@ public final class SimilarityProvider { return similarity; } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + SimilarityProvider that = (SimilarityProvider) o; + /** + * We check name only because the similarity is + * re-created for each new instance and they don't implement equals. + * This is not entirely correct though but we only use equality checks + * for similarities inside the same index and names are unique in this case. + **/ + return Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + /** + * We use name only because the similarity is + * re-created for each new instance and they don't implement equals. + * This is not entirely correct though but we only use equality checks + * for similarities a single index and names are unique in this case. + **/ + return Objects.hash(name); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java index 28767cb34d7..42eab104d6a 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java @@ -89,6 +89,17 @@ public abstract class FieldTypeTestCase extends ESTestCase { other.setIndexAnalyzer(new NamedAnalyzer("foo", AnalyzerScope.INDEX, new StandardAnalyzer())); } }, + // check that we can update if the analyzer is unchanged + new Modifier("analyzer", true) { + @Override + public void modify(MappedFieldType ft) { + ft.setIndexAnalyzer(new NamedAnalyzer("foo", AnalyzerScope.INDEX, new StandardAnalyzer())); + } + @Override + public void normalizeOther(MappedFieldType other) { + other.setIndexAnalyzer(new NamedAnalyzer("foo", AnalyzerScope.INDEX, new StandardAnalyzer())); + } + }, new Modifier("search_analyzer", true) { @Override public void modify(MappedFieldType ft) { @@ -137,6 +148,17 @@ public abstract class FieldTypeTestCase extends ESTestCase { other.setSimilarity(new SimilarityProvider("bar", new BM25Similarity())); } }, + // check that we can update if the similarity is unchanged + new Modifier("similarity", true) { + @Override + public void modify(MappedFieldType ft) { + ft.setSimilarity(new SimilarityProvider("foo", new BM25Similarity())); + } + @Override + public void normalizeOther(MappedFieldType other) { + other.setSimilarity(new SimilarityProvider("foo", new BM25Similarity())); + } + }, new Modifier("eager_global_ordinals", true) { @Override public void modify(MappedFieldType ft) {