From 35f2fddadbfee83fffe1c7868265536cf2d00879 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 6 Mar 2019 11:09:12 -0800 Subject: [PATCH 1/2] Remove the type argument in validateMapperStructure. --- .../mapper/ParentJoinFieldMapperTests.java | 4 ++-- .../index/mapper/MapperMergeValidator.java | 23 ++++++++----------- .../index/mapper/MapperService.java | 2 +- .../mapper/MapperMergeValidatorTests.java | 4 ++-- .../index/mapper/TextFieldMapperTests.java | 2 +- .../index/mapper/UpdateMappingTests.java | 4 ++-- 6 files changed, 18 insertions(+), 21 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 6653117c62afb..7fb4a18c66b9a 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 void testMultipleJoinFields() throws Exception { .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 void testMultipleJoinFields() throws Exception { .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 440be98ad9ec9..023cddfe9b82c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -37,26 +37,23 @@ 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, + checkFieldUniqueness(objectMappers, fieldMappers, fieldAliasMappers, fullPathObjectMappers, fieldTypes); checkObjectsCompatibility(objectMappers, fullPathObjectMappers); } - private static void checkFieldUniqueness(String type, - Collection objectMappers, + private static void checkFieldUniqueness(Collection objectMappers, Collection fieldMappers, Collection fieldAliasMappers, Map fullPathObjectMappers, @@ -67,7 +64,7 @@ private static void checkFieldUniqueness(String type, 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,24 +73,24 @@ private static void checkFieldUniqueness(String type, .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"); + throw new IllegalArgumentException("[" + fieldName + "] is defined as a field but this name is " + + "already used for an object"); } } 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"); + throw new IllegalArgumentException("[" + objectPath + "] is defined as an object " + + "but this name is already used for a field in other types"); } } } 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 398ce4cdd17ce..d114b584266dd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -471,7 +471,7 @@ private synchronized Map internalMerge(@Nullable Documen Collections.addAll(fieldMappers, metadataMappers); MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers); - MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers, + MapperMergeValidator.validateMapperStructure(objectMappers, fieldMappers, fieldAliasMappers, fullPathObjectMappers, fieldTypes); checkPartitionedIndexConstraints(newMapper); 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 af17918baacb9..d93bb88bbb763 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,13 @@ public void testDuplicateFieldAliasAndObject() { 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()); + 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 e527f98f73c20..7314ecb1de7c2 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 void testIndexPrefixMapping() throws IOException { 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 84f0dc31093a6..bd6a373f3cd9a 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 void testReuseMetaField() throws IOException { 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.")); } } From 7b7e58d1c0e5a0da22b8212f0ce658cc65e6a59f Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 6 Mar 2019 13:50:46 -0800 Subject: [PATCH 2/2] Remove unnecessary checks now that there is a single mapping definition. --- .../index/mapper/MapperMergeValidator.java | 45 +------------------ .../index/mapper/MapperService.java | 3 +- .../mapper/MapperMergeValidatorTests.java | 4 +- 3 files changed, 3 insertions(+), 49 deletions(-) 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 023cddfe9b82c..3ca70f4c99996 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -40,26 +40,10 @@ class MapperMergeValidator { * @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(Collection objectMappers, Collection fieldMappers, - Collection fieldAliasMappers, - Map fullPathObjectMappers, - FieldTypeLookup fieldTypes) { - checkFieldUniqueness(objectMappers, fieldMappers, - fieldAliasMappers, fullPathObjectMappers, fieldTypes); - checkObjectsCompatibility(objectMappers, fullPathObjectMappers); - } - - private static void checkFieldUniqueness(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(); @@ -78,33 +62,6 @@ private static void checkFieldUniqueness(Collection objectMappers, 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 but this name is " + - "already used for an object"); - } - } - - for (String objectPath : objectFullNames) { - if (fieldTypes.get(objectPath) != null) { - throw new IllegalArgumentException("[" + objectPath + "] is defined as an object " + - "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 d114b584266dd..0716b5a8c280b 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 @@ private synchronized Map internalMerge(@Nullable Documen Collections.addAll(fieldMappers, metadataMappers); MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers); - MapperMergeValidator.validateMapperStructure(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 d93bb88bbb763..353b2ccdd1cda 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java @@ -39,9 +39,7 @@ public void testDuplicateFieldAliasAndObject() { MapperMergeValidator.validateMapperStructure( singletonList(objectMapper), emptyList(), - singletonList(aliasMapper), - emptyMap(), - new FieldTypeLookup())); + singletonList(aliasMapper))); assertEquals("Field [some.path] is defined both as an object and a field.", e.getMessage()); }