Remove typename checks in mapping updates#47347
Conversation
romseygeek
added
>non-issue
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
| existingSource = existingMapper.mappingSource(); | ||
| } | ||
| DocumentMapper mergedMapper = mapperService.merge(typeForUpdate, mappingUpdateSource, MergeReason.MAPPING_UPDATE); | ||
| DocumentMapper mergedMapper = mapperService.merge(request.type(), mappingUpdateSource, MergeReason.MAPPING_UPDATE); |
There was a problem hiding this comment.
Here we no longer use the resolved type typeForUpdate. This type resolution logic allowed for 'put mapping' to work in the following scenario:
- An index contains a custom type like
my_type. - The user adds new mappings using a typeless put mapping call (which uses the
_doctype under the hood).
We still need to support this case, because there could be indices carried over from 7.x with a custom type name. I think that CI didn't catch the issue because the relevant 'mix typeful and typeless' tests were removed in #41676. Perhaps we could restore some of those tests, and make sure to preserve the necessary logic here.
There was a problem hiding this comment.
Type names will be removed entirely for 8x though, so I don't think this issue will arise? The point of this PR is to remove the type resolution checks because we know that an index can only have a single type, so it doesn't matter what it's called. It's a staging commit that means we can remove type information from things like put-mapping or create-index without having to go and change all our tests to use a _doc type, only to then remove the types completely in a follow-up commit.
There was a problem hiding this comment.
Sorry @romseygeek, I should have done some tests before writing that comment! The scenario I described actually seems to work, because we completely ignore the type on the request. I don't see a bug, and it sounds like we're on the same page with this PR.
I do wonder if could use more tests around the scenario I described, I'll follow-up on #41676 to discuss.
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
This looks good to me.
| @@ -303,7 +302,8 @@ public void merge(IndexMetaData indexMetaData, MergeReason reason) { | |||
| } | |||
|
|
|||
| public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { | |||
There was a problem hiding this comment.
It would be good to remove MapperService#getTypeForUpdate, since it is no longer used.
|
@elasticmachine update branch |
4 participants
This commit removes types validation during mapping updates. This will make
further work on types removal easier, as it will prevent test failures due to type-name
clashes when we remove type information from PutMapping and CreateIndex requests
Part of #41059