Simplify dynamic template parser context creation #74326
Conversation
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
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
|
||
| @Override | ||
| public Mapper.TypeParser.ParserContext dynamicTemplateParserContext(DateFormatter dateFormatter) { | ||
| public Mapper.TypeParser.ParserContext.DynamicTemplateParserContext dynamicTemplateParserContext(DateFormatter dateFormatter) { |
There was a problem hiding this comment.
For conciseness, I would consider renaming this method back to parserContext now that its return type is specific
There was a problem hiding this comment.
Alternatively, I think the type of this method can stay as just Mapper.TypeParser.ParserContext? And then DynamicTemplateParserContext can become a top-level package-private class and we lose some of the crazy nesting :)
There was a problem hiding this comment.
Counter proposal: shall we get ParserContext out of Mapper.TypeParser completely as a follow-up? it's used for runtime fields too so it is not specific to Mappers, and we could rename it to to something that is less confusing compared to ParseContext that is instead used for documents.
There was a problem hiding this comment.
shall we get ParserContext out of Mapper.TypeParser completely as a follow-up?
Strong +1 from me! I think we can still leave this method return type untouched though?
There was a problem hiding this comment.
Ok good discussion: changing the type makes sure that the wrong context cannot be provided, and helps readers understand that when parsing dynamic templates only the appropriate context can be used.
I guess I see how it can be cleaner and tempting to use the more generic type, but what are the advantages in this case compared to the confusion that it adds in that it is hard to follow and very easy to miss?
There was a problem hiding this comment.
My argument would be that: we only have a single concrete implementation of ParseContext (and are unlikely to add more) so I don't think we need to worry about returning an incorrect type elsewhere; that the name of the method describes the fact that it's specifically for dynamic templates; in general it's best to return interfaces rather than implementations; and that having four layers of nested class name makes my eyes hurt.
There was a problem hiding this comment.
the nesting I agree on. My point is that with keeping the type generic we may end up making the same mistake that got us here aka carrying around the wrong context. The name of a method does not enforce anything, while the return type does. I agree on the software engineering guidelines around using interfaces but they also introduce complexity. The important bit is that parsing mappings requires a ParserContext, which is the case. But we can clarify that when we parse as part of dynamic templates, the context that is provided will be specific, we don't need to keep things overly generic everywhere as that brings no benefits?
There was a problem hiding this comment.
The previous bug was because you had do to two things: 1) get a new parser context from the ParseContext, and 2) convert it to a dynamic context. This commit merges the two, so that what you get back from 1) is always a dynamic context. There's no way of getting a parser context from ParseContext that isn't dynamic. So I don't see how we could make the same mistake again?
There was a problem hiding this comment.
I feel like it is still a bit delicate if we use the more generic type, we may change that due to refactoring, not too sure. Maybe I am being a bit paranoid but this is still a bit sketchy. Anyways, I will do as you ask.
| return new DynamicTemplateParserContext(this); | ||
| } | ||
|
|
||
| private static class MultiFieldParserContext extends ParserContext { |
There was a problem hiding this comment.
we may want to look into taking the same approach with MultiFieldParseContext, to prevent mistakes in the future.
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
Nice, I tried to do this earlier and got mixed up with the different functions in the MapperService constructor but this cleans things up. I left a suggestion about class/package structure.
|
|
||
| @Override | ||
| public Mapper.TypeParser.ParserContext dynamicTemplateParserContext(DateFormatter dateFormatter) { | ||
| public Mapper.TypeParser.ParserContext.DynamicTemplateParserContext dynamicTemplateParserContext(DateFormatter dateFormatter) { |
There was a problem hiding this comment.
Alternatively, I think the type of this method can stay as just Mapper.TypeParser.ParserContext? And then DynamicTemplateParserContext can become a top-level package-private class and we lose some of the crazy nesting :)
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
4 participants
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