From cebd7bdd7f5eddb80b26afc0f19301520d6e6c68 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 1 Dec 2015 12:22:07 +0100 Subject: [PATCH] Mappings: Don't ignore merge failures. --- .../index/mapper/MapperService.java | 11 ++- .../mapper/update/UpdateMappingTests.java | 96 +++++++++++++++++++ 2 files changed, 102 insertions(+), 5 deletions(-) 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 f617dd5c6f0..90909737805 100755 --- a/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -250,13 +250,14 @@ public class MapperService extends AbstractIndexComponent implements Closeable { DocumentMapper oldMapper = mappers.get(mapper.type()); if (oldMapper != null) { - MergeResult result = oldMapper.merge(mapper.mapping(), false, updateAllTypes); + // simulate first + MergeResult result = oldMapper.merge(mapper.mapping(), true, updateAllTypes); if (result.hasConflicts()) { - // TODO: What should we do??? - if (logger.isDebugEnabled()) { - logger.debug("merging mapping for type [{}] resulted in conflicts: [{}]", mapper.type(), Arrays.toString(result.buildConflicts())); - } + throw new MergeMappingException(result.buildConflicts()); } + // then apply for real + result = oldMapper.merge(mapper.mapping(), false, updateAllTypes); + assert result.hasConflicts() == false; // we already simulated return oldMapper; } else { List newObjectMappers = new ArrayList<>(); diff --git a/core/src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java b/core/src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java index 5149ab10575..7c15875bc11 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java @@ -29,7 +29,9 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.index.mapper.MergeMappingException; import org.elasticsearch.index.mapper.MergeResult; +import org.elasticsearch.index.mapper.core.LongFieldMapper; import org.elasticsearch.test.ESSingleNodeTestCase; import java.io.IOException; @@ -107,6 +109,100 @@ public class UpdateMappingTests extends ESSingleNodeTestCase { assertThat(mappingAfterUpdate, equalTo(mappingBeforeUpdate)); } + public void testConflictSameType() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties").startObject("foo").field("type", "long").endObject() + .endObject().endObject().endObject(); + MapperService mapperService = createIndex("test", Settings.settingsBuilder().build(), "type", mapping).mapperService(); + + XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties").startObject("foo").field("type", "double").endObject() + .endObject().endObject().endObject(); + + try { + mapperService.merge("type", new CompressedXContent(update.string()), false, false); + fail(); + } catch (MergeMappingException e) { + // expected + } + + try { + mapperService.merge("type", new CompressedXContent(update.string()), false, false); + fail(); + } catch (MergeMappingException e) { + // expected + } + + assertTrue(mapperService.documentMapper("type").mapping().root().getMapper("foo") instanceof LongFieldMapper); + } + + public void testConflictNewType() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type1") + .startObject("properties").startObject("foo").field("type", "long").endObject() + .endObject().endObject().endObject(); + MapperService mapperService = createIndex("test", Settings.settingsBuilder().build(), "type1", mapping).mapperService(); + + XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type2") + .startObject("properties").startObject("foo").field("type", "double").endObject() + .endObject().endObject().endObject(); + + try { + mapperService.merge("type2", new CompressedXContent(update.string()), false, false); + fail(); + } catch (IllegalArgumentException e) { + // expected + assertTrue(e.getMessage().contains("conflicts with existing mapping in other types")); + } + + try { + mapperService.merge("type2", new CompressedXContent(update.string()), false, false); + fail(); + } catch (IllegalArgumentException e) { + // expected + assertTrue(e.getMessage().contains("conflicts with existing mapping in other types")); + } + + assertTrue(mapperService.documentMapper("type1").mapping().root().getMapper("foo") instanceof LongFieldMapper); + assertNull(mapperService.documentMapper("type2")); + } + + // same as the testConflictNewType except that the mapping update is on an existing type + @AwaitsFix(bugUrl="https://github.com/elastic/elasticsearch/issues/15049") + public void testConflictNewTypeUpdate() throws Exception { + XContentBuilder mapping1 = XContentFactory.jsonBuilder().startObject().startObject("type1") + .startObject("properties").startObject("foo").field("type", "long").endObject() + .endObject().endObject().endObject(); + XContentBuilder mapping2 = XContentFactory.jsonBuilder().startObject().startObject("type2").endObject().endObject(); + MapperService mapperService = createIndex("test", Settings.settingsBuilder().build()).mapperService(); + + mapperService.merge("type1", new CompressedXContent(mapping1.string()), false, false); + mapperService.merge("type2", new CompressedXContent(mapping2.string()), false, false); + + XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type2") + .startObject("properties").startObject("foo").field("type", "double").endObject() + .endObject().endObject().endObject(); + + try { + mapperService.merge("type2", new CompressedXContent(update.string()), false, false); + fail(); + } catch (IllegalArgumentException e) { + // expected + assertTrue(e.getMessage().contains("conflicts with existing mapping in other types")); + } + + try { + mapperService.merge("type2", new CompressedXContent(update.string()), false, false); + fail(); + } catch (IllegalArgumentException e) { + // expected + assertTrue(e.getMessage().contains("conflicts with existing mapping in other types")); + } + + assertTrue(mapperService.documentMapper("type1").mapping().root().getMapper("foo") instanceof LongFieldMapper); + assertNotNull(mapperService.documentMapper("type2")); + assertNull(mapperService.documentMapper("type2").mapping().root().getMapper("foo")); + } + public void testIndexFieldParsingBackcompat() throws IOException { IndexService indexService = createIndex("test", Settings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_1_4_2.id).build()); XContentBuilder indexMapping = XContentFactory.jsonBuilder();