From 8558a408943e77773f06619b0e98f1a59731ba6e Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Thu, 3 Dec 2015 15:31:52 +0100 Subject: [PATCH] Fix copy_to when the target is a dynamic object field. Fixes #11237 --- .../index/mapper/DocumentParser.java | 82 ++++++---- .../copyto/CopyToMapperIntegrationIT.java | 20 +++ .../mapper/copyto/CopyToMapperTests.java | 150 +++++++++++++++++- 3 files changed, 215 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index de4dc387c88..805607096cf 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -28,8 +28,6 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.joda.FormatDateTimeFormatter; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ReleasableLock; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.mapper.core.DateFieldMapper.DateFieldType; @@ -47,7 +45,6 @@ import java.io.IOException; import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; /** A parser for documents, given mappings from a DocumentMapper */ @@ -712,37 +709,64 @@ class DocumentParser implements Closeable { // The path of the dest field might be completely different from the current one so we need to reset it context = context.overridePath(new ContentPath(0)); + String[] paths = Strings.splitStringToArray(field, '.'); + String fieldName = paths[paths.length-1]; ObjectMapper mapper = context.root(); - String objectPath = ""; - String fieldPath = field; - int posDot = field.lastIndexOf('.'); - if (posDot > 0) { - objectPath = field.substring(0, posDot); - context.path().add(objectPath); - mapper = context.docMapper().objectMappers().get(objectPath); - fieldPath = field.substring(posDot + 1); + ObjectMapper[] mappers = new ObjectMapper[paths.length-1]; + if (paths.length > 1) { + ObjectMapper parent = context.root(); + for (int i = 0; i < paths.length-1; i++) { + mapper = context.docMapper().objectMappers().get(context.path().fullPathAsText(paths[i])); + if (mapper == null) { + // One mapping is missing, check if we are allowed to create a dynamic one. + ObjectMapper.Dynamic dynamic = parent.dynamic(); + if (dynamic == null) { + dynamic = dynamicOrDefault(context.root().dynamic()); + } + + switch (dynamic) { + case STRICT: + throw new StrictDynamicMappingException(parent.fullPath(), paths[i]); + case TRUE: + Mapper.Builder builder = context.root().findTemplateBuilder(context, paths[i], "object"); + if (builder == null) { + // if this is a non root object, then explicitly set the dynamic behavior if set + if (!(parent instanceof RootObjectMapper) && parent.dynamic() != ObjectMapper.Defaults.DYNAMIC) { + ((ObjectMapper.Builder) builder).dynamic(parent.dynamic()); + } + builder = MapperBuilders.object(paths[i]).enabled(true).pathType(parent.pathType()); + } + Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings(), context.path()); + mapper = (ObjectMapper) builder.build(builderContext); + if (mapper.nested() != ObjectMapper.Nested.NO) { + throw new MapperParsingException("It is forbidden to create dynamic nested objects ([" + context.path().fullPathAsText(paths[i]) + "]) through `copy_to`"); + } + break; + case FALSE: + // Maybe we should log something to tell the user that the copy_to is ignored in this case. + break; + default: + throw new AssertionError("Unexpected dynamic type " + dynamic); + + } + } + context.path().add(paths[i]); + mappers[i] = mapper; + parent = mapper; + } } - if (mapper == null) { - //TODO: Create an object dynamically? - throw new MapperParsingException("attempt to copy value to non-existing object [" + field + "]"); - } - ObjectMapper update = parseDynamicValue(context, mapper, fieldPath, context.parser().currentToken()); + ObjectMapper update = parseDynamicValue(context, mapper, fieldName, context.parser().currentToken()); assert update != null; // we are parsing a dynamic value so we necessarily created a new mapping - // propagate the update to the root - while (objectPath.length() > 0) { - String parentPath = ""; - ObjectMapper parent = context.root(); - posDot = objectPath.lastIndexOf('.'); - if (posDot > 0) { - parentPath = objectPath.substring(0, posDot); - parent = context.docMapper().objectMappers().get(parentPath); + if (paths.length > 1) { + for (int i = paths.length - 2; i >= 0; i--) { + ObjectMapper parent = context.root(); + if (i > 0) { + parent = mappers[i-1]; + } + assert parent != null; + update = parent.mappingUpdate(update); } - if (parent == null) { - throw new IllegalStateException("[" + objectPath + "] has no parent for path [" + parentPath + "]"); - } - update = parent.mappingUpdate(update); - objectPath = parentPath; } context.addDynamicMappingsUpdate(update); } diff --git a/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperIntegrationIT.java b/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperIntegrationIT.java index 1d6e72834c3..4a010747624 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperIntegrationIT.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperIntegrationIT.java @@ -30,6 +30,7 @@ import org.elasticsearch.test.ESIntegTestCase; import java.io.IOException; +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; @@ -68,6 +69,25 @@ public class CopyToMapperIntegrationIT extends ESIntegTestCase { } + public void testDynamicObjectCopyTo() throws Exception { + String mapping = jsonBuilder().startObject().startObject("doc").startObject("properties") + .startObject("foo") + .field("type", "string") + .field("copy_to", "root.top.child") + .endObject() + .endObject().endObject().endObject().string(); + assertAcked( + client().admin().indices().prepareCreate("test-idx") + .addMapping("doc", mapping) + ); + client().prepareIndex("test-idx", "doc", "1") + .setSource("foo", "bar") + .get(); + client().admin().indices().prepareRefresh("test-idx").execute().actionGet(); + SearchResponse response = client().prepareSearch("test-idx") + .setQuery(QueryBuilders.termQuery("root.top.child", "bar")).get(); + assertThat(response.getHits().totalHits(), equalTo(1L)); + } private XContentBuilder createDynamicTemplateMapping() throws IOException { return XContentFactory.jsonBuilder().startObject().startObject("doc") diff --git a/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java index 301d6b13e3b..d94ae2b6735 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java @@ -167,27 +167,126 @@ public class CopyToMapperTests extends ESSingleNodeTestCase { } - public void testCopyToFieldsNonExistingInnerObjectParsing() throws Exception { - String mapping = jsonBuilder().startObject().startObject("type1").startObject("properties") - + public void testCopyToDynamicInnerObjectParsing() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type1") + .startObject("properties") .startObject("copy_test") - .field("type", "string") - .field("copy_to", "very.inner.field") + .field("type", "string") + .field("copy_to", "very.inner.field") .endObject() - - .endObject().endObject().endObject().string(); + .endObject() + .endObject().endObject().string(); DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping); BytesReference json = jsonBuilder().startObject() .field("copy_test", "foo") + .field("new_field", "bar") .endObject().bytes(); + ParseContext.Document doc = docMapper.parse("test", "type1", "1", json).rootDoc(); + assertThat(doc.getFields("copy_test").length, equalTo(1)); + assertThat(doc.getFields("copy_test")[0].stringValue(), equalTo("foo")); + + assertThat(doc.getFields("very.inner.field").length, equalTo(1)); + assertThat(doc.getFields("very.inner.field")[0].stringValue(), equalTo("foo")); + + assertThat(doc.getFields("new_field").length, equalTo(1)); + assertThat(doc.getFields("new_field")[0].stringValue(), equalTo("bar")); + } + + public void testCopyToDynamicInnerInnerObjectParsing() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type1") + .startObject("properties") + .startObject("copy_test") + .field("type", "string") + .field("copy_to", "very.far.inner.field") + .endObject() + .startObject("very") + .field("type", "object") + .startObject("properties") + .startObject("far") + .field("type", "object") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject().endObject().string(); + + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping); + + BytesReference json = jsonBuilder().startObject() + .field("copy_test", "foo") + .field("new_field", "bar") + .endObject().bytes(); + + ParseContext.Document doc = docMapper.parse("test", "type1", "1", json).rootDoc(); + assertThat(doc.getFields("copy_test").length, equalTo(1)); + assertThat(doc.getFields("copy_test")[0].stringValue(), equalTo("foo")); + + assertThat(doc.getFields("very.far.inner.field").length, equalTo(1)); + assertThat(doc.getFields("very.far.inner.field")[0].stringValue(), equalTo("foo")); + + assertThat(doc.getFields("new_field").length, equalTo(1)); + assertThat(doc.getFields("new_field")[0].stringValue(), equalTo("bar")); + } + + public void testCopyToStrictDynamicInnerObjectParsing() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type1") + .field("dynamic", "strict") + .startObject("properties") + .startObject("copy_test") + .field("type", "string") + .field("copy_to", "very.inner.field") + .endObject() + .endObject() + .endObject().endObject().string(); + + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping); + + BytesReference json = jsonBuilder().startObject() + .field("copy_test", "foo") + .endObject().bytes(); + try { docMapper.parse("test", "type1", "1", json).rootDoc(); fail(); } catch (MapperParsingException ex) { - assertThat(ex.getMessage(), startsWith("attempt to copy value to non-existing object")); + assertThat(ex.getMessage(), startsWith("mapping set to strict, dynamic introduction of [very] within [type1] is not allowed")); + } + } + + public void testCopyToInnerStrictDynamicInnerObjectParsing() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type1") + .startObject("properties") + .startObject("copy_test") + .field("type", "string") + .field("copy_to", "very.far.field") + .endObject() + .startObject("very") + .field("type", "object") + .startObject("properties") + .startObject("far") + .field("type", "object") + .field("dynamic", "strict") + .endObject() + .endObject() + .endObject() + + .endObject() + .endObject().endObject().string(); + + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping); + + BytesReference json = jsonBuilder().startObject() + .field("copy_test", "foo") + .endObject().bytes(); + + try { + docMapper.parse("test", "type1", "1", json).rootDoc(); + fail(); + } catch (MapperParsingException ex) { + assertThat(ex.getMessage(), startsWith("mapping set to strict, dynamic introduction of [field] within [very.far] is not allowed")); } } @@ -337,6 +436,41 @@ public class CopyToMapperTests extends ESSingleNodeTestCase { } } + public void testCopyToDynamicNestedObjectParsing() throws Exception { + String mapping = jsonBuilder().startObject().startObject("type1") + .startArray("dynamic_templates") + .startObject() + .startObject("objects") + .field("match_mapping_type", "object") + .startObject("mapping") + .field("type", "nested") + .endObject() + .endObject() + .endObject() + .endArray() + .startObject("properties") + .startObject("copy_test") + .field("type", "string") + .field("copy_to", "very.inner.field") + .endObject() + .endObject() + .endObject().endObject().string(); + + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping); + + BytesReference json = jsonBuilder().startObject() + .field("copy_test", "foo") + .field("new_field", "bar") + .endObject().bytes(); + + try { + docMapper.parse("test", "type1", "1", json).rootDoc(); + fail(); + } catch (MapperParsingException ex) { + assertThat(ex.getMessage(), startsWith("It is forbidden to create dynamic nested objects ([very]) through `copy_to`")); + } + } + private void assertFieldValue(Document doc, String field, Number... expected) { IndexableField[] values = doc.getFields(field); if (values == null) {