From f6d8b12796a1724cb3c323ccaf17e964af880ee5 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 22 Apr 2015 16:38:07 -0700 Subject: [PATCH] Mappings: Explicitly disallow multi fields from using object or nested fields Multi fields currently parse any field type passed in. However, they were only intended to support copying simple values from the outter field. This change adds validation to ensure object and nested fields are not used within multi fields. closes #10745 --- .../mapper/core/AbstractFieldMapper.java | 33 ++++++++++--------- .../index/mapper/core/TypeParsers.java | 4 +++ .../mapper/multifield/MultiFieldTests.java | 27 +++++++++++++++ 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java index c18e7c06563..654e203d14d 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java @@ -837,7 +837,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { public static class MultiFields { public static MultiFields empty() { - return new MultiFields(Defaults.PATH_TYPE, ImmutableOpenMap.of()); + return new MultiFields(Defaults.PATH_TYPE, ImmutableOpenMap.of()); } public static class Builder { @@ -860,7 +860,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { if (pathType == Defaults.PATH_TYPE && mapperBuilders.isEmpty()) { return empty(); } else if (mapperBuilders.isEmpty()) { - return new MultiFields(pathType, ImmutableOpenMap.of()); + return new MultiFields(pathType, ImmutableOpenMap.of()); } else { ContentPath.Type origPathType = context.path().pathType(); context.path().pathType(pathType); @@ -869,26 +869,27 @@ public abstract class AbstractFieldMapper implements FieldMapper { for (ObjectObjectCursor cursor : this.mapperBuilders) { String key = cursor.key; Mapper.Builder value = cursor.value; - mapperBuilders.put(key, value.build(context)); + Mapper mapper = value.build(context); + assert mapper instanceof FieldMapper; + mapperBuilders.put(key, mapper); } context.path().remove(); context.path().pathType(origPathType); - ImmutableOpenMap.Builder mappers = mapperBuilders.cast(); + ImmutableOpenMap.Builder mappers = mapperBuilders.cast(); return new MultiFields(pathType, mappers.build()); } } - } private final ContentPath.Type pathType; - private volatile ImmutableOpenMap mappers; + private volatile ImmutableOpenMap mappers; - public MultiFields(ContentPath.Type pathType, ImmutableOpenMap mappers) { + public MultiFields(ContentPath.Type pathType, ImmutableOpenMap mappers) { this.pathType = pathType; this.mappers = mappers; // we disable the all in multi-field mappers - for (ObjectCursor cursor : mappers.values()) { - Mapper mapper = cursor.value; + for (ObjectCursor cursor : mappers.values()) { + FieldMapper mapper = cursor.value; if (mapper instanceof AllFieldMapper.IncludeInAll) { ((AllFieldMapper.IncludeInAll) mapper).unsetIncludeInAll(); } @@ -906,7 +907,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { context.path().pathType(pathType); context.path().add(mainField.name()); - for (ObjectCursor cursor : mappers.values()) { + for (ObjectCursor cursor : mappers.values()) { cursor.value.parse(context); } context.path().remove(); @@ -918,10 +919,10 @@ public abstract class AbstractFieldMapper implements FieldMapper { AbstractFieldMapper mergeWithMultiField = (AbstractFieldMapper) mergeWith; List> newFieldMappers = null; - ImmutableOpenMap.Builder newMappersBuilder = null; + ImmutableOpenMap.Builder newMappersBuilder = null; - for (ObjectCursor cursor : mergeWithMultiField.multiFields.mappers.values()) { - Mapper mergeWithMapper = cursor.value; + for (ObjectCursor cursor : mergeWithMultiField.multiFields.mappers.values()) { + FieldMapper mergeWithMapper = cursor.value; Mapper mergeIntoMapper = mappers.get(mergeWithMapper.name()); if (mergeIntoMapper == null) { // no mapping, simply add it if not simulating @@ -938,7 +939,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { if (newFieldMappers == null) { newFieldMappers = new ArrayList<>(2); } - newFieldMappers.add((FieldMapper) mergeWithMapper); + newFieldMappers.add(mergeWithMapper); } } } else { @@ -957,13 +958,13 @@ public abstract class AbstractFieldMapper implements FieldMapper { } public void traverse(FieldMapperListener fieldMapperListener) { - for (ObjectCursor cursor : mappers.values()) { + for (ObjectCursor cursor : mappers.values()) { cursor.value.traverse(fieldMapperListener); } } public void close() { - for (ObjectCursor cursor : mappers.values()) { + for (ObjectCursor cursor : mappers.values()) { cursor.value.close(); } } diff --git a/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java b/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java index cfed59be20f..20f9eda9b26 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java @@ -34,6 +34,7 @@ import org.elasticsearch.index.mapper.DocumentMapperParser; import org.elasticsearch.index.mapper.FieldMapper.Loading; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperParsingException; +import org.elasticsearch.index.mapper.object.ObjectMapper; import java.util.ArrayList; import java.util.Collections; @@ -332,6 +333,9 @@ public class TypeParsers { } else { throw new MapperParsingException("No type specified for property [" + multiFieldName + "]"); } + if (type.equals(ObjectMapper.CONTENT_TYPE) || type.equals(ObjectMapper.NESTED_CONTENT_TYPE)) { + throw new MapperParsingException("Type [" + type + "] cannot be used in multi field"); + } Mapper.TypeParser typeParser = parserContext.typeParser(type); if (typeParser == null) { diff --git a/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java b/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java index 8a54985f0ce..75c58a7bf8f 100644 --- a/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java +++ b/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java @@ -30,6 +30,7 @@ import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.DocumentMapperParser; import org.elasticsearch.index.mapper.FieldMapper; +import org.elasticsearch.index.mapper.MapperParsingException; import org.elasticsearch.index.mapper.ParseContext.Document; import org.elasticsearch.index.mapper.core.*; import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper; @@ -481,4 +482,30 @@ public class MultiFieldTests extends ElasticsearchSingleNodeTest { DocumentMapper docMapper2 = parser.parse(docMapper.mappingSource().string()); assertThat(docMapper.mappingSource(), equalTo(docMapper2.mappingSource())); } + + public void testObjectFieldNotAllowed() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type").startObject("properties").startObject("my_field") + .field("type", "string").startObject("fields").startObject("multi").field("type", "object").endObject().endObject() + .endObject().endObject().endObject().endObject().string(); + final DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser(); + try { + parser.parse(mapping); + fail("expected mapping parse failure"); + } catch (MapperParsingException e) { + assertTrue(e.getMessage().contains("cannot be used in multi field")); + } + } + + public void testNestedFieldNotAllowed() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type").startObject("properties").startObject("my_field") + .field("type", "string").startObject("fields").startObject("multi").field("type", "nested").endObject().endObject() + .endObject().endObject().endObject().endObject().string(); + final DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser(); + try { + parser.parse(mapping); + fail("expected mapping parse failure"); + } catch (MapperParsingException e) { + assertTrue(e.getMessage().contains("cannot be used in multi field")); + } + } }