From 150065182ebc02e62ce81cbf7a7886ab1bcf89af Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Thu, 2 Apr 2020 12:49:53 -0700 Subject: [PATCH] Disallow changing 'enabled' on the root mapper. (#54463) 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. --- .../index/mapper/ObjectMapper.java | 17 ++++------------- .../index/mapper/ObjectMapperMergeTests.java | 13 +++++++++++-- .../snapshots/SourceOnlySnapshotShardTests.java | 2 -- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 157988dab3709..b1ec7d0361cb4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -465,9 +465,12 @@ protected void doMerge(final ObjectMapper mergeWith) { 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) { @@ -481,18 +484,6 @@ protected void doMerge(final ObjectMapper mergeWith) { } } - 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 fullNameToFieldType) { List updatedMappers = null; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index cae799473da12..a2f62cf02447c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -83,7 +83,7 @@ public void testMergeWhenDisablingField() { // 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 void testMergeWhenEnablingField() { // 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 mappers) { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotShardTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotShardTests.java index 3e7ee8ff121c1..37371d9cdeb91 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotShardTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotShardTests.java @@ -46,7 +46,6 @@ 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; @@ -236,7 +235,6 @@ public void testRestoreMinmal() throws IOException { 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(), () -> {