Skip to content

Commit

Permalink
Disallow changing 'enabled' on the root mapper. (#54681)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jtibshirani authored Apr 2, 2020
1 parent 39c4ec6 commit 5fb7602
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 17 deletions.
13 changes: 13 additions & 0 deletions docs/reference/migration/migrate_7_8.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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<String, MappedFieldType> fullNameToFieldType) {
List<Mapper> updatedMappers = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(), () -> {
Expand Down

0 comments on commit 5fb7602

Please sign in to comment.