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

In elastic#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.
jasontedor · Apr 2, 2020 · 1500651 · 1500651
1 parent 0695417
commit 1500651
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 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
@@ -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<String, MappedFieldType> fullNameToFieldType) {
         List<Mapper> 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
@@ -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<String, Mapper> mappers) {

diff --git a/...k/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotShardTests.java b/...k/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(), () -> {