From 96ec48afcde8b8689cbce09e6f8afe5c3153d9ae Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 10 Mar 2016 17:28:13 -0800 Subject: [PATCH] Fix dynamic mapper when its parent already has an update The change to move dynamic mapping handling to the end of document parsing has an edge case which can cause dynamic mappings to fail document parsing. If field a.b is added as an as part of the root update, followed by a.c.d, then we need to expand the mappers on the stack, since a is hidden inside the root update which exists on the stack. This change adds a test for this case, as well as tries to better document how the logic works for building up the stack before adding a dynamic mapper. --- .../index/mapper/DocumentParser.java | 88 +++++++++++++------ .../index/mapper/DocumentParserTests.java | 66 ++++++++------ 2 files changed, 104 insertions(+), 50 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 36c1cf106f4..8c8ded9b543 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -23,18 +23,14 @@ import java.io.Closeable; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.Iterator; import java.util.List; -import java.util.Set; import org.apache.lucene.document.Field; -import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.CloseableThreadLocal; import org.elasticsearch.common.Strings; import org.elasticsearch.common.joda.FormatDateTimeFormatter; -import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.IndexSettings; @@ -240,28 +236,26 @@ final class DocumentParser implements Closeable { } previousMapper = newMapper; String[] nameParts = newMapper.name().split("\\."); - // find common elements with the previously processed dynamic mapper - int keepBefore = 1; - while (keepBefore < parentMappers.size() && - parentMappers.get(keepBefore).simpleName().equals(nameParts[keepBefore - 1])) { - ++keepBefore; - } - popMappers(parentMappers, keepBefore, true); - if (keepBefore < nameParts.length) { - String updateParentName = nameParts[keepBefore - 1]; - final ObjectMapper lastParent = parentMappers.get(parentMappers.size() - 1); - Mapper updateParent = lastParent.getMapper(updateParentName); - if (updateParent == null) { - // the parent we need is not on the stack, so look it up in the full mappings - if (keepBefore > 1) { - // only prefix with parent mapper if the parent mapper isn't the root (which has a fake name) - updateParentName = lastParent.name() + '.' + updateParentName; - } - updateParent = docMapper.objectMappers().get(updateParentName); - } - assert updateParent instanceof ObjectMapper; - newMapper = createUpdate((ObjectMapper)updateParent, nameParts, keepBefore, newMapper); + // We first need the stack to only contain mappers in common with the previously processed mapper + // For example, if the first mapper processed was a.b.c, and we now have a.d, the stack will contain + // a.b, and we want to merge b back into the stack so it just contains a + int i = removeUncommonMappers(parentMappers, nameParts); + + // Then we need to add back mappers that may already exist within the stack, but are not on it. + // For example, if we processed a.b, followed by an object mapper a.c.d, and now are adding a.c.d.e + // then the stack will only have a on it because we will have already merged a.c.d into the stack. + // So we need to pull a.c, followed by a.c.d, onto the stack so e can be added to the end. + i = expandCommonMappers(parentMappers, nameParts, i); + + // If there are still parents of the new mapper which are not on the stack, we need to pull them + // from the existing mappings. In order to maintain the invariant that the stack only contains + // fields which are updated, we cannot simply add the existing mappers to the stack, since they + // may have other subfields which will not be updated. Instead, we pull the mapper from the existing + // mappings, and build an update with only the new mapper and its parents. This then becomes our + // "new mapper", and can be added to the stack. + if (i < nameParts.length - 1) { + newMapper = createExistingMapperUpdate(parentMappers, nameParts, i, docMapper, newMapper); } if (newMapper instanceof ObjectMapper) { @@ -299,12 +293,56 @@ final class DocumentParser implements Closeable { parentMappers.set(lastIndex, withNewMapper); } + /** + * Removes mappers that exist on the stack, but are not part of the path of the current nameParts, + * Returns the next unprocessed index from nameParts. + */ + private static int removeUncommonMappers(List parentMappers, String[] nameParts) { + int keepBefore = 1; + while (keepBefore < parentMappers.size() && + parentMappers.get(keepBefore).simpleName().equals(nameParts[keepBefore - 1])) { + ++keepBefore; + } + popMappers(parentMappers, keepBefore, true); + return keepBefore - 1; + } + + /** + * Adds mappers from the end of the stack that exist as updates within those mappers. + * Returns the next unprocessed index from nameParts. + */ + private static int expandCommonMappers(List parentMappers, String[] nameParts, int i) { + ObjectMapper last = parentMappers.get(parentMappers.size() - 1); + while (i < nameParts.length - 1 && last.getMapper(nameParts[i]) != null) { + Mapper newLast = last.getMapper(nameParts[i]); + assert newLast instanceof ObjectMapper; + parentMappers.add((ObjectMapper)newLast); + ++i; + } + return i; + } + + /** Creates an update for intermediate object mappers that are not on the stack, but parents of newMapper. */ + private static ObjectMapper createExistingMapperUpdate(List parentMappers, String[] nameParts, int i, + DocumentMapper docMapper, Mapper newMapper) { + String updateParentName = nameParts[i]; + final ObjectMapper lastParent = parentMappers.get(parentMappers.size() - 1); + if (parentMappers.size() > 1) { + // only prefix with parent mapper if the parent mapper isn't the root (which has a fake name) + updateParentName = lastParent.name() + '.' + nameParts[i]; + } + ObjectMapper updateParent = docMapper.objectMappers().get(updateParentName); + assert updateParent != null : updateParentName + " doesn't exist"; + return createUpdate(updateParent, nameParts, i + 1, newMapper); + } + /** Build an update for the parent which will contain the given mapper and any intermediate fields. */ private static ObjectMapper createUpdate(ObjectMapper parent, String[] nameParts, int i, Mapper mapper) { List parentMappers = new ArrayList<>(); ObjectMapper previousIntermediate = parent; for (; i < nameParts.length - 1; ++i) { Mapper intermediate = previousIntermediate.getMapper(nameParts[i]); + assert intermediate != null : "Field " + previousIntermediate.name() + " does not have a subfield " + nameParts[i]; assert intermediate instanceof ObjectMapper; parentMappers.add((ObjectMapper)intermediate); previousIntermediate = (ObjectMapper)intermediate; diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index cbc858b642d..e4d1e306af3 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -72,9 +72,10 @@ public class DocumentParserTests extends ESSingleNodeTestCase { DocumentMapper createDummyMapping(MapperService mapperService) throws Exception { String mapping = jsonBuilder().startObject().startObject("type").startObject("properties") - .startObject("a").startObject("properties") - .startObject("b").field("type", "object").startObject("properties") - .startObject("c").field("type", "object") + .startObject("y").field("type", "object").endObject() + .startObject("x").startObject("properties") + .startObject("subx").field("type", "object").startObject("properties") + .startObject("subsubx").field("type", "object") .endObject().endObject().endObject().endObject().endObject().endObject().endObject().endObject().string(); DocumentMapper defaultMapper = mapperService.documentMapperParser().parse("type", new CompressedXContent(mapping)); @@ -109,40 +110,55 @@ public class DocumentParserTests extends ESSingleNodeTestCase { public void testSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(createIndex("test").mapperService()); - List updates = Collections.singletonList(new MockFieldMapper("a.foo")); + List updates = Collections.singletonList(new MockFieldMapper("x.foo")); Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mapping(), docMapper, updates); - Mapper aMapper = mapping.root().getMapper("a"); - assertNotNull(aMapper); - assertTrue(aMapper instanceof ObjectMapper); - assertNotNull(((ObjectMapper)aMapper).getMapper("foo")); - assertNull(((ObjectMapper)aMapper).getMapper("b")); + Mapper xMapper = mapping.root().getMapper("x"); + assertNotNull(xMapper); + assertTrue(xMapper instanceof ObjectMapper); + assertNotNull(((ObjectMapper)xMapper).getMapper("foo")); + assertNull(((ObjectMapper)xMapper).getMapper("subx")); } public void testMultipleSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(createIndex("test").mapperService()); List updates = new ArrayList<>(); - updates.add(new MockFieldMapper("a.foo")); - updates.add(new MockFieldMapper("a.bar")); + updates.add(new MockFieldMapper("x.foo")); + updates.add(new MockFieldMapper("x.bar")); Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mapping(), docMapper, updates); - Mapper aMapper = mapping.root().getMapper("a"); - assertNotNull(aMapper); - assertTrue(aMapper instanceof ObjectMapper); - assertNotNull(((ObjectMapper)aMapper).getMapper("foo")); - assertNotNull(((ObjectMapper)aMapper).getMapper("bar")); - assertNull(((ObjectMapper)aMapper).getMapper("b")); + Mapper xMapper = mapping.root().getMapper("x"); + assertNotNull(xMapper); + assertTrue(xMapper instanceof ObjectMapper); + assertNotNull(((ObjectMapper)xMapper).getMapper("foo")); + assertNotNull(((ObjectMapper)xMapper).getMapper("bar")); + assertNull(((ObjectMapper)xMapper).getMapper("subx")); } public void testDeepSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(createIndex("test").mapperService()); - List updates = Collections.singletonList(new MockFieldMapper("a.b.foo")); + List updates = Collections.singletonList(new MockFieldMapper("x.subx.foo")); Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mapping(), docMapper, updates); - Mapper aMapper = mapping.root().getMapper("a"); - assertNotNull(aMapper); - assertTrue(aMapper instanceof ObjectMapper); - Mapper bMapper = ((ObjectMapper)aMapper).getMapper("b"); - assertTrue(bMapper instanceof ObjectMapper); - assertNotNull(((ObjectMapper)bMapper).getMapper("foo")); - assertNull(((ObjectMapper)bMapper).getMapper("c")); + Mapper xMapper = mapping.root().getMapper("x"); + assertNotNull(xMapper); + assertTrue(xMapper instanceof ObjectMapper); + Mapper subxMapper = ((ObjectMapper)xMapper).getMapper("subx"); + assertTrue(subxMapper instanceof ObjectMapper); + assertNotNull(((ObjectMapper)subxMapper).getMapper("foo")); + assertNull(((ObjectMapper)subxMapper).getMapper("subsubx")); + } + + public void testDeepSubfieldAfterSubfieldMappingUpdate() throws Exception { + DocumentMapper docMapper = createDummyMapping(createIndex("test").mapperService()); + List updates = new ArrayList<>(); + updates.add(new MockFieldMapper("x.a")); + updates.add(new MockFieldMapper("x.subx.b")); + Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mapping(), docMapper, updates); + Mapper xMapper = mapping.root().getMapper("x"); + assertNotNull(xMapper); + assertTrue(xMapper instanceof ObjectMapper); + assertNotNull(((ObjectMapper)xMapper).getMapper("a")); + Mapper subxMapper = ((ObjectMapper)xMapper).getMapper("subx"); + assertTrue(subxMapper instanceof ObjectMapper); + assertNotNull(((ObjectMapper)subxMapper).getMapper("b")); } public void testObjectMappingUpdate() throws Exception {