Check mapping compatibility up-front. #15123
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have a bug when updating an existing mapping and the mapping update conflicts with another type. In such a case, the mapping is already partially applied before we raise the exception, which can lead to all sorts of bugs. This commit moves the compatibility checks up-front so that we don't start applying a mapping before we are sure that it is not only applicable to the current type but also to other types. Note that this might break compatibility for some users: we had some tests that checked that fields that did not have a conflict were added even though other fields from the same mapping update were not compatible. This does not work anymore. MapperService.merge has been refactored to take a map of mapping updates so that we can perform some cross-type validation (necessary for _parent) that was previously spread in-between MapperService and MetaDataMappingService. We now also verify that fields are defined only once in a given mapping. It was previously possible to have a field twice either by reusing the name of a metadata mapper or by using subfields both explicitly in a plugin and in the code of a field mapper. The check that _parent can't point to an existing type would only be actually applied if the master node has mappings locally. So for instance in case of dedicated master nodes, it would be ignored. This is fixed now by adding mappings from all types when an index does not exist locally, instead of only mappings from the types that are being updated.
jpountz
added
>bug
:Search Foundations/Mapping
Member
There was a problem hiding this comment.
Maybe we could avoid compressing just to pass to the mapper service? Seems like the api for merge could take non compressed and we can use helpers to wrap a decompressor when we already have compressed? Just a thought for a spinoff issue (Ive had it on my mind for a while).
Contributor
Author
There was a problem hiding this comment.
Agreed, there are so many places where we compress just to be able to merge the mapping!
Member
|
This looks great! Just left a couple questions, but as is this change looks good to me to push. |
Contributor
Author
Contributor
Author
|
Here is a PR to fix an issue with cross-type validation for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have a bug when updating an existing mapping and the mapping update conflicts
with another type. In such a case, the mapping is already partially applied
before we raise the exception, which can lead to all sorts of bugs.
This commit moves the compatibility checks up-front so that we don't start
applying a mapping before we are sure that it is not only applicable to the
current type but also to other types.
Note that this might break compatibility for some users: we had some tests
that checked that fields that did not have a conflict were added even though
other fields from the same mapping update were not compatible. This does not
work anymore.
MapperService.merge has been refactored to take a map of mapping updates so
that we can perform some cross-type validation (necessary for _parent) that
was previously spread in-between MapperService and MetaDataMappingService.
We now also verify that fields are defined only once in a given mapping. It
was previously possible to have a field twice either by reusing the name of
a metadata mapper or by using subfields both explicitly in a plugin and in
the code of a field mapper.
The check that _parent can't point to an existing type would only be actually
applied if the master node has mappings locally. So for instance in case of
dedicated master nodes, it would be ignored. This is fixed now by adding
mappings from all types when an index does not exist locally, instead of only
mappings from the types that are being updated.