Disallow changing 'enabled' on the root mapper. #54463
Conversation
jtibshirani
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
jtibshirani
force-pushed
the
root-mapper-enabled
branch
from
4982bd6 to
ea9433a
Compare
| 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()); |
There was a problem hiding this comment.
It's pretty confusing that this error message will contain the singleton type name: "The [enabled] parameter can't be updated for the object mapping [_doc]." I think we should address this in a separate, more general change to not pass in the type as the root object mapper's name.
jtibshirani
force-pushed
the
root-mapper-enabled
branch
from
625a312 to
16ddeb9
Compare
jtibshirani
force-pushed
the
root-mapper-enabled
branch
from
16ddeb9 to
df69ea7
Compare
| @@ -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); | |||
There was a problem hiding this comment.
I'll double check with someone from the distributed team that this change is okay.
There was a problem hiding this comment.
@original-brownbear would you be up for taking a look? I saw that you made several changes in this area recently.
There was a problem hiding this comment.
I think this is fine. This line seems unnecessary and makes the test behave less like a real restore as far as I can see anyway (just restoring the meta from the repo in a real restore).
=> LGTM :)
There was a problem hiding this comment.
Thanks @original-brownbear ! It also seemed strange to me, that we would add the original mappings as part of restoring a source-only snapshot.
|
Although it could be considered breaking, I plan to backport this to 7.8. This matches how we handled #33933, which prevented changing |
In #33933 we disallowed changing the
enabledparameter 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.