From b47f5976a52de0c097ebbe1446f601947039b255 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 10 Aug 2015 01:16:50 -0700 Subject: [PATCH] Mappings: Fix field type compatiblity check to work when only one previous type exists. This was a straight up bug found in #12753. If only one type existed, the compatibility check for a new type was not strict, so changes to an updateable setting like search_analyzer got through (but only partially). This change fixes the check and adds tests (which were previously a TODO). This also fixes a bug in dynamic field creation which woudln't copy fielddata settings when duplicating a pre-existing field with the same name. closes #12753 --- .../index/mapper/DocumentParser.java | 2 + .../index/mapper/FieldTypeLookup.java | 10 +-- .../index/mapper/MappedFieldType.java | 2 +- .../index/mapper/FieldTypeLookupTests.java | 85 ++++++++++++++++++- .../search/child/ParentFieldLoadingIT.java | 12 ++- 5 files changed, 100 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 6aa66f20d6f..e379ea45968 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -629,6 +629,7 @@ class DocumentParser implements Closeable { // best-effort to not introduce a conflict if (builder instanceof StringFieldMapper.Builder) { StringFieldMapper.Builder stringBuilder = (StringFieldMapper.Builder) builder; + stringBuilder.fieldDataSettings(existingFieldType.fieldDataType().getSettings()); stringBuilder.store(existingFieldType.stored()); stringBuilder.indexOptions(existingFieldType.indexOptions()); stringBuilder.tokenized(existingFieldType.tokenized()); @@ -638,6 +639,7 @@ class DocumentParser implements Closeable { stringBuilder.searchAnalyzer(existingFieldType.searchAnalyzer()); } else if (builder instanceof NumberFieldMapper.Builder) { NumberFieldMapper.Builder numberBuilder = (NumberFieldMapper.Builder) builder; + numberBuilder.fieldDataSettings(existingFieldType.fieldDataType().getSettings()); numberBuilder.store(existingFieldType.stored()); numberBuilder.indexOptions(existingFieldType.indexOptions()); numberBuilder.tokenized(existingFieldType.tokenized()); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index 7f4d76d0b42..4dfba263e23 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -110,21 +110,21 @@ class FieldTypeLookup implements Iterable { List conflicts = new ArrayList<>(); ref.get().checkTypeName(fieldMapper.fieldType(), conflicts); if (conflicts.isEmpty()) { // only check compat if they are the same type - boolean strict = ref.getNumAssociatedMappers() > 1 && updateAllTypes == false; + boolean strict = updateAllTypes == false; ref.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict); } if (conflicts.isEmpty() == false) { - throw new IllegalArgumentException("Mapper for [" + fieldMapper.fieldType().names().fullName() + "] conflicts with existing mapping in other types" + conflicts.toString()); + throw new IllegalArgumentException("Mapper for [" + fieldMapper.fieldType().names().fullName() + "] conflicts with existing mapping in other types:\n" + conflicts.toString()); } } // field type for the index name must be compatible too - MappedFieldTypeReference indexNameRef = fullNameToFieldType.get(fieldMapper.fieldType().names().indexName()); + MappedFieldTypeReference indexNameRef = indexNameToFieldType.get(fieldMapper.fieldType().names().indexName()); if (indexNameRef != null) { List conflicts = new ArrayList<>(); - ref.get().checkTypeName(fieldMapper.fieldType(), conflicts); + indexNameRef.get().checkTypeName(fieldMapper.fieldType(), conflicts); if (conflicts.isEmpty()) { // only check compat if they are the same type - boolean strict = indexNameRef.getNumAssociatedMappers() > 1 && updateAllTypes == false; + boolean strict = updateAllTypes == false; indexNameRef.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict); } if (conflicts.isEmpty() == false) { diff --git a/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java index 7dc78ddd5e0..65113e61033 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java @@ -274,7 +274,7 @@ public abstract class MappedFieldType extends FieldType { conflicts.add("mapper [" + names().fullName() + "] has different analyzer"); } - if (!names().equals(other.names())) { + if (!names().indexName().equals(other.names().indexName())) { conflicts.add("mapper [" + names().fullName() + "] has different index_name"); } if (Objects.equals(similarity(), other.similarity()) == false) { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java b/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java index ca31a789f28..bfefe436fa7 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -24,6 +24,7 @@ import com.google.common.collect.Lists; import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.mapper.core.StringFieldMapper; import org.elasticsearch.test.ESTestCase; import java.io.IOException; @@ -131,7 +132,67 @@ public class FieldTypeLookupTests extends ESTestCase { } } - // TODO: add tests for validation + public void testCheckCompatibilityNewField() { + FakeFieldMapper f1 = new FakeFieldMapper("foo", "bar"); + FieldTypeLookup lookup = new FieldTypeLookup(); + lookup.checkCompatibility(newList(f1), false); + } + + public void testCheckCompatibilityMismatchedTypes() { + FieldMapper f1 = new FakeFieldMapper("foo", "bar"); + FieldTypeLookup lookup = new FieldTypeLookup(); + lookup = lookup.copyAndAddAll(newList(f1)); + + MappedFieldType ft2 = FakeFieldMapper.makeOtherFieldType("foo", "foo"); + FieldMapper f2 = new FakeFieldMapper("foo", ft2); + try { + lookup.checkCompatibility(newList(f2), false); + fail("expected type mismatch"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]")); + } + // fails even if updateAllTypes == true + try { + lookup.checkCompatibility(newList(f2), true); + fail("expected type mismatch"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]")); + } + } + + public void testCheckCompatibilityConflict() { + FieldMapper f1 = new FakeFieldMapper("foo", "bar"); + FieldTypeLookup lookup = new FieldTypeLookup(); + lookup = lookup.copyAndAddAll(newList(f1)); + + MappedFieldType ft2 = FakeFieldMapper.makeFieldType("foo", "bar"); + ft2.setBoost(2.0f); + FieldMapper f2 = new FakeFieldMapper("foo", ft2); + try { + lookup.checkCompatibility(newList(f2), false); + fail("expected conflict"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().contains("to update [boost] across all types")); + } + lookup.checkCompatibility(newList(f2), true); // boost is updateable, so ok if forcing + // now with a non changeable setting + MappedFieldType ft3 = FakeFieldMapper.makeFieldType("foo", "bar"); + ft3.setStored(true); + FieldMapper f3 = new FakeFieldMapper("foo", ft3); + try { + lookup.checkCompatibility(newList(f3), false); + fail("expected conflict"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().contains("has different store values")); + } + // even with updateAllTypes == true, incompatible + try { + lookup.checkCompatibility(newList(f3), true); + fail("expected conflict"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().contains("has different store values")); + } + } public void testSimpleMatchIndexNames() { FakeFieldMapper f1 = new FakeFieldMapper("foo", "baz"); @@ -179,11 +240,19 @@ public class FieldTypeLookupTests extends ESTestCase { public FakeFieldMapper(String fullName, String indexName) { super(fullName, makeFieldType(fullName, indexName), makeFieldType(fullName, indexName), dummySettings, null, null); } + public FakeFieldMapper(String fullName, MappedFieldType fieldType) { + super(fullName, fieldType, fieldType, dummySettings, null, null); + } static MappedFieldType makeFieldType(String fullName, String indexName) { FakeFieldType fieldType = new FakeFieldType(); fieldType.setNames(new MappedFieldType.Names(indexName, indexName, fullName)); return fieldType; } + static MappedFieldType makeOtherFieldType(String fullName, String indexName) { + OtherFakeFieldType fieldType = new OtherFakeFieldType(); + fieldType.setNames(new MappedFieldType.Names(indexName, indexName, fullName)); + return fieldType; + } static class FakeFieldType extends MappedFieldType { public FakeFieldType() {} protected FakeFieldType(FakeFieldType ref) { @@ -198,6 +267,20 @@ public class FieldTypeLookupTests extends ESTestCase { return "faketype"; } } + static class OtherFakeFieldType extends MappedFieldType { + public OtherFakeFieldType() {} + protected OtherFakeFieldType(OtherFakeFieldType ref) { + super(ref); + } + @Override + public MappedFieldType clone() { + return new OtherFakeFieldType(this); + } + @Override + public String typeName() { + return "otherfaketype"; + } + } @Override protected String contentType() { return null; } @Override diff --git a/core/src/test/java/org/elasticsearch/search/child/ParentFieldLoadingIT.java b/core/src/test/java/org/elasticsearch/search/child/ParentFieldLoadingIT.java index 86596648c39..8a57e22297f 100644 --- a/core/src/test/java/org/elasticsearch/search/child/ParentFieldLoadingIT.java +++ b/core/src/test/java/org/elasticsearch/search/child/ParentFieldLoadingIT.java @@ -57,7 +57,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase { assertAcked(prepareCreate("test") .setSettings(indexSettings) .addMapping("parent") - .addMapping("child", childMapping(MappedFieldType.Loading.LAZY))); + .addMapping("child", childMapping(MappedFieldType.Loading.LAZY)) + .setUpdateAllTypes(true)); ensureGreen(); client().prepareIndex("test", "parent", "1").setSource("{}").get(); @@ -72,7 +73,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase { assertAcked(prepareCreate("test") .setSettings(indexSettings) .addMapping("parent") - .addMapping("child", "_parent", "type=parent")); + .addMapping("child", "_parent", "type=parent") + .setUpdateAllTypes(true)); ensureGreen(); client().prepareIndex("test", "parent", "1").setSource("{}").get(); @@ -87,7 +89,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase { assertAcked(prepareCreate("test") .setSettings(indexSettings) .addMapping("parent") - .addMapping("child", childMapping(MappedFieldType.Loading.EAGER))); + .addMapping("child", childMapping(MappedFieldType.Loading.EAGER)) + .setUpdateAllTypes(true)); ensureGreen(); client().prepareIndex("test", "parent", "1").setSource("{}").get(); @@ -102,7 +105,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase { assertAcked(prepareCreate("test") .setSettings(indexSettings) .addMapping("parent") - .addMapping("child", childMapping(MappedFieldType.Loading.EAGER_GLOBAL_ORDINALS))); + .addMapping("child", childMapping(MappedFieldType.Loading.EAGER_GLOBAL_ORDINALS)) + .setUpdateAllTypes(true)); ensureGreen(); // Need to do 2 separate refreshes, otherwise we have 1 segment and then we can't measure if global ordinals