From a5a66ff8bb128e43ac5cb819ce6bcab582a40789 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 20 Jun 2018 15:26:50 -0700 Subject: [PATCH 1/5] Perform basic validation on the target of an alias. --- .../index/mapper/FieldTypeLookup.java | 34 ++++++++++++--- .../index/mapper/FieldAliasMapperTests.java | 3 ++ .../index/mapper/FieldTypeLookupTests.java | 41 +++++++++++++++++-- 3 files changed, 68 insertions(+), 10 deletions(-) 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 a7d0177224d45..dc2ac4af87626 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -75,7 +75,7 @@ public FieldTypeLookup copyAndAddAll(String type, fullName = fullName.copyAndPut(fieldType.name(), fieldMapper.fieldType()); } else { // modification of an existing field - checkCompatibility(fullNameFieldType, fieldType); + validateFieldType(fullNameFieldType, fieldType); if (fieldType.equals(fullNameFieldType) == false) { fullName = fullName.copyAndPut(fieldType.name(), fieldMapper.fieldType()); } @@ -84,8 +84,10 @@ public FieldTypeLookup copyAndAddAll(String type, for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) { String aliasName = fieldAliasMapper.name(); - String fieldName = fieldAliasMapper.path(); - aliases = aliases.copyAndPut(aliasName, fieldName); + String path = fieldAliasMapper.path(); + + validateAlias(aliasName, path, aliases, fullName); + aliases = aliases.copyAndPut(aliasName, path); } return new FieldTypeLookup(fullName, aliases); @@ -95,7 +97,7 @@ public FieldTypeLookup copyAndAddAll(String type, * Checks if the given field type is compatible with an existing field type. * An IllegalArgumentException is thrown in case of incompatibility. */ - private void checkCompatibility(MappedFieldType existingFieldType, MappedFieldType newFieldType) { + private void validateFieldType(MappedFieldType existingFieldType, MappedFieldType newFieldType) { List conflicts = new ArrayList<>(); existingFieldType.checkCompatibility(newFieldType, conflicts); if (conflicts.isEmpty() == false) { @@ -103,10 +105,30 @@ private void checkCompatibility(MappedFieldType existingFieldType, MappedFieldTy } } + private void validateAlias(String aliasName, + String path, + CopyOnWriteHashMap aliasToConcreteName, + CopyOnWriteHashMap fullNameToFieldType) { + 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) { - String resolvedField = aliasToConcreteName.getOrDefault(field, field); - return fullNameToFieldType.get(resolvedField); + String concreteField = aliasToConcreteName.getOrDefault(field, field); + return fullNameToFieldType.get(concreteField); } /** diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java index 889bcfa81cde5..9f87ad3d0390a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java @@ -138,6 +138,9 @@ public void testMergeFailure() throws IOException { String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject() .startObject("type") .startObject("properties") + .startObject("concrete-field") + .field("type", "text") + .endObject() .startObject("alias-field") .field("type", "alias") .field("path", "concrete-field") 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 8a49169e09e79..3296d5b59633d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -79,7 +79,7 @@ public void testAddExistingField() { assertEquals(f2.fieldType(), lookup2.get("foo")); } - public void testCheckCompatibilityMismatchedTypes() { + public void testMismatchedFieldTypes() { FieldMapper f1 = new MockFieldMapper("foo"); FieldTypeLookup lookup = new FieldTypeLookup(); lookup = lookup.copyAndAddAll("type", newList(f1), emptyList()); @@ -95,7 +95,7 @@ public void testCheckCompatibilityMismatchedTypes() { } } - public void testCheckCompatibilityConflict() { + public void testConflictingFieldTypes() { FieldMapper f1 = new MockFieldMapper("foo"); FieldTypeLookup lookup = new FieldTypeLookup(); lookup = lookup.copyAndAddAll("type", newList(f1), emptyList()); @@ -181,12 +181,45 @@ public void testUpdateConcreteFieldWithAlias() { 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 testSimpleMatchToFullName() { MockFieldMapper field1 = new MockFieldMapper("foo"); MockFieldMapper field2 = new MockFieldMapper("bar"); - FieldAliasMapper alias1 = new FieldAliasMapper("food", "food", "path"); - FieldAliasMapper alias2 = new FieldAliasMapper("barometer", "barometer", "other-path"); + FieldAliasMapper alias1 = new FieldAliasMapper("food", "food", "foo"); + FieldAliasMapper alias2 = new FieldAliasMapper("barometer", "barometer", "bar"); FieldTypeLookup lookup = new FieldTypeLookup(); lookup = lookup.copyAndAddAll("type", From 0a3a4cd67f10cb81499bec67fc1467ed5f90274d Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 20 Jun 2018 18:13:48 -0700 Subject: [PATCH 2/5] Pull out some validation logic into a separate class. --- .../index/mapper/MapperMergeValidator.java | 151 ++++++++++++++++++ .../index/mapper/MapperService.java | 115 +------------ 2 files changed, 153 insertions(+), 113 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java new file mode 100644 index 0000000000000..17d8145d3c078 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -0,0 +1,151 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.index.mapper; + +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +public class MapperMergeValidator { + public static void validateMapperStructure(String type, + Collection objectMappers, + Collection fieldMappers, + Map fullPathObjectMappers, + FieldTypeLookup fieldTypes) { + checkFieldUniqueness(type, objectMappers, fieldMappers, fullPathObjectMappers, fieldTypes); + checkObjectsCompatibility(objectMappers, fullPathObjectMappers); + } + + private static void checkFieldUniqueness(String type, + Collection objectMappers, + Collection fieldMappers, + Map fullPathObjectMappers, + FieldTypeLookup fieldTypes) { + + // first check within mapping + final Set objectFullNames = new HashSet<>(); + for (ObjectMapper objectMapper : objectMappers) { + final String fullPath = objectMapper.fullPath(); + if (objectFullNames.add(fullPath) == false) { + throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]"); + } + } + + final Set fieldNames = new HashSet<>(); + for (FieldMapper fieldMapper : fieldMappers) { + final String name = fieldMapper.name(); + if (objectFullNames.contains(name)) { + throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]"); + } else if (fieldNames.add(name) == false) { + throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]"); + } + } + + // 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); + } + } + } + + public static void validateCopyTo(List fieldMappers, + Map fullPathObjectMappers, + FieldTypeLookup fieldTypes) { + for (FieldMapper mapper : fieldMappers) { + if (mapper.copyTo() != null && mapper.copyTo().copyToFields().isEmpty() == false) { + String sourceParent = parentObject(mapper.name()); + if (sourceParent != null && fieldTypes.get(sourceParent) != null) { + throw new IllegalArgumentException("[copy_to] may not be used to copy from a multi-field: [" + mapper.name() + "]"); + } + + final String sourceScope = getNestedScope(mapper.name(), fullPathObjectMappers); + for (String copyTo : mapper.copyTo().copyToFields()) { + String copyToParent = parentObject(copyTo); + if (copyToParent != null && fieldTypes.get(copyToParent) != null) { + throw new IllegalArgumentException("[copy_to] may not be used to copy to a multi-field: [" + copyTo + "]"); + } + + if (fullPathObjectMappers.containsKey(copyTo)) { + throw new IllegalArgumentException("Cannot copy to field [" + copyTo + "] since it is mapped as an object"); + } + + final String targetScope = getNestedScope(copyTo, fullPathObjectMappers); + checkNestedScopeCompatibility(sourceScope, targetScope); + } + } + } + } + + private static String getNestedScope(String path, Map fullPathObjectMappers) { + for (String parentPath = parentObject(path); parentPath != null; parentPath = parentObject(parentPath)) { + ObjectMapper objectMapper = fullPathObjectMappers.get(parentPath); + if (objectMapper != null && objectMapper.nested().isNested()) { + return parentPath; + } + } + return null; + } + + private static void checkNestedScopeCompatibility(String source, String target) { + boolean targetIsParentOfSource; + if (source == null || target == null) { + targetIsParentOfSource = target == null; + } else { + targetIsParentOfSource = source.equals(target) || source.startsWith(target + "."); + } + if (targetIsParentOfSource == false) { + throw new IllegalArgumentException( + "Illegal combination of [copy_to] and [nested] mappings: [copy_to] may only copy data to the current nested " + + "document or any of its parents, however one [copy_to] directive is trying to copy data from nested object [" + + source + "] to [" + target + "]"); + } + } + + private static String parentObject(String field) { + int lastDot = field.lastIndexOf('.'); + if (lastDot == -1) { + return null; + } + return field.substring(0, lastDot); + } +} 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 b99cae2e3b6d7..0b1384dbeb9d2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -397,8 +397,7 @@ private synchronized Map internalMerge(@Nullable Documen List fieldAliasMappers = new ArrayList<>(); Collections.addAll(fieldMappers, newMapper.mapping().metadataMappers); MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers); - checkFieldUniqueness(newMapper.type(), objectMappers, fieldMappers, fullPathObjectMappers, fieldTypes); - checkObjectsCompatibility(objectMappers, fullPathObjectMappers); + MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers, fullPathObjectMappers, fieldTypes); checkPartitionedIndexConstraints(newMapper); // update lookup data-structures @@ -417,7 +416,7 @@ private synchronized Map internalMerge(@Nullable Documen } } - validateCopyTo(fieldMappers, fullPathObjectMappers, fieldTypes); + MapperMergeValidator.validateCopyTo(fieldMappers, fullPathObjectMappers, fieldTypes); if (reason == MergeReason.MAPPING_UPDATE) { // this check will only be performed on the master node when there is @@ -503,56 +502,6 @@ private boolean assertSerialization(DocumentMapper mapper) { return true; } - private static void checkFieldUniqueness(String type, Collection objectMappers, Collection fieldMappers, - Map fullPathObjectMappers, FieldTypeLookup fieldTypes) { - - // first check within mapping - final Set objectFullNames = new HashSet<>(); - for (ObjectMapper objectMapper : objectMappers) { - final String fullPath = objectMapper.fullPath(); - if (objectFullNames.add(fullPath) == false) { - throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]"); - } - } - - final Set fieldNames = new HashSet<>(); - for (FieldMapper fieldMapper : fieldMappers) { - final String name = fieldMapper.name(); - if (objectFullNames.contains(name)) { - throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]"); - } else if (fieldNames.add(name) == false) { - throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]"); - } - } - - // 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); - } - } - } - private void checkNestedFieldsLimit(Map fullPathObjectMappers) { long allowedNestedFields = indexSettings.getValue(INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING); long actualNestedFields = 0; @@ -609,66 +558,6 @@ private static void checkIndexSortCompatibility(IndexSortConfig sortConfig, bool } } - private static void validateCopyTo(List fieldMappers, Map fullPathObjectMappers, - FieldTypeLookup fieldTypes) { - for (FieldMapper mapper : fieldMappers) { - if (mapper.copyTo() != null && mapper.copyTo().copyToFields().isEmpty() == false) { - String sourceParent = parentObject(mapper.name()); - if (sourceParent != null && fieldTypes.get(sourceParent) != null) { - throw new IllegalArgumentException("[copy_to] may not be used to copy from a multi-field: [" + mapper.name() + "]"); - } - - final String sourceScope = getNestedScope(mapper.name(), fullPathObjectMappers); - for (String copyTo : mapper.copyTo().copyToFields()) { - String copyToParent = parentObject(copyTo); - if (copyToParent != null && fieldTypes.get(copyToParent) != null) { - throw new IllegalArgumentException("[copy_to] may not be used to copy to a multi-field: [" + copyTo + "]"); - } - - if (fullPathObjectMappers.containsKey(copyTo)) { - throw new IllegalArgumentException("Cannot copy to field [" + copyTo + "] since it is mapped as an object"); - } - - final String targetScope = getNestedScope(copyTo, fullPathObjectMappers); - checkNestedScopeCompatibility(sourceScope, targetScope); - } - } - } - } - - private static String getNestedScope(String path, Map fullPathObjectMappers) { - for (String parentPath = parentObject(path); parentPath != null; parentPath = parentObject(parentPath)) { - ObjectMapper objectMapper = fullPathObjectMappers.get(parentPath); - if (objectMapper != null && objectMapper.nested().isNested()) { - return parentPath; - } - } - return null; - } - - private static void checkNestedScopeCompatibility(String source, String target) { - boolean targetIsParentOfSource; - if (source == null || target == null) { - targetIsParentOfSource = target == null; - } else { - targetIsParentOfSource = source.equals(target) || source.startsWith(target + "."); - } - if (targetIsParentOfSource == false) { - throw new IllegalArgumentException( - "Illegal combination of [copy_to] and [nested] mappings: [copy_to] may only copy data to the current nested " + - "document or any of its parents, however one [copy_to] directive is trying to copy data from nested object [" + - source + "] to [" + target + "]"); - } - } - - private static String parentObject(String field) { - int lastDot = field.lastIndexOf('.'); - if (lastDot == -1) { - return null; - } - return field.substring(0, lastDot); - } - public DocumentMapper parse(String mappingType, CompressedXContent mappingSource, boolean applyDefault) throws MapperParsingException { return documentParser.parse(mappingType, mappingSource, applyDefault ? defaultMappingSource : null); } From d2ebbbee2c76e1216d98a5787cb646eca1679295 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 20 Jun 2018 15:56:33 -0700 Subject: [PATCH 3/5] Validate that an alias and its target have the same nested scope. --- .../index/mapper/MapperMergeValidator.java | 34 ++++++- .../index/mapper/MapperService.java | 3 +- .../mapper/MapperMergeValidatorTests.java | 97 +++++++++++++++++++ .../index/mapper/MapperServiceTests.java | 36 +++++++ 4 files changed, 166 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java 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 17d8145d3c078..730a698441bbc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -23,6 +23,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; public class MapperMergeValidator { @@ -88,9 +89,21 @@ private static void checkObjectsCompatibility(Collection objectMap } } - public static void validateCopyTo(List fieldMappers, - Map fullPathObjectMappers, - FieldTypeLookup fieldTypes) { + /** + * 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. + */ + public static void validateFieldReferences(List fieldMappers, + List fieldAliasMappers, + Map fullPathObjectMappers, + FieldTypeLookup fieldTypes) { + validateCopyTo(fieldMappers, fullPathObjectMappers, fieldTypes); + validateFieldAliasTargets(fieldAliasMappers, fullPathObjectMappers); + } + + private static void validateCopyTo(List fieldMappers, + Map fullPathObjectMappers, + FieldTypeLookup fieldTypes) { for (FieldMapper mapper : fieldMappers) { if (mapper.copyTo() != null && mapper.copyTo().copyToFields().isEmpty() == false) { String sourceParent = parentObject(mapper.name()); @@ -116,6 +129,21 @@ public static void validateCopyTo(List fieldMappers, } } + private static void validateFieldAliasTargets(List fieldAliasMappers, + Map fullPathObjectMappers) { + for (FieldAliasMapper mapper : fieldAliasMappers) { + String aliasName = mapper.name(); + String path = mapper.path(); + + String aliasScope = getNestedScope(aliasName, fullPathObjectMappers); + String pathScope = getNestedScope(path, fullPathObjectMappers); + if (!Objects.equals(aliasScope, pathScope)) { + throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + + aliasName + "]: an alias must have the same nested scope as its target."); + } + } + } + private static String getNestedScope(String path, Map fullPathObjectMappers) { for (String parentPath = parentObject(path); parentPath != null; parentPath = parentObject(parentPath)) { ObjectMapper objectMapper = fullPathObjectMappers.get(parentPath); 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 0b1384dbeb9d2..61f61f1511606 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -416,7 +416,8 @@ private synchronized Map internalMerge(@Nullable Documen } } - MapperMergeValidator.validateCopyTo(fieldMappers, fullPathObjectMappers, fieldTypes); + MapperMergeValidator.validateFieldReferences(fieldMappers, fieldAliasMappers, + fullPathObjectMappers, fieldTypes); if (reason == MergeReason.MAPPING_UPDATE) { // this check will only be performed on the master node when there is diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java new file mode 100644 index 0000000000000..7123758d64881 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java @@ -0,0 +1,97 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.index.mapper; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.test.ESTestCase; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; + +public class MapperMergeValidatorTests extends ESTestCase { + + public void testFieldAliasWithNestedScope() { + ObjectMapper objectMapper = createNestedObjectMapper("nested"); + FieldAliasMapper aliasMapper = new FieldAliasMapper("alias", "nested.alias", "nested.field"); + + MapperMergeValidator.validateFieldReferences(emptyList(), + singletonList(aliasMapper), + Collections.singletonMap("nested", objectMapper), + new FieldTypeLookup()); + } + + public void testFieldAliasWithDifferentObjectScopes() { + Map fullPathObjectMappers = new HashMap<>(); + fullPathObjectMappers.put("object1", createObjectMapper("object1")); + fullPathObjectMappers.put("object2", createObjectMapper("object2")); + + FieldAliasMapper aliasMapper = new FieldAliasMapper("alias", "object2.alias", "object1.field"); + + MapperMergeValidator.validateFieldReferences(emptyList(), + singletonList(aliasMapper), + fullPathObjectMappers, + new FieldTypeLookup()); + } + + public void testFieldAliasWithNestedTarget() { + ObjectMapper objectMapper = createNestedObjectMapper("nested"); + FieldAliasMapper aliasMapper = new FieldAliasMapper("alias", "alias", "nested.field"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateFieldReferences(emptyList(), + singletonList(aliasMapper), + Collections.singletonMap("nested", objectMapper), + new FieldTypeLookup())); + assertEquals("Invalid [path] value [nested.field] for field alias [alias]: " + + "an alias must have the same nested scope as its target.", e.getMessage()); + } + + public void testFieldAliasWithDifferentNestedScopes() { + Map fullPathObjectMappers = new HashMap<>(); + fullPathObjectMappers.put("nested1", createNestedObjectMapper("nested1")); + fullPathObjectMappers.put("nested2", createNestedObjectMapper("nested2")); + + FieldAliasMapper aliasMapper = new FieldAliasMapper("alias", "nested2.alias", "nested1.field"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateFieldReferences(emptyList(), + singletonList(aliasMapper), + fullPathObjectMappers, + new FieldTypeLookup())); + assertEquals("Invalid [path] value [nested1.field] for field alias [nested2.alias]: " + + "an alias must have the same nested scope as its target.", e.getMessage()); + } + + private static ObjectMapper createObjectMapper(String name) { + return new ObjectMapper(name, name, true, + ObjectMapper.Nested.NO, + ObjectMapper.Dynamic.FALSE, emptyMap(), Settings.EMPTY); + } + + private static ObjectMapper createNestedObjectMapper(String name) { + return new ObjectMapper(name, name, true, + ObjectMapper.Nested.newNested(false, false), + ObjectMapper.Dynamic.FALSE, emptyMap(), Settings.EMPTY); + } +} diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 6bccb7106f656..aa8e72d121dc3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -235,6 +235,42 @@ public void testIndexSortWithNestedFields() throws IOException { containsString("cannot have nested fields when index sort is activated")); } + public void testFieldAliasWithMismatchedNestedScope() throws Throwable { + IndexService indexService = createIndex("test"); + MapperService mapperService = indexService.mapperService(); + + CompressedXContent mapping = new CompressedXContent(BytesReference.bytes( + XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("nested") + .field("type", "nested") + .startObject("properties") + .startObject("field") + .field("type", "text") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject())); + + mapperService.merge("type", mapping, MergeReason.MAPPING_UPDATE); + + CompressedXContent mappingUpdate = new CompressedXContent(BytesReference.bytes( + XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("alias") + .field("type", "alias") + .field("path", "nested.field") + .endObject() + .endObject() + .endObject())); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> mapperService.merge("type", mappingUpdate, MergeReason.MAPPING_UPDATE)); + assertEquals("Invalid [path] value [nested.field] for field alias [alias]: an alias" + + " must have the same nested scope as its target.", e.getMessage()); + } + public void testForbidMultipleTypes() throws IOException { String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type").endObject().endObject()); MapperService mapperService = createIndex("test").mapperService(); From ec765598511a4308711866f85dbd8b7e7d089336 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 20 Jun 2018 20:55:12 -0700 Subject: [PATCH 4/5] Validate the uniqueness of field alias mappings. --- .../index/mapper/FieldTypeLookup.java | 42 +++++++++++-------- .../index/mapper/MapperMergeValidator.java | 29 +++++++------ .../index/mapper/MapperService.java | 4 +- .../index/mapper/FieldTypeLookupTests.java | 29 +++++++++++++ .../mapper/MapperMergeValidatorTests.java | 14 +++++++ 5 files changed, 87 insertions(+), 31 deletions(-) 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 dc2ac4af87626..7b61b979409de 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -70,15 +70,9 @@ public FieldTypeLookup copyAndAddAll(String type, MappedFieldType fieldType = fieldMapper.fieldType(); MappedFieldType fullNameFieldType = fullName.get(fieldType.name()); - if (fullNameFieldType == null) { - // introduction of a new field - fullName = fullName.copyAndPut(fieldType.name(), fieldMapper.fieldType()); - } else { - // modification of an existing field - validateFieldType(fullNameFieldType, fieldType); - if (fieldType.equals(fullNameFieldType) == false) { - fullName = fullName.copyAndPut(fieldType.name(), fieldMapper.fieldType()); - } + if (!Objects.equals(fieldType, fullNameFieldType)) { + validateField(fullNameFieldType, fieldType, aliases); + fullName = fullName.copyAndPut(fieldType.name(), fieldType); } } @@ -93,15 +87,22 @@ public FieldTypeLookup copyAndAddAll(String type, return new FieldTypeLookup(fullName, aliases); } - /** - * Checks if the given field type is compatible with an existing field type. - * An IllegalArgumentException is thrown in case of incompatibility. - */ - private void validateFieldType(MappedFieldType existingFieldType, MappedFieldType newFieldType) { - 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()); + 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()); + } } } @@ -109,6 +110,11 @@ 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."); 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 730a698441bbc..f02c76dd85bbf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -25,41 +25,46 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Stream; public class MapperMergeValidator { public static void validateMapperStructure(String type, Collection objectMappers, Collection fieldMappers, + Collection fieldAliasMappers, Map fullPathObjectMappers, FieldTypeLookup fieldTypes) { - checkFieldUniqueness(type, objectMappers, fieldMappers, fullPathObjectMappers, 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 - final Set objectFullNames = new HashSet<>(); + Set objectFullNames = new HashSet<>(); for (ObjectMapper objectMapper : objectMappers) { - final String fullPath = objectMapper.fullPath(); + String fullPath = objectMapper.fullPath(); if (objectFullNames.add(fullPath) == false) { throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]"); } } - final Set fieldNames = new HashSet<>(); - for (FieldMapper fieldMapper : fieldMappers) { - final String name = fieldMapper.name(); - if (objectFullNames.contains(name)) { - throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]"); - } else if (fieldNames.add(name) == false) { - throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]"); - } - } + 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 in [" + type + "]"); + } else if (fieldNames.add(name) == false) { + throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]"); + } + }); // then check other types for (String fieldName : fieldNames) { 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 61f61f1511606..0bfdab8015214 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -397,7 +397,9 @@ private synchronized Map internalMerge(@Nullable Documen List fieldAliasMappers = new ArrayList<>(); Collections.addAll(fieldMappers, newMapper.mapping().metadataMappers); MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers); - MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers, fullPathObjectMappers, fieldTypes); + + MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers, + fieldAliasMappers, fullPathObjectMappers, fieldTypes); checkPartitionedIndexConstraints(newMapper); // update lookup data-structures 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 3296d5b59633d..6e27823f8a0c0 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -214,6 +214,35 @@ public void testAliasWithNonExistentPath() { " 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 7123758d64881..cb7bdcb828c47 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java @@ -31,6 +31,20 @@ public class MapperMergeValidatorTests extends ESTestCase { + public void testDuplicateFieldAliasAndObject() { + ObjectMapper objectMapper = createObjectMapper("some.path"); + FieldAliasMapper aliasMapper = new FieldAliasMapper("path", "some.path", "field"); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + MapperMergeValidator.validateMapperStructure("type", + 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()); + } + public void testFieldAliasWithNestedScope() { ObjectMapper objectMapper = createNestedObjectMapper("nested"); FieldAliasMapper aliasMapper = new FieldAliasMapper("alias", "nested.alias", "nested.field"); From f34f57d66bdf1d7dfc5a57b9aa146118fd769dc6 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Mon, 25 Jun 2018 12:48:34 -0700 Subject: [PATCH 5/5] Address code review feedback. --- .../index/mapper/FieldTypeLookup.java | 9 +++++ .../index/mapper/MapperMergeValidator.java | 36 +++++++++++++++++-- .../mapper/MapperMergeValidatorTests.java | 15 +++++--- .../index/mapper/MapperServiceTests.java | 3 +- 4 files changed, 54 insertions(+), 9 deletions(-) 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 7b61b979409de..c7d92e9f829aa 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -87,6 +87,9 @@ public FieldTypeLookup copyAndAddAll(String type, return new FieldTypeLookup(fullName, aliases); } + /** + * Checks that the new field type is valid. + */ private void validateField(MappedFieldType existingFieldType, MappedFieldType newFieldType, CopyOnWriteHashMap aliasToConcreteName) { @@ -106,6 +109,12 @@ private void validateField(MappedFieldType existingFieldType, } } + /** + * 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, 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 f02c76dd85bbf..440be98ad9ec9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java @@ -27,7 +27,23 @@ import java.util.Set; import java.util.stream.Stream; -public class MapperMergeValidator { +/** + * A utility class that helps validate certain aspects of a mappings update. + */ +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. + * + * @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, Collection fieldMappers, @@ -97,6 +113,11 @@ private static void checkObjectsCompatibility(Collection objectMap /** * 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. + * + * @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 validateFieldReferences(List fieldMappers, List fieldAliasMappers, @@ -142,9 +163,18 @@ private static void validateFieldAliasTargets(List fieldAliasM String aliasScope = getNestedScope(aliasName, fullPathObjectMappers); String pathScope = getNestedScope(path, fullPathObjectMappers); + if (!Objects.equals(aliasScope, pathScope)) { - throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + - aliasName + "]: an alias must have the same nested scope as its target."); + StringBuilder message = new StringBuilder("Invalid [path] value [" + path + "] for field alias [" + + aliasName + "]: an alias must have the same nested scope as its target. "); + message.append(aliasScope == null + ? "The alias is not nested" + : "The alias's nested scope is [" + aliasScope + "]"); + message.append(", but "); + message.append(pathScope == null + ? "the target is not nested." + : "the target's nested scope is [" + pathScope + "]."); + throw new IllegalArgumentException(message.toString()); } } } 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 cb7bdcb828c47..af17918baacb9 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java @@ -77,8 +77,11 @@ public void testFieldAliasWithNestedTarget() { singletonList(aliasMapper), Collections.singletonMap("nested", objectMapper), new FieldTypeLookup())); - assertEquals("Invalid [path] value [nested.field] for field alias [alias]: " + - "an alias must have the same nested scope as its target.", e.getMessage()); + + String expectedMessage = "Invalid [path] value [nested.field] for field alias [alias]: " + + "an alias must have the same nested scope as its target. The alias is not nested, " + + "but the target's nested scope is [nested]."; + assertEquals(expectedMessage, e.getMessage()); } public void testFieldAliasWithDifferentNestedScopes() { @@ -93,8 +96,12 @@ public void testFieldAliasWithDifferentNestedScopes() { singletonList(aliasMapper), fullPathObjectMappers, new FieldTypeLookup())); - assertEquals("Invalid [path] value [nested1.field] for field alias [nested2.alias]: " + - "an alias must have the same nested scope as its target.", e.getMessage()); + + + String expectedMessage = "Invalid [path] value [nested1.field] for field alias [nested2.alias]: " + + "an alias must have the same nested scope as its target. The alias's nested scope is [nested2], " + + "but the target's nested scope is [nested1]."; + assertEquals(expectedMessage, e.getMessage()); } private static ObjectMapper createObjectMapper(String name) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index aa8e72d121dc3..20e0dd4639c3a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -267,8 +267,7 @@ public void testFieldAliasWithMismatchedNestedScope() throws Throwable { IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> mapperService.merge("type", mappingUpdate, MergeReason.MAPPING_UPDATE)); - assertEquals("Invalid [path] value [nested.field] for field alias [alias]: an alias" + - " must have the same nested scope as its target.", e.getMessage()); + assertThat(e.getMessage(), containsString("Invalid [path] value [nested.field] for field alias [alias]")); } public void testForbidMultipleTypes() throws IOException {