Disallow changing 'enabled' on the root mapper. (#54681)

In #33933 we disallowed changing the `enabled` parameter in object mappings.
However, the fix didn't cover the root object mapper. This PR adjusts the change
to also include the root mapper and clarifies the error message.
This commit is contained in:
Julie Tibshirani 2020-04-02 15:28:48 -07:00 committed by GitHub
parent 39c4ec6821
commit 5fb7602227
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 28 additions and 17 deletions

View File

@ -18,6 +18,19 @@ coming[7.8.0]
//end::notable-breaking-changes[]
[discrete]
[[breaking_78_mappings_changes]]
=== Mappings changes
[discrete]
[[prevent-enabled-setting-change]]
==== `enabled` setting cannot be changed on root mapper
Previously, Elasticsearch accepted mapping updates that tried to change the
`enabled` setting on the root mapping. The update was not applied, but the
request didn't throw an error. As of 7.8, requests that attempt to change
`enabled` on the root mapping will fail.
[discrete]
[[breaking_78_settings_changes]]
=== Settings changes

View File

@ -459,9 +459,12 @@ public class ObjectMapper extends Mapper implements Cloneable {
this.dynamic = mergeWith.dynamic;
}
if (isEnabled() != mergeWith.isEnabled()) {
throw new MapperException("The [enabled] parameter can't be updated for the object mapping [" + name() + "].");
}
for (Mapper mergeWithMapper : mergeWith) {
Mapper mergeIntoMapper = mappers.get(mergeWithMapper.simpleName());
checkEnabledFieldChange(mergeWith, mergeWithMapper, mergeIntoMapper);
Mapper merged;
if (mergeIntoMapper == null) {
@ -475,18 +478,6 @@ public class ObjectMapper extends Mapper implements Cloneable {
}
}
private static void checkEnabledFieldChange(ObjectMapper mergeWith, Mapper mergeWithMapper, Mapper mergeIntoMapper) {
if (mergeIntoMapper instanceof ObjectMapper && mergeWithMapper instanceof ObjectMapper) {
final ObjectMapper mergeIntoObjectMapper = (ObjectMapper) mergeIntoMapper;
final ObjectMapper mergeWithObjectMapper = (ObjectMapper) mergeWithMapper;
if (mergeIntoObjectMapper.isEnabled() != mergeWithObjectMapper.isEnabled()) {
final String path = mergeWith.fullPath() + "." + mergeWithObjectMapper.simpleName() + ".enabled";
throw new MapperException("Can't update attribute for type [" + path + "] in index mapping");
}
}
}
@Override
public ObjectMapper updateFieldType(Map<String, MappedFieldType> fullNameToFieldType) {
List<Mapper> updatedMappers = null;

View File

@ -83,7 +83,7 @@ public class ObjectMapperMergeTests extends ESTestCase {
// WHEN merging mappings
// THEN a MapperException is thrown with an excepted message
MapperException e = expectThrows(MapperException.class, () -> rootObjectMapper.merge(mergeWith));
assertEquals("Can't update attribute for type [type1.foo.enabled] in index mapping", e.getMessage());
assertEquals("The [enabled] parameter can't be updated for the object mapping [foo].", e.getMessage());
}
public void testMergeWhenEnablingField() {
@ -93,7 +93,16 @@ public class ObjectMapperMergeTests extends ESTestCase {
// WHEN merging mappings
// THEN a MapperException is thrown with an excepted message
MapperException e = expectThrows(MapperException.class, () -> rootObjectMapper.merge(mergeWith));
assertEquals("Can't update attribute for type [type1.disabled.enabled] in index mapping", e.getMessage());
assertEquals("The [enabled] parameter can't be updated for the object mapping [disabled].", e.getMessage());
}
public void testDisableRootMapper() {
String type = MapperService.SINGLE_MAPPING_NAME;
ObjectMapper firstMapper = createRootObjectMapper(type, true, Collections.emptyMap());
ObjectMapper secondMapper = createRootObjectMapper(type, false, Collections.emptyMap());
MapperException e = expectThrows(MapperException.class, () -> firstMapper.merge(secondMapper));
assertEquals("The [enabled] parameter can't be updated for the object mapping [" + type + "].", e.getMessage());
}
private static RootObjectMapper createRootObjectMapper(String name, boolean enabled, Map<String, Mapper> mappers) {

View File

@ -46,7 +46,6 @@ import org.elasticsearch.index.engine.Engine;
import org.elasticsearch.index.engine.EngineException;
import org.elasticsearch.index.engine.InternalEngineFactory;
import org.elasticsearch.index.fieldvisitor.FieldsVisitor;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.SeqNoFieldMapper;
import org.elasticsearch.index.mapper.SourceToParse;
import org.elasticsearch.index.mapper.Uid;
@ -237,7 +236,6 @@ public class SourceOnlySnapshotShardTests extends IndexShardTestCase {
IndexMetadata metadata = runAsSnapshot(threadPool, () -> repository.getSnapshotIndexMetadata(snapshotId, indexId));
IndexShard restoredShard = newShard(
shardRouting, metadata, null, SourceOnlySnapshotRepository.getEngineFactory(), () -> {}, RetentionLeaseSyncer.EMPTY);
restoredShard.mapperService().merge(shard.indexSettings().getIndexMetadata(), MapperService.MergeReason.MAPPING_RECOVERY);
DiscoveryNode discoveryNode = new DiscoveryNode("node_g", buildNewFakeTransportAddress(), Version.CURRENT);
restoredShard.markAsRecovering("test from snap", new RecoveryState(restoredShard.routingEntry(), discoveryNode, null));
runAsSnapshot(shard.getThreadPool(), () -> {