From 1891d4f83de8e91cc33ded6966f0efce57c43f5d Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Wed, 4 Apr 2018 10:26:50 +0100 Subject: [PATCH] Check presence of multi-types before validating new mapping (#29316) Before doing any kind of validation on a new mapping, we should first do the multi-type validation in order to provide better error messages. For #29313, this means that the exception message will be Rejecting mapping update to [range_index_new] as the final mapping would have more than 1 type: [_doc, mytype] instead of [expected_attendees] is defined as an object in mapping [mytype] but this name is already used for a field in other types --- .../index/mapper/MapperService.java | 19 +++++----- .../admin/indices/create/CreateIndexIT.java | 19 ---------- .../index/mapper/MapperServiceTests.java | 20 +++++++++- .../index/mapper/UpdateMappingTests.java | 37 +++++++------------ 4 files changed, 41 insertions(+), 54 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 4f3b045bfc2..e13c23754ab 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -385,6 +385,16 @@ public class MapperService extends AbstractIndexComponent implements Closeable { results.put(DEFAULT_MAPPING, defaultMapper); } + if (indexSettings.isSingleType()) { + Set actualTypes = new HashSet<>(mappers.keySet()); + documentMappers.forEach(mapper -> actualTypes.add(mapper.type())); + actualTypes.remove(DEFAULT_MAPPING); + if (actualTypes.size() > 1) { + throw new IllegalArgumentException( + "Rejecting mapping update to [" + index().getName() + "] as the final mapping would have more than 1 type: " + actualTypes); + } + } + for (DocumentMapper mapper : documentMappers) { // check naming validateTypeName(mapper.type()); @@ -478,15 +488,6 @@ public class MapperService extends AbstractIndexComponent implements Closeable { } } - if (indexSettings.isSingleType()) { - Set actualTypes = new HashSet<>(mappers.keySet()); - actualTypes.remove(DEFAULT_MAPPING); - if (actualTypes.size() > 1) { - throw new IllegalArgumentException( - "Rejecting mapping update to [" + index().getName() + "] as the final mapping would have more than 1 type: " + actualTypes); - } - } - // make structures immutable mappers = Collections.unmodifiableMap(mappers); results = Collections.unmodifiableMap(results); diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java b/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java index df63613b5b9..c27d9ef65b2 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java @@ -264,25 +264,6 @@ public class CreateIndexIT extends ESIntegTestCase { logger.info("total: {}", expected.getHits().getTotalHits()); } - /** - * Asserts that the root cause of mapping conflicts is readable. - */ - public void testMappingConflictRootCause() throws Exception { - CreateIndexRequestBuilder b = prepareCreate("test"); - b.addMapping("type1", jsonBuilder().startObject().startObject("properties") - .startObject("text") - .field("type", "text") - .field("analyzer", "standard") - .field("search_analyzer", "whitespace") - .endObject().endObject().endObject()); - b.addMapping("type2", jsonBuilder().humanReadable(true).startObject().startObject("properties") - .startObject("text") - .field("type", "text") - .endObject().endObject().endObject()); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> b.get()); - assertThat(e.getMessage(), containsString("Mapper for [text] conflicts with existing mapping:")); - } - public void testRestartIndexCreationAfterFullClusterRestart() throws Exception { client().admin().cluster().prepareUpdateSettings().setTransientSettings(Settings.builder().put("cluster.routing.allocation.enable", "none")).get(); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index e130b128ac8..732fa9bad18 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -164,7 +164,7 @@ public class MapperServiceTests extends ESSingleNodeTestCase { indexService2.mapperService().merge("type", objectMapping, MergeReason.MAPPING_UPDATE); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> indexService1.mapperService().merge("type2", objectMapping, MergeReason.MAPPING_UPDATE)); + () -> indexService1.mapperService().merge("type", objectMapping, MergeReason.MAPPING_UPDATE)); assertThat(e.getMessage(), containsString("Limit of mapping depth [1] in index [test1] has been exceeded")); } @@ -255,7 +255,6 @@ public class MapperServiceTests extends ESSingleNodeTestCase { // partitioned index cannot have parent/child relationships IllegalArgumentException parentException = expectThrows(IllegalArgumentException.class, () -> { client().admin().indices().prepareCreate("test-index") - .addMapping("parent", "{\"parent\":{\"_routing\":{\"required\":true}}}", XContentType.JSON) .addMapping("child", "{\"child\": {\"_routing\":{\"required\":true}, \"_parent\": {\"type\": \"parent\"}}}", XContentType.JSON) .setSettings(Settings.builder() @@ -307,6 +306,23 @@ public class MapperServiceTests extends ESSingleNodeTestCase { assertThat(e.getMessage(), Matchers.startsWith("Rejecting mapping update to [test] as the final mapping would have more than 1 type: ")); } + /** + * This test checks that the multi-type validation is done before we do any other kind of validation on the mapping that's added, + * see https://github.com/elastic/elasticsearch/issues/29313 + */ + public void testForbidMultipleTypesWithConflictingMappings() throws IOException { + String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties").startObject("field1").field("type", "integer_range").endObject().endObject().endObject().endObject()); + MapperService mapperService = createIndex("test").mapperService(); + mapperService.merge("type", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE); + + String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type2") + .startObject("properties").startObject("field1").field("type", "integer").endObject().endObject().endObject().endObject()); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> mapperService.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE)); + assertThat(e.getMessage(), Matchers.startsWith("Rejecting mapping update to [test] as the final mapping would have more than 1 type: ")); + } + public void testDefaultMappingIsRejectedOn7() throws IOException { String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("_default_").endObject().endObject()); MapperService mapperService = createIndex("test").mapperService(); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java index fd9c2e2b375..311257b837d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java @@ -117,34 +117,25 @@ public class UpdateMappingTests extends ESSingleNodeTestCase { } public void testConflictNewType() throws Exception { - XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type1") + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("properties").startObject("foo").field("type", "long").endObject() .endObject().endObject().endObject(); - MapperService mapperService = createIndex("test", Settings.builder().build(), "type1", mapping).mapperService(); + MapperService mapperService = createIndex("test", Settings.builder().build(), "type", mapping).mapperService(); - XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type2") + XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("properties").startObject("foo").field("type", "double").endObject() .endObject().endObject().endObject(); try { - mapperService.merge("type2", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE); + mapperService.merge("type", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE); fail(); } catch (IllegalArgumentException e) { // expected assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]")); } - try { - mapperService.merge("type2", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE); - fail(); - } catch (IllegalArgumentException e) { - // expected - assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]")); - } - - assertThat(((FieldMapper) mapperService.documentMapper("type1").mapping().root().getMapper("foo")).fieldType().typeName(), + assertThat(((FieldMapper) mapperService.documentMapper("type").mapping().root().getMapper("foo")).fieldType().typeName(), equalTo("long")); - assertNull(mapperService.documentMapper("type2")); } // same as the testConflictNewType except that the mapping update is on an existing type @@ -208,7 +199,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase { public void testRejectFieldDefinedTwice() throws IOException { String mapping1 = Strings.toString(XContentFactory.jsonBuilder().startObject() - .startObject("type1") + .startObject("type") .startObject("properties") .startObject("foo") .field("type", "object") @@ -216,7 +207,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase { .endObject() .endObject().endObject()); String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject() - .startObject("type2") + .startObject("type") .startObject("properties") .startObject("foo") .field("type", "long") @@ -225,17 +216,15 @@ public class UpdateMappingTests extends ESSingleNodeTestCase { .endObject().endObject()); MapperService mapperService1 = createIndex("test1").mapperService(); - mapperService1.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE); + mapperService1.merge("type", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> mapperService1.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE)); - assertThat(e.getMessage(), equalTo("[foo] is defined as a field in mapping [type2" - + "] but this name is already used for an object in other types")); + () -> mapperService1.merge("type", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE)); + assertThat(e.getMessage(), equalTo("Can't merge a non object mapping [foo] with an object mapping [foo]")); MapperService mapperService2 = createIndex("test2").mapperService(); - mapperService2.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE); + mapperService2.merge("type", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE); e = expectThrows(IllegalArgumentException.class, - () -> mapperService2.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE)); - assertThat(e.getMessage(), equalTo("[foo] is defined as an object in mapping [type1" - + "] but this name is already used for a field in other types")); + () -> mapperService2.merge("type", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE)); + assertThat(e.getMessage(), equalTo("mapper [foo] of different type, current_type [long], merged_type [ObjectMapper]")); } }