Skip to content

Commit

Permalink
Make sure to validate the type before attempting to merge a new mappi…
Browse files Browse the repository at this point in the history 
…ng. (#45157)

Currently, when adding a new mapping, we attempt to parse + merge it before
checking whether its top-level document type matches the existing type. So when
a user attempts to introduce a new mapping type, we may give a confusing error
message around merging instead of complaining that it's not possible to add
more than one type ("Rejecting mapping update to [my-index] as the final
mapping would have more than 1 type...").

This PR moves the type validation to the start of
`MetaDataMappingService#applyRequest` so that we make sure the type matches
before performing any mapper merging.

We already partially addressed this issue in #29316, but the tests there
focused on `MapperService` and did not catch this problem with end-to-end
mapping updates.

Addresses #43012.
  • Loading branch information
@jtibshirani
jtibshirani authored Aug 12, 2019
1 parent 5021c5d commit abb30f0
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 49 deletions.
16 changes: 8 additions & 8 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,13 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
// try and parse it (no need to add it here) so we can bail early in case of parsing exception
DocumentMapper newMapper;
DocumentMapper existingMapper = mapperService.documentMapper();

String typeForUpdate = mapperService.getTypeForUpdate(mappingType, mappingUpdateSource);
if (existingMapper != null && existingMapper.type().equals(typeForUpdate) == false) {
throw new IllegalArgumentException("Rejecting mapping update to [" + mapperService.index().getName() +
"] as the final mapping would have more than 1 type: " + Arrays.asList(existingMapper.type(), typeForUpdate));
}

if (MapperService.DEFAULT_MAPPING.equals(request.type())) {
// _default_ types do not go through merging, but we do test the new settings. Also don't apply the old default
newMapper = mapperService.parse(request.type(), mappingUpdateSource, false);
Expand Down Expand Up @@ -299,14 +306,7 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
final Index index = indexMetaData.getIndex();
final MapperService mapperService = indexMapperServices.get(index);

// If the _type name is _doc and there is no _doc top-level key then this means that we
// are handling a typeless call. In such a case, we override _doc with the actual type
// name in the mappings. This allows to use typeless APIs on typed indices.
String typeForUpdate = mappingType; // the type to use to apply the mapping update
if (isMappingSourceTyped(request.type(), mappingUpdateSource) == false) {
typeForUpdate = mapperService.resolveDocumentType(mappingType);
}

String typeForUpdate = mapperService.getTypeForUpdate(mappingType, mappingUpdateSource);
CompressedXContent existingSource = null;
DocumentMapper existingMapper = mapperService.documentMapper(typeForUpdate);
if (existingMapper != null) {
Expand Down
19 changes: 9 additions & 10 deletions server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import com.carrotsearch.hppc.ObjectHashSet;
import com.carrotsearch.hppc.cursors.ObjectCursor;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.lucene.analysis.Analyzer;
Expand Down Expand Up @@ -72,7 +71,6 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;
Expand Down Expand Up @@ -450,14 +448,6 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
results.put(DEFAULT_MAPPING, defaultMapper);
}

{
if (mapper != null && this.mapper != null && Objects.equals(this.mapper.type(), mapper.type()) == false) {
throw new IllegalArgumentException(
"Rejecting mapping update to [" + index().getName() + "] as the final mapping would have more than 1 type: "
+ Arrays.asList(this.mapper.type(), mapper.type()));
}
}

DocumentMapper newMapper = null;
if (mapper != null) {
// check naming
Expand Down Expand Up @@ -705,6 +695,15 @@ public static boolean isMappingSourceTyped(String type, CompressedXContent mappi
return isMappingSourceTyped(type, root);
}

/**
* If the _type name is _doc and there is no _doc top-level key then this means that we
* are handling a typeless call. In such a case, we override _doc with the actual type
* name in the mappings. This allows to use typeless APIs on typed indices.
*/
public String getTypeForUpdate(String type, CompressedXContent mappingSource) {
return isMappingSourceTyped(type, mappingSource) == false ? resolveDocumentType(type) : type;
}

/**
* Resolves a type from a mapping-related request into the type that should be used when
* merging and updating mappings.
Expand Down
78 changes: 78 additions & 0 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@

package org.elasticsearch.cluster.metadata;

import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingClusterStateUpdateRequest;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateTaskExecutor;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.mapper.MapperService;
Expand All @@ -34,6 +38,7 @@
import java.util.Collection;
import java.util.Collections;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;

Expand Down Expand Up @@ -134,4 +139,77 @@ public void testMappingUpdateAccepts_docAsType() throws Exception {
Collections.singletonMap("foo",
Collections.singletonMap("type", "keyword"))), mappingMetaData.sourceAsMap());
}

public void testForbidMultipleTypes() throws Exception {
CreateIndexRequestBuilder createIndexRequest = client().admin().indices()
.prepareCreate("test")
.addMapping(MapperService.SINGLE_MAPPING_NAME);
IndexService indexService = createIndex("test", createIndexRequest);

MetaDataMappingService mappingService = getInstanceFromNode(MetaDataMappingService.class);
ClusterService clusterService = getInstanceFromNode(ClusterService.class);

PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest()
.type("other_type")
.indices(new Index[] {indexService.index()})
.source(Strings.toString(XContentFactory.jsonBuilder()
.startObject()
.startObject("other_type").endObject()
.endObject()));
ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =
mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
assertThat(result.executionResults.size(), equalTo(1));

ClusterStateTaskExecutor.TaskResult taskResult = result.executionResults.values().iterator().next();
assertFalse(taskResult.isSuccess());
assertThat(taskResult.getFailure().getMessage(), containsString(
"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 Exception {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject(MapperService.SINGLE_MAPPING_NAME)
.startObject("properties")
.startObject("field1")
.field("type", "text")
.endObject()
.endObject()
.endObject()
.endObject();

CreateIndexRequestBuilder createIndexRequest = client().admin().indices()
.prepareCreate("test")
.addMapping(MapperService.SINGLE_MAPPING_NAME, mapping);
IndexService indexService = createIndex("test", createIndexRequest);

MetaDataMappingService mappingService = getInstanceFromNode(MetaDataMappingService.class);
ClusterService clusterService = getInstanceFromNode(ClusterService.class);

String conflictingMapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
.startObject("other_type")
.startObject("properties")
.startObject("field1")
.field("type", "keyword")
.endObject()
.endObject()
.endObject()
.endObject());

PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest()
.type("other_type")
.indices(new Index[] {indexService.index()})
.source(conflictingMapping);
ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =
mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
assertThat(result.executionResults.size(), equalTo(1));

ClusterStateTaskExecutor.TaskResult taskResult = result.executionResults.values().iterator().next();
assertFalse(taskResult.isSuccess());
assertThat(taskResult.getFailure().getMessage(), containsString(
"Rejecting mapping update to [test] as the final mapping would have more than 1 type: "));
}
}
31 changes: 0 additions & 31 deletions server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.startsWith;

public class MapperServiceTests extends ESSingleNodeTestCase {

Expand Down Expand Up @@ -298,36 +297,6 @@ public void testTotalFieldsLimitWithFieldAlias() throws Throwable {
assertEquals("Limit of total fields [" + numberOfNonAliasFields + "] in index [test2] has been exceeded", e.getMessage());
}

public void testForbidMultipleTypes() throws IOException {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type").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").endObject().endObject());
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> mapperService.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE));
assertThat(e.getMessage(), 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(), 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

0 comments on commit abb30f0

Please sign in to comment.