From a4b87349916612d41e60fb768b8201f1a88a4c54 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Thu, 2 Apr 2020 12:49:53 -0700 Subject: [PATCH 1/2] 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 fe60e1b62d9d6..fd4f21b194b35 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -459,9 +459,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) { @@ -475,18 +478,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 6800bed859e3a..92b2d1a620a06 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; @@ -237,7 +236,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(), () -> { From 7f8d79ad688c4690f3b63a5e9ac6326029e1e2b2 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Thu, 2 Apr 2020 14:32:45 -0700 Subject: [PATCH 2/2] Add a note to the breaking changes docs. --- docs/reference/migration/migrate_7_8.asciidoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/reference/migration/migrate_7_8.asciidoc b/docs/reference/migration/migrate_7_8.asciidoc index bbaf21ebfcc1b..9de24cab9e573 100644 --- a/docs/reference/migration/migrate_7_8.asciidoc +++ b/docs/reference/migration/migrate_7_8.asciidoc @@ -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