Skip to content

Check presence of multi-types before validating new mapping #29316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ywelsch merged 4 commits into from
Apr 4, 2018
Merged

Check presence of multi-types before validating new mapping #29316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
408ec34
Check presence of multi-types before validating new mapping
ywelsch Mar 30, 2018
16b98e5
fix tests
ywelsch Apr 3, 2018
6609b00
Merge remote-tracking branch 'elastic/master' into check-multi-types-…
ywelsch Apr 3, 2018
5f8e33b
test has become irrelevant
ywelsch Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,16 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
results.put(DEFAULT_MAPPING, defaultMapper);
}

if (indexSettings.isSingleType()) {
Set<String> 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());
Expand Down Expand Up @@ -478,15 +488,6 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
}
}

if (indexSettings.isSingleType()) {
Set<String> 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);
Expand Down
19 changes: 0 additions & 19 deletions server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,25 +264,6 @@ public void onFailure(Exception e) {
logger.info("total: {}", expected.getHits().getTotalHits());
}

/**
* Asserts that the root cause of mapping conflicts is readable.
*/
public void testMappingConflictRootCause() throws Exception {
Copy link
Contributor Author

@ywelsch ywelsch Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while backporting to 6.x, I've reenabled this test (but set index.version.created to 5.x)

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();
Expand Down
20 changes: 18 additions & 2 deletions server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public void testMappingDepthExceedsLimit() throws Throwable {
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"));
}

Expand Down Expand Up @@ -255,7 +255,6 @@ public void testPartitionedConstraints() {
// 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()
Expand Down Expand Up @@ -307,6 +306,23 @@ public void testForbidMultipleTypes() throws IOException {
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();
Expand Down
37 changes: 13 additions & 24 deletions server/src/test/java/org/elasticsearch/index/mapper/UpdateMappingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,34 +117,25 @@ public void testConflictSameType() throws Exception {
}

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);
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);
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]"));
}

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
Expand Down Expand Up @@ -208,15 +199,15 @@ public void testReuseMetaField() throws IOException {

public void testRejectFieldDefinedTwice() throws IOException {
String mapping1 = Strings.toString(XContentFactory.jsonBuilder().startObject()
.startObject("type1")
.startObject("type")
.startObject("properties")
.startObject("foo")
.field("type", "object")
.endObject()
.endObject()
.endObject().endObject());
String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject()
.startObject("type2")
.startObject("type")
.startObject("properties")
.startObject("foo")
.field("type", "long")
Expand All @@ -225,17 +216,15 @@ public void testRejectFieldDefinedTwice() throws IOException {
.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]"));
}
}