From e9669b3762a603c10f0fb415ad8fb684f9d7e323 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 1 Aug 2017 16:23:28 +0200 Subject: [PATCH] Better validation of `copy_to`. (#25983) We are currently quite lenient about the targets of `copy_to`. However in a number of cases we can detect illegal use of `copy_to` at mapping update time. For instance, it does not make sense to use object fields as targets of `copy_to`, or fields that would end up in a different nested document. --- .../index/mapper/MapperService.java | 54 ++++++++- .../index/mapper/CopyToMapperTests.java | 107 ++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java index c101c845bd13b..85cdc7243a221 100755 --- a/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -423,6 +423,10 @@ private synchronized Map internalMerge(@Nullable Documen } } + if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_6_0_0_beta1)) { + validateCopyTo(fieldMappers, fullPathObjectMappers); + } + if (reason == MergeReason.MAPPING_UPDATE) { // this check will only be performed on the master node when there is // a call to the update mapping API. For all other cases like @@ -648,12 +652,60 @@ private void checkPartitionedIndexConstraints(DocumentMapper newMapper) { } } - private void checkIndexSortCompatibility(IndexSortConfig sortConfig, boolean hasNested) { + private static void checkIndexSortCompatibility(IndexSortConfig sortConfig, boolean hasNested) { if (sortConfig.hasIndexSort() && hasNested) { throw new IllegalArgumentException("cannot have nested fields when index sort is activated"); } } + private static void validateCopyTo(List fieldMappers, Map fullPathObjectMappers) { + for (FieldMapper mapper : fieldMappers) { + if (mapper.copyTo() != null && mapper.copyTo().copyToFields().isEmpty() == false) { + final String sourceScope = getNestedScope(mapper.name(), fullPathObjectMappers); + for (String copyTo : mapper.copyTo().copyToFields()) { + 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.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); } diff --git a/core/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java index 4b2f629c36eea..a5ba66fd8c22d 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java @@ -29,8 +29,10 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.IndexService; +import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.ParseContext.Document; import org.elasticsearch.test.ESSingleNodeTestCase; +import org.hamcrest.Matchers; import java.util.Arrays; import java.util.List; @@ -414,6 +416,111 @@ public void testCopyToNestedField() throws Exception { assertFieldValue(root, "n1.n2.target"); } + public void testCopyToChildNested() throws Exception { + IndexService indexService = createIndex("test"); + XContentBuilder rootToNestedMapping = jsonBuilder().startObject() + .startObject("doc") + .startObject("properties") + .startObject("source") + .field("type", "long") + .field("copy_to", "n1.target") + .endObject() + .startObject("n1") + .field("type", "nested") + .startObject("properties") + .startObject("target") + .field("type", "long") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> indexService.mapperService().merge("doc", new CompressedXContent(rootToNestedMapping.bytes()), + MergeReason.MAPPING_UPDATE, false)); + assertThat(e.getMessage(), Matchers.startsWith("Illegal combination of [copy_to] and [nested] mappings")); + + XContentBuilder nestedToNestedMapping = jsonBuilder().startObject() + .startObject("doc") + .startObject("properties") + .startObject("n1") + .field("type", "nested") + .startObject("properties") + .startObject("source") + .field("type", "long") + .field("copy_to", "n1.n2.target") + .endObject() + .startObject("n2") + .field("type", "nested") + .startObject("properties") + .startObject("target") + .field("type", "long") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + e = expectThrows(IllegalArgumentException.class, + () -> indexService.mapperService().merge("doc", new CompressedXContent(nestedToNestedMapping.bytes()), + MergeReason.MAPPING_UPDATE, false)); + } + + public void testCopyToSiblingNested() throws Exception { + IndexService indexService = createIndex("test"); + XContentBuilder rootToNestedMapping = jsonBuilder().startObject() + .startObject("doc") + .startObject("properties") + .startObject("n1") + .field("type", "nested") + .startObject("properties") + .startObject("source") + .field("type", "long") + .field("copy_to", "n2.target") + .endObject() + .endObject() + .endObject() + .startObject("n2") + .field("type", "nested") + .startObject("properties") + .startObject("target") + .field("type", "long") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> indexService.mapperService().merge("doc", new CompressedXContent(rootToNestedMapping.bytes()), + MergeReason.MAPPING_UPDATE, false)); + assertThat(e.getMessage(), Matchers.startsWith("Illegal combination of [copy_to] and [nested] mappings")); + } + + public void testCopyToObject() throws Exception { + IndexService indexService = createIndex("test"); + XContentBuilder rootToNestedMapping = jsonBuilder().startObject() + .startObject("doc") + .startObject("properties") + .startObject("source") + .field("type", "long") + .field("copy_to", "target") + .endObject() + .startObject("target") + .field("type", "object") + .endObject() + .endObject() + .endObject() + .endObject(); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> indexService.mapperService().merge("doc", new CompressedXContent(rootToNestedMapping.bytes()), + MergeReason.MAPPING_UPDATE, false)); + assertThat(e.getMessage(), Matchers.startsWith("Cannot copy to field [target] since it is mapped as an object")); + } + public void testCopyToDynamicNestedObjectParsing() throws Exception { String mapping = jsonBuilder().startObject().startObject("type1") .startArray("dynamic_templates")