Skip to content

Commit

Permalink
Better validation of copy_to. (#25983)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jpountz
jpountz authored Aug 1, 2017
1 parent 53c829b commit e9669b3
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 1 deletion.
54 changes: 53 additions & 1 deletion core/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ private synchronized Map<String, DocumentMapper> 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
Expand Down Expand Up @@ -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<FieldMapper> fieldMappers, Map<String, ObjectMapper> 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<String, ObjectMapper> 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);
}
Expand Down
107 changes: 107 additions & 0 deletions core/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")
Expand Down

0 comments on commit e9669b3

Please sign in to comment.