Skip to content
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

Remove typename checks in mapping updates #47347

Merged
merged 4 commits into from
Oct 3, 2019
Merged

Remove typename checks in mapping updates #47347

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0941218
Remove typename checks in mapping updates
romseygeek Oct 1, 2019
fa832d1
remove type clash test
romseygeek Oct 1, 2019
4d5af5b
Merge branch 'master' into types-removal/update-mapping-type-check
elasticmachine Oct 3, 2019
d3ab721
remove unused method
romseygeek Oct 3, 2019
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
9 changes: 0 additions & 9 deletions rest-api-spec/src/main/resources/rest-api-spec/test/delete/70_mix_typeless_typeful.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,6 @@
id: 1
body: { foo: bar }

- do:
catch: bad_request
delete:
index: index
type: some_random_type
id: 1

- match: { error.root_cause.0.reason: "/Rejecting.mapping.update.to.\\[index\\].as.the.final.mapping.would.have.more.than.1.type.*/" }

- do:
delete:
index: index
Expand Down
35 changes: 5 additions & 30 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,14 @@
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MapperService.MergeReason;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.indices.InvalidTypeNameException;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.index.mapper.MapperService.isMappingSourceTyped;
import static org.elasticsearch.indices.cluster.IndicesClusterStateService.AllocatedIndices.IndexRemovalReason.NO_LONGER_ASSIGNED;

/**
Expand Down Expand Up @@ -245,7 +242,7 @@ class PutMappingExecutor implements ClusterStateTaskExecutor<PutMappingClusterSt

private ClusterState applyRequest(ClusterState currentState, PutMappingClusterStateUpdateRequest request,
Map<Index, MapperService> indexMapperServices) throws IOException {
String mappingType = request.type();

CompressedXContent mappingUpdateSource = new CompressedXContent(request.source());
final MetaData metaData = currentState.metaData();
final List<IndexMetaData> updateList = new ArrayList<>();
Expand All @@ -260,32 +257,11 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
updateList.add(indexMetaData);
// try and parse it (no need to add it here) so we can bail early in case of parsing exception
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));
}

DocumentMapper newMapper = mapperService.parse(request.type(), mappingUpdateSource);
if (existingMapper != null) {
// first, simulate: just call merge and ignore the result
existingMapper.merge(newMapper.mapping());
}


if (mappingType == null) {
mappingType = newMapper.type();
} else if (mappingType.equals(newMapper.type()) == false
&& (isMappingSourceTyped(request.type(), mappingUpdateSource)
|| mapperService.resolveDocumentType(mappingType).equals(newMapper.type()) == false)) {
throw new InvalidTypeNameException("Type name provided does not match type name within mapping definition.");
}
}
assert mappingType != null;

if (MapperService.SINGLE_MAPPING_NAME.equals(mappingType) == false
&& mappingType.charAt(0) == '_') {
throw new InvalidTypeNameException("Document mapping type name can't start with '_', found: [" + mappingType + "]");
}
MetaData.Builder builder = MetaData.builder(metaData);
boolean updated = false;
Expand All @@ -296,13 +272,12 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
final Index index = indexMetaData.getIndex();
final MapperService mapperService = indexMapperServices.get(index);

String typeForUpdate = mapperService.getTypeForUpdate(mappingType, mappingUpdateSource);
CompressedXContent existingSource = null;
DocumentMapper existingMapper = mapperService.documentMapper(typeForUpdate);
DocumentMapper existingMapper = mapperService.documentMapper();
if (existingMapper != null) {
existingSource = existingMapper.mappingSource();
}
DocumentMapper mergedMapper = mapperService.merge(typeForUpdate, mappingUpdateSource, MergeReason.MAPPING_UPDATE);
DocumentMapper mergedMapper = mapperService.merge(request.type(), mappingUpdateSource, MergeReason.MAPPING_UPDATE);
Copy link
Contributor

@jtibshirani jtibshirani Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Here we no longer use the resolved type typeForUpdate. This type resolution logic allowed for 'put mapping' to work in the following scenario:

  • An index contains a custom type like my_type.
  • The user adds new mappings using a typeless put mapping call (which uses the _doc type under the hood).

We still need to support this case, because there could be indices carried over from 7.x with a custom type name. I think that CI didn't catch the issue because the relevant 'mix typeful and typeless' tests were removed in #41676. Perhaps we could restore some of those tests, and make sure to preserve the necessary logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type names will be removed entirely for 8x though, so I don't think this issue will arise? The point of this PR is to remove the type resolution checks because we know that an index can only have a single type, so it doesn't matter what it's called. It's a staging commit that means we can remove type information from things like put-mapping or create-index without having to go and change all our tests to use a _doc type, only to then remove the types completely in a follow-up commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @romseygeek, I should have done some tests before writing that comment! The scenario I described actually seems to work, because we completely ignore the type on the request. I don't see a bug, and it sounds like we're on the same page with this PR.

I do wonder if could use more tests around the scenario I described, I'll follow-up on #41676 to discuss.

CompressedXContent updatedSource = mergedMapper.mappingSource();

if (existingSource != null) {
Expand All @@ -321,9 +296,9 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
} else {
updatedMapping = true;
if (logger.isDebugEnabled()) {
logger.debug("{} create_mapping [{}] with source [{}]", index, mappingType, updatedSource);
logger.debug("{} create_mapping with source [{}]", index, updatedSource);
} else if (logger.isInfoEnabled()) {
logger.info("{} create_mapping [{}]", index, mappingType);
logger.info("{} create_mapping", index);
}
}

Expand Down
12 changes: 2 additions & 10 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 @@ -70,7 +70,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 @@ -303,7 +302,8 @@ public void merge(IndexMetaData indexMetaData, MergeReason reason) {
}

public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to remove MapperService#getTypeForUpdate, since it is no longer used.

return internalMerge(Collections.singletonMap(type, mappingSource), reason).get(type);
// TODO change internalMerge() to return a single DocumentMapper rather than a Map
return internalMerge(Collections.singletonMap(type, mappingSource), reason).values().iterator().next();
}

private synchronized Map<String, DocumentMapper> internalMerge(IndexMetaData indexMetaData,
Expand Down Expand Up @@ -373,14 +373,6 @@ private synchronized Map<String, DocumentMapper> internalMerge(DocumentMapper ma

Map<String, DocumentMapper> results = new LinkedHashMap<>(2);

{
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
77 changes: 0 additions & 77 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,11 @@

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 @@ -38,7 +34,6 @@
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 @@ -140,76 +135,4 @@ public void testMappingUpdateAccepts_docAsType() throws Exception {
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
import java.util.Map;

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

public class MapperServiceTests extends ESSingleNodeTestCase {
Expand Down Expand Up @@ -267,36 +266,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 testFieldNameLengthLimit() throws Throwable {
int maxFieldNameLength = randomIntBetween(15, 20);
String testString = new String(new char[maxFieldNameLength + 1]).replace("\0", "a");
Expand Down