From be9c37fc76b6d38e4edcad652d2752355c2b1e75 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Thu, 7 Mar 2019 14:11:27 -0800 Subject: [PATCH] Small simplifications to mapping validation. (#39777) These simplifications to `MapperMergeValidator` are possible now that there is always a single mapping definition. * Remove the type argument in `validateMapperStructure`. * Remove unnecessary checks against existing mappers. --- .../mapper/ParentJoinFieldMapperTests.java | 4 +- .../index/mapper/MapperMergeValidator.java | 56 ++----------------- .../index/mapper/MapperService.java | 3 +- .../mapper/MapperMergeValidatorTests.java | 8 +-- .../index/mapper/TextFieldMapperTests.java | 2 +- .../index/mapper/UpdateMappingTests.java | 4 +- 6 files changed, 14 insertions(+), 63 deletions(-) diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java index 6653117c62a..7fb4a18c66b 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java @@ -400,7 +400,7 @@ public class ParentJoinFieldMapperTests extends ESSingleNodeTestCase { .endObject()); IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type", new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE)); - assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice in [type]")); + assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice.")); } { @@ -426,7 +426,7 @@ public class ParentJoinFieldMapperTests extends ESSingleNodeTestCase { .endObject()); IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE)); - assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice in [type]")); + assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice.")); } } 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 440be98ad9e..3ca70f4c999 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -37,37 +37,18 @@ class MapperMergeValidator { * duplicate fields are present, and if the provided fields have already been * defined with a different data type. * - * @param type The mapping type, for use in error messages. * @param objectMappers The newly added object mappers. * @param fieldMappers The newly added field mappers. * @param fieldAliasMappers The newly added field alias mappers. - * @param fullPathObjectMappers All object mappers, indexed by their full path. - * @param fieldTypes All field and field alias mappers, collected into a lookup structure. */ - public static void validateMapperStructure(String type, - Collection objectMappers, + public static void validateMapperStructure(Collection objectMappers, Collection fieldMappers, - Collection fieldAliasMappers, - Map fullPathObjectMappers, - FieldTypeLookup fieldTypes) { - checkFieldUniqueness(type, objectMappers, fieldMappers, - fieldAliasMappers, fullPathObjectMappers, fieldTypes); - checkObjectsCompatibility(objectMappers, fullPathObjectMappers); - } - - private static void checkFieldUniqueness(String type, - Collection objectMappers, - Collection fieldMappers, - Collection fieldAliasMappers, - Map fullPathObjectMappers, - FieldTypeLookup fieldTypes) { - - // first check within mapping + Collection fieldAliasMappers) { Set objectFullNames = new HashSet<>(); for (ObjectMapper objectMapper : objectMappers) { String fullPath = objectMapper.fullPath(); if (objectFullNames.add(fullPath) == false) { - throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]"); + throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice."); } } @@ -76,38 +57,11 @@ class MapperMergeValidator { .forEach(mapper -> { String name = mapper.name(); if (objectFullNames.contains(name)) { - throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]"); + 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 in [" + type + "]"); + throw new IllegalArgumentException("Field [" + name + "] is defined twice."); } }); - - // then check other types - for (String fieldName : fieldNames) { - if (fullPathObjectMappers.containsKey(fieldName)) { - throw new IllegalArgumentException("[" + fieldName + "] is defined as a field in mapping [" + type - + "] but this name is already used for an object in other types"); - } - } - - for (String objectPath : objectFullNames) { - if (fieldTypes.get(objectPath) != null) { - throw new IllegalArgumentException("[" + objectPath + "] is defined as an object in mapping [" + type - + "] but this name is already used for a field in other types"); - } - } - } - - private static void checkObjectsCompatibility(Collection objectMappers, - Map fullPathObjectMappers) { - for (ObjectMapper newObjectMapper : objectMappers) { - ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath()); - if (existingObjectMapper != null) { - // simulate a merge and ignore the result, we are just interested - // in exceptions here - existingObjectMapper.merge(newObjectMapper); - } - } } /** 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 398ce4cdd17..0716b5a8c28 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -471,8 +471,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable { Collections.addAll(fieldMappers, metadataMappers); MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers); - MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers, - fieldAliasMappers, fullPathObjectMappers, fieldTypes); + MapperMergeValidator.validateMapperStructure(objectMappers, fieldMappers, fieldAliasMappers); checkPartitionedIndexConstraints(newMapper); // update lookup data-structures 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 af17918baac..353b2ccdd1c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java @@ -36,13 +36,11 @@ public class MapperMergeValidatorTests extends ESTestCase { FieldAliasMapper aliasMapper = new FieldAliasMapper("path", "some.path", "field"); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> - MapperMergeValidator.validateMapperStructure("type", + MapperMergeValidator.validateMapperStructure( singletonList(objectMapper), emptyList(), - singletonList(aliasMapper), - emptyMap(), - new FieldTypeLookup())); - assertEquals("Field [some.path] is defined both as an object and a field in [type]", e.getMessage()); + singletonList(aliasMapper))); + assertEquals("Field [some.path] is defined both as an object and a field.", e.getMessage()); } public void testFieldAliasWithNestedScope() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java index e527f98f73c..7314ecb1de7 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java @@ -880,7 +880,7 @@ public class TextFieldMapperTests extends ESSingleNodeTestCase { IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type", new CompressedXContent(illegalMapping), MergeReason.MAPPING_UPDATE)); - assertThat(e.getMessage(), containsString("Field [field._index_prefix] is defined twice in [type]")); + assertThat(e.getMessage(), containsString("Field [field._index_prefix] is defined twice.")); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java index 84f0dc31093..bd6a373f3cd 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java @@ -151,14 +151,14 @@ public class UpdateMappingTests extends ESSingleNodeTestCase { mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE); fail(); } catch (IllegalArgumentException e) { - assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]")); + assertTrue(e.getMessage().contains("Field [_id] is defined twice.")); } try { mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE); fail(); } catch (IllegalArgumentException e) { - assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]")); + assertTrue(e.getMessage().contains("Field [_id] is defined twice.")); } }