Add explicit tests for dynamic template validation on bad mapping parameters #74177
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
romseygeek
added
>non-issue
:Search Foundations/Mapping
Collaborator
|
Pinging @elastic/es-search (Team:Search) |
javanna
approved these changes
Jun 16, 2021
| Map<String, Object> mapping = dynamicTemplate.mappingForName(name, dynamicType); | ||
| if (dynamicTemplate.isRuntimeMapping()) { | ||
| Mapper.TypeParser.ParserContext parserContext = context.parserContext(dateFormatter); | ||
| parserContext = parserContext.createDynamicTemplateFieldContext(parserContext); |
Member
There was a problem hiding this comment.
oh boy this is super sneaky. I wonder how we can prevent this from happening again in the long run.
Contributor
Author
There was a problem hiding this comment.
Actually I think ParseContext.parserContext() is only ever called in the context of dynamic fields, so we can probably move this directly into that method. Will give that a go!
Member
There was a problem hiding this comment.
I found the createDynamicTemplateFieldContext more misleading, because it returns again a parser context and it's' very easy to miss.
Contributor
Author
There was a problem hiding this comment.
I've pushed a change which makes it unnecessary - let me know what you think
javanna
reviewed
Jun 16, 2021
Member
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
thanks for the iteration, I had another question about it.
|
|
||
| ParserContext createDynamicTemplateFieldContext(ParserContext in) { | ||
| return new DynamicTemplateParserContext(in); | ||
| ParserContext createDynamicTemplateFieldContext() { |
Member
There was a problem hiding this comment.
Is this the only usage of the parser context function? should we change the function argument directly to create the dynamic parser context directly instead of going through a parser context to create another one?
Contributor
Author
There was a problem hiding this comment.
It's also used when validating the templates where we do want to throw an error rather than issuing warnings, so we need to be able to distinguish between the two cases - hence the extra step.
romseygeek
added a commit
that referenced
this pull request
Jun 17, 2021
…ameters (#74177) When a dynamic template is applied to a 7.x index, we do some validation checks against the generated mappings and issue a deprecation warning if the validation fails. We had some tests that were checking this at a low level, but nothing that exercised the full logic. When dynamic runtimes were added, we expected this behaviour to carry over; however, a bug in building the ParserContext to pass to the runtime Builder meant that we would instead always throw an error. In fact, erroring here is a good result, as the only reason we issue a deprecation warning for non-runtime fields is to preserve backward compatibility; given that runtime fields are new, it has never been possible to add a bad dynamic runtime template. This commit adds specific tests for both situations. It ensures that the parser context passed to a runtime builder will know that it is in a dynamic context, but it also removes the version check and deprecation warning, as they were never actually triggered and we can be stricter here.
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Jun 18, 2021
DocumentParser takes care of parsing documents and applying dynamic mapping updates for each new field it encounters, if applicable, as well as applying the matching dynamic templates. To do this, it needs to be able to parse mappings, which requires a ParserContext that the ParseContext exposes given a DateFormatter. So far, we have carried around a function that allows to create a new ParserContext given a date formatter, and then call a specific method that allows to wrap it to provide a special parser context that is specific for dynamic templates, which is needed to make some decisions down the line based on whether mappings come from dynamic templates or not. This commit tries to simplify this by exposing the specific dynamic template parser context only to the document parse context, as that's all it needs. We recently ended up using the wrong context and with this change it would not be possible. The function that is passed around is the one that create a DynamicTemplateParserContext, so it cannot be mistaken for the one that creates a generic ParserContext. Relates to elastic#74177
javanna
added a commit
that referenced
this pull request
Jun 21, 2021
DocumentParser takes care of parsing documents and applying dynamic mapping updates for each new field it encounters, if applicable, as well as applying the matching dynamic templates. To do this, it needs to be able to parse mappings, which requires a ParserContext that the ParseContext exposes given a DateFormatter. So far, we have carried around a function that allows to create a new ParserContext given a date formatter, and then call a specific method that allows to wrap it to provide a special parser context that is specific for dynamic templates, which is needed to make some decisions down the line based on whether mappings come from dynamic templates or not. This commit tries to simplify this by exposing the specific dynamic template parser context only to the document parse context, as that's all it needs. We recently ended up using the wrong context and with this change it would not be possible. Relates to #74177
javanna
added a commit
that referenced
this pull request
Jun 21, 2021
DocumentParser takes care of parsing documents and applying dynamic mapping updates for each new field it encounters, if applicable, as well as applying the matching dynamic templates. To do this, it needs to be able to parse mappings, which requires a ParserContext that the ParseContext exposes given a DateFormatter. So far, we have carried around a function that allows to create a new ParserContext given a date formatter, and then call a specific method that allows to wrap it to provide a special parser context that is specific for dynamic templates, which is needed to make some decisions down the line based on whether mappings come from dynamic templates or not. This commit tries to simplify this by exposing the specific dynamic template parser context only to the document parse context, as that's all it needs. We recently ended up using the wrong context and with this change it would not be possible. Relates to #74177
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
When a dynamic template is applied to a 7.x index, we do some validation checks
against the generated mappings and issue a deprecation warning if the validation
fails. We had some tests that were checking this at a low level, but nothing that
exercised the full logic.
When dynamic runtimes were added, we expected this behaviour to carry over;
however, a bug in building the ParserContext to pass to the runtime Builder meant
that we would instead always throw an error. In fact, erroring here is a good result,
as the only reason we issue a deprecation warning for non-runtime fields is to
preserve backward compatibility; given that runtime fields are new, it has never
been possible to add a bad dynamic runtime template.
This commit adds specific tests for both situations. It ensures that the parser
context passed to a runtime builder will know that it is in a dynamic context,
but it also removes the version check and deprecation warning, as they were
never actually triggered and we can be stricter here.