honeymoose/OpenSearch
1
0
Fork 0
mirror of https://github.com/honeymoose/OpenSearch.git synced 2025-02-24 05:44:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

Make sure to validate the type before attempting to merge a new mapping. (#45157)

Browse Source 
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.
This commit is contained in:
Julie Tibshirani 2019-08-12 14:10:32 -07:00
parent 4d97d2c50f
commit dc1856ca53
4 changed files with 95 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java
View File

@ -265,6 +265,13 @@ public class MetaDataMappingService {
// 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);
@ -299,14 +306,7 @@ public class MetaDataMappingService {
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) {

19
server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View File

@ -21,7 +21,6 @@ package org.elasticsearch.index.mapper;
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;
@ -72,7 +71,6 @@ import java.util.HashSet;
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;
@ -452,14 +450,6 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
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
@ -707,6 +697,15 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
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.

78
server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java
View File

@ -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;
@ -34,6 +38,7 @@ import org.elasticsearch.test.InternalSettingsPlugin;
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;
@ -134,4 +139,77 @@ public class MetaDataMappingServiceTests extends ESSingleNodeTestCase {
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
server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java
View File

@ -57,7 +57,6 @@ import java.util.concurrent.ExecutionException;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.startsWith;
public class MapperServiceTests extends ESSingleNodeTestCase {
@ -298,36 +297,6 @@ public class MapperServiceTests extends ESSingleNodeTestCase {
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();