diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index c7d92e9f829..ab8b5c612fe 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -22,11 +22,9 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.common.collect.CopyOnWriteHashMap; import org.elasticsearch.common.regex.Regex; -import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.Iterator; -import java.util.List; import java.util.Objects; import java.util.Set; @@ -71,7 +69,6 @@ class FieldTypeLookup implements Iterable { MappedFieldType fullNameFieldType = fullName.get(fieldType.name()); if (!Objects.equals(fieldType, fullNameFieldType)) { - validateField(fullNameFieldType, fieldType, aliases); fullName = fullName.copyAndPut(fieldType.name(), fieldType); } } @@ -79,66 +76,12 @@ class FieldTypeLookup implements Iterable { for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) { String aliasName = fieldAliasMapper.name(); String path = fieldAliasMapper.path(); - - validateAlias(aliasName, path, aliases, fullName); aliases = aliases.copyAndPut(aliasName, path); } return new FieldTypeLookup(fullName, aliases); } - /** - * Checks that the new field type is valid. - */ - private void validateField(MappedFieldType existingFieldType, - MappedFieldType newFieldType, - CopyOnWriteHashMap aliasToConcreteName) { - String fieldName = newFieldType.name(); - if (aliasToConcreteName.containsKey(fieldName)) { - throw new IllegalArgumentException("The name for field [" + fieldName + "] has already" + - " been used to define a field alias."); - } - - if (existingFieldType != null) { - List conflicts = new ArrayList<>(); - existingFieldType.checkCompatibility(newFieldType, conflicts); - if (conflicts.isEmpty() == false) { - throw new IllegalArgumentException("Mapper for [" + fieldName + - "] conflicts with existing mapping:\n" + conflicts.toString()); - } - } - } - - /** - * Checks that the new field alias is valid. - * - * Note that this method assumes that new concrete fields have already been processed, so that it - * can verify that an alias refers to an existing concrete field. - */ - private void validateAlias(String aliasName, - String path, - CopyOnWriteHashMap aliasToConcreteName, - CopyOnWriteHashMap fullNameToFieldType) { - if (fullNameToFieldType.containsKey(aliasName)) { - throw new IllegalArgumentException("The name for field alias [" + aliasName + "] has already" + - " been used to define a concrete field."); - } - - if (path.equals(aliasName)) { - throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + - aliasName + "]: an alias cannot refer to itself."); - } - - if (aliasToConcreteName.containsKey(path)) { - throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + - aliasName + "]: an alias cannot refer to another alias."); - } - - if (!fullNameToFieldType.containsKey(path)) { - throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + - aliasName + "]: an alias must refer to an existing field in the mappings."); - } - } /** Returns the field for the given field */ public MappedFieldType get(String field) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java index 3ca70f4c999..0a6dfddffb2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -19,13 +19,13 @@ package org.elasticsearch.index.mapper; +import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.stream.Stream; /** * A utility class that helps validate certain aspects of a mappings update. @@ -33,17 +33,18 @@ import java.util.stream.Stream; class MapperMergeValidator { /** - * Validates the overall structure of the mapping addition, including whether - * duplicate fields are present, and if the provided fields have already been - * defined with a different data type. + * Validates the new mapping addition, checking whether duplicate entries are present and if the + * provided fields are compatible with the mappings that are already defined. * * @param objectMappers The newly added object mappers. * @param fieldMappers The newly added field mappers. * @param fieldAliasMappers The newly added field alias mappers. + * @param fieldTypes Any existing field and field alias mappers, collected into a lookup structure. */ - public static void validateMapperStructure(Collection objectMappers, - Collection fieldMappers, - Collection fieldAliasMappers) { + public static void validateNewMappers(Collection objectMappers, + Collection fieldMappers, + Collection fieldAliasMappers, + FieldTypeLookup fieldTypes) { Set objectFullNames = new HashSet<>(); for (ObjectMapper objectMapper : objectMappers) { String fullPath = objectMapper.fullPath(); @@ -53,17 +54,75 @@ class MapperMergeValidator { } Set fieldNames = new HashSet<>(); - Stream.concat(fieldMappers.stream(), fieldAliasMappers.stream()) - .forEach(mapper -> { - String name = mapper.name(); - if (objectFullNames.contains(name)) { - throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field."); - } else if (fieldNames.add(name) == false) { - throw new IllegalArgumentException("Field [" + name + "] is defined twice."); - } - }); + for (FieldMapper fieldMapper : fieldMappers) { + String name = fieldMapper.name(); + if (objectFullNames.contains(name)) { + throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field."); + } else if (fieldNames.add(name) == false) { + throw new IllegalArgumentException("Field [" + name + "] is defined twice."); + } + + validateFieldMapper(fieldMapper, fieldTypes); + } + + Set fieldAliasNames = new HashSet<>(); + for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) { + String name = fieldAliasMapper.name(); + if (objectFullNames.contains(name)) { + throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field."); + } else if (fieldNames.contains(name)) { + throw new IllegalArgumentException("Field [" + name + "] is defined both as an alias and a concrete field."); + } else if (fieldAliasNames.add(name) == false) { + throw new IllegalArgumentException("Field [" + name + "] is defined twice."); + } + + validateFieldAliasMapper(name, fieldAliasMapper.path(), fieldNames, fieldAliasNames); + } } + /** + * Checks that the new field mapper does not conflict with existing mappings. + */ + private static void validateFieldMapper(FieldMapper fieldMapper, + FieldTypeLookup fieldTypes) { + MappedFieldType newFieldType = fieldMapper.fieldType(); + MappedFieldType existingFieldType = fieldTypes.get(newFieldType.name()); + + if (existingFieldType != null && Objects.equals(newFieldType, existingFieldType) == false) { + List conflicts = new ArrayList<>(); + existingFieldType.checkCompatibility(newFieldType, conflicts); + if (conflicts.isEmpty() == false) { + throw new IllegalArgumentException("Mapper for [" + newFieldType.name() + + "] conflicts with existing mapping:\n" + conflicts.toString()); + } + } + } + + /** + * Checks that the new field alias is valid. + * + * Note that this method assumes that new concrete fields have already been processed, so that it + * can verify that an alias refers to an existing concrete field. + */ + private static void validateFieldAliasMapper(String aliasName, + String path, + Set fieldMappers, + Set fieldAliasMappers) { + if (path.equals(aliasName)) { + throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + + aliasName + "]: an alias cannot refer to itself."); + } + + if (fieldAliasMappers.contains(path)) { + throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + + aliasName + "]: an alias cannot refer to another alias."); + } + + if (fieldMappers.contains(path) == false) { + throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + + aliasName + "]: an alias must refer to an existing field in the mappings."); + } + } /** * Verifies that each field reference, e.g. the value of copy_to or the target * of a field alias, corresponds to a valid part of the mapping. diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 0716b5a8c28..53d49e657a3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -471,11 +471,10 @@ public class MapperService extends AbstractIndexComponent implements Closeable { Collections.addAll(fieldMappers, metadataMappers); MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers); - MapperMergeValidator.validateMapperStructure(objectMappers, fieldMappers, fieldAliasMappers); + MapperMergeValidator.validateNewMappers(objectMappers, fieldMappers, fieldAliasMappers, fieldTypes); checkPartitionedIndexConstraints(newMapper); // update lookup data-structures - // this will in particular make sure that the merged fields are compatible with other types fieldTypes = fieldTypes.copyAndAddAll(newMapper.type(), fieldMappers, fieldAliasMappers); for (ObjectMapper objectMapper : objectMappers) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java index 6e27823f8a0..78a069d4959 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -79,46 +79,6 @@ public class FieldTypeLookupTests extends ESTestCase { assertEquals(f2.fieldType(), lookup2.get("foo")); } - public void testMismatchedFieldTypes() { - FieldMapper f1 = new MockFieldMapper("foo"); - FieldTypeLookup lookup = new FieldTypeLookup(); - lookup = lookup.copyAndAddAll("type", newList(f1), emptyList()); - - OtherFakeFieldType ft2 = new OtherFakeFieldType(); - ft2.setName("foo"); - FieldMapper f2 = new MockFieldMapper("foo", ft2); - try { - lookup.copyAndAddAll("type2", newList(f2), emptyList()); - fail("expected type mismatch"); - } catch (IllegalArgumentException e) { - assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]")); - } - } - - public void testConflictingFieldTypes() { - FieldMapper f1 = new MockFieldMapper("foo"); - FieldTypeLookup lookup = new FieldTypeLookup(); - lookup = lookup.copyAndAddAll("type", newList(f1), emptyList()); - - MappedFieldType ft2 = new MockFieldMapper.FakeFieldType(); - ft2.setName("foo"); - ft2.setBoost(2.0f); - FieldMapper f2 = new MockFieldMapper("foo", ft2); - lookup.copyAndAddAll("type", newList(f2), emptyList()); // boost is updateable, so ok since we are implicitly updating all types - lookup.copyAndAddAll("type2", newList(f2), emptyList()); // boost is updateable, so ok if forcing - // now with a non changeable setting - MappedFieldType ft3 = new MockFieldMapper.FakeFieldType(); - ft3.setName("foo"); - ft3.setStored(true); - FieldMapper f3 = new MockFieldMapper("foo", ft3); - try { - lookup.copyAndAddAll("type2", newList(f3), emptyList()); - fail("expected conflict"); - } catch (IllegalArgumentException e) { - assertTrue(e.getMessage().contains("has different [store] values")); - } - } - public void testAddFieldAlias() { MockFieldMapper field = new MockFieldMapper("foo"); FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo"); @@ -181,68 +141,6 @@ public class FieldTypeLookupTests extends ESTestCase { assertEquals(fieldType2, aliasType2); } - public void testAliasThatRefersToAlias() { - MockFieldMapper field = new MockFieldMapper("foo"); - FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo"); - FieldTypeLookup lookup = new FieldTypeLookup() - .copyAndAddAll("type", newList(field), newList(alias)); - - FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "alias"); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> lookup.copyAndAddAll("type", emptyList(), newList(invalidAlias))); - assertEquals("Invalid [path] value [alias] for field alias [invalid-alias]: an alias" + - " cannot refer to another alias.", e.getMessage()); - } - - public void testAliasThatRefersToItself() { - FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "invalid-alias"); - - FieldTypeLookup lookup = new FieldTypeLookup(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> lookup.copyAndAddAll("type", emptyList(), newList(invalidAlias))); - assertEquals("Invalid [path] value [invalid-alias] for field alias [invalid-alias]: an alias" + - " cannot refer to itself.", e.getMessage()); - } - - public void testAliasWithNonExistentPath() { - FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "non-existent"); - - FieldTypeLookup lookup = new FieldTypeLookup(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> lookup.copyAndAddAll("type", emptyList(), newList(invalidAlias))); - assertEquals("Invalid [path] value [non-existent] for field alias [invalid-alias]: an alias" + - " must refer to an existing field in the mappings.", e.getMessage()); - } - - public void testAddAliasWithPreexistingField() { - MockFieldMapper field = new MockFieldMapper("field"); - FieldTypeLookup lookup = new FieldTypeLookup() - .copyAndAddAll("type", newList(field), emptyList()); - - MockFieldMapper invalidField = new MockFieldMapper("invalid"); - FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid", "invalid", "field"); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> lookup.copyAndAddAll("type", newList(invalidField), newList(invalidAlias))); - assertEquals("The name for field alias [invalid] has already been used to define a concrete field.", - e.getMessage()); - } - - public void testAddFieldWithPreexistingAlias() { - MockFieldMapper field = new MockFieldMapper("field"); - FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid", "invalid", "field"); - - FieldTypeLookup lookup = new FieldTypeLookup() - .copyAndAddAll("type", newList(field), newList(invalidAlias)); - - MockFieldMapper invalidField = new MockFieldMapper("invalid"); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> lookup.copyAndAddAll("type", newList(invalidField), emptyList())); - assertEquals("The name for field [invalid] has already been used to define a field alias.", - e.getMessage()); - } - public void testSimpleMatchToFullName() { MockFieldMapper field1 = new MockFieldMapper("foo"); MockFieldMapper field2 = new MockFieldMapper("bar"); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java index 353b2ccdd1c..312c79a63c3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java @@ -21,6 +21,7 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -31,18 +32,128 @@ import static java.util.Collections.singletonList; public class MapperMergeValidatorTests extends ESTestCase { + public void testMismatchedFieldTypes() { + FieldMapper existingField = new MockFieldMapper("foo"); + FieldTypeLookup lookup = new FieldTypeLookup() + .copyAndAddAll("type", singletonList(existingField), emptyList()); + + FieldTypeLookupTests.OtherFakeFieldType newFieldType = new FieldTypeLookupTests.OtherFakeFieldType(); + newFieldType.setName("foo"); + FieldMapper invalidField = new MockFieldMapper("foo", newFieldType); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateNewMappers( + emptyList(), + singletonList(invalidField), + emptyList(), + lookup)); + assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]")); + } + + public void testConflictingFieldTypes() { + FieldMapper existingField = new MockFieldMapper("foo"); + FieldTypeLookup lookup = new FieldTypeLookup() + .copyAndAddAll("type", singletonList(existingField), emptyList()); + + MappedFieldType newFieldType = new MockFieldMapper.FakeFieldType(); + newFieldType.setName("foo"); + newFieldType.setBoost(2.0f); + FieldMapper validField = new MockFieldMapper("foo", newFieldType); + + // Boost is updateable, so no exception should be thrown. + MapperMergeValidator.validateNewMappers( + emptyList(), + singletonList(validField), + emptyList(), + lookup); + + MappedFieldType invalidFieldType = new MockFieldMapper.FakeFieldType(); + invalidFieldType.setName("foo"); + invalidFieldType.setStored(true); + FieldMapper invalidField = new MockFieldMapper("foo", invalidFieldType); + + // Store is not updateable, so we expect an exception. + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateNewMappers( + emptyList(), + singletonList(invalidField), + emptyList(), + lookup)); + assertTrue(e.getMessage().contains("has different [store] values")); + } + public void testDuplicateFieldAliasAndObject() { ObjectMapper objectMapper = createObjectMapper("some.path"); FieldAliasMapper aliasMapper = new FieldAliasMapper("path", "some.path", "field"); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> - MapperMergeValidator.validateMapperStructure( + MapperMergeValidator.validateNewMappers( singletonList(objectMapper), emptyList(), - singletonList(aliasMapper))); + singletonList(aliasMapper), + new FieldTypeLookup())); assertEquals("Field [some.path] is defined both as an object and a field.", e.getMessage()); } + public void testDuplicateFieldAliasAndConcreteField() { + FieldMapper field = new MockFieldMapper("field"); + FieldMapper invalidField = new MockFieldMapper("invalid"); + FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid", "invalid", "field"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateNewMappers( + emptyList(), + Arrays.asList(field, invalidField), + singletonList(invalidAlias), + new FieldTypeLookup())); + + assertEquals("Field [invalid] is defined both as an alias and a concrete field.", e.getMessage()); + } + + public void testAliasThatRefersToAlias() { + FieldMapper field = new MockFieldMapper("field"); + FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "field"); + FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "alias"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateNewMappers( + emptyList(), + singletonList(field), + Arrays.asList(alias, invalidAlias), + new FieldTypeLookup())); + + assertEquals("Invalid [path] value [alias] for field alias [invalid-alias]: an alias" + + " cannot refer to another alias.", e.getMessage()); + } + + public void testAliasThatRefersToItself() { + FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "invalid-alias"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateNewMappers( + emptyList(), + emptyList(), + singletonList(invalidAlias), + new FieldTypeLookup())); + + assertEquals("Invalid [path] value [invalid-alias] for field alias [invalid-alias]: an alias" + + " cannot refer to itself.", e.getMessage()); + } + + public void testAliasWithNonExistentPath() { + FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "non-existent"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateNewMappers( + emptyList(), + emptyList(), + singletonList(invalidAlias), + new FieldTypeLookup())); + + assertEquals("Invalid [path] value [non-existent] for field alias [invalid-alias]: an alias" + + " must refer to an existing field in the mappings.", e.getMessage()); + } + public void testFieldAliasWithNestedScope() { ObjectMapper objectMapper = createNestedObjectMapper("nested"); FieldAliasMapper aliasMapper = new FieldAliasMapper("alias", "nested.alias", "nested.field");