Move ambiguous object field name detection into DotExpandingXContentParser #82359
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
| @@ -461,7 +461,6 @@ private static void innerParseObject(DocumentParserContext context, ObjectMapper | |||
| while (token != XContentParser.Token.END_OBJECT) { | |||
| if (token == XContentParser.Token.FIELD_NAME) { | |||
| currentFieldName = context.parser().currentName(); | |||
| splitAndValidatePath(currentFieldName); | |||
There was a problem hiding this comment.
is it ok to omit this call now? What was it doing and where is that method called now? Seems like the new method incorporates some of its logic?
There was a problem hiding this comment.
The new method is pretty much a straight copy. It's called in the DotExpandingXContentParser, which InternalDocumentParserContext creates to wrap the source document's xcontent parser. The existing call sites were places where we previously split up field names, but that all happens in the expanding parser now so it makes more sense to handle things there.
There was a problem hiding this comment.
ok then I guess I am unclear on why the original method stays around, are there still places where we call it?
There was a problem hiding this comment.
yep it's still called from createDynamicUpdate(). This gets completely refactored in #81449 so it will go away entirely there.
There was a problem hiding this comment.
I see, thanks! Can we in the meantime share the code between the two then, or are there subtle differences?
There was a problem hiding this comment.
They are slightly different because the x-content lib doesn't have access to Strings, but in implementation they are the same. Given that they're private static though, and that the copy in DocumentParser will be removed immediately in a follow up, I'd prefer to keep two versions for the moment?
|
@elasticmachine run elasticsearch-ci/docs |
|
@elasticmachine update branch |
| for (String part : parts) { | ||
| // check if the field name contains only whitespace | ||
| if (part.isEmpty()) { | ||
| throw new IllegalArgumentException("object field cannot contain only whitespace: ['" + fullFieldPath + "']"); |
There was a problem hiding this comment.
@romseygeek is the mention of "object" in this error and the next one accurate? Don't we split any path regardless of the token we are reading from the parser?
There was a problem hiding this comment.
Yes I think that's a leftover, and it was probably inaccurate even before this change. It should be 'field name'
| @@ -48,7 +48,7 @@ public Token nextToken() throws IOException { | |||
|
|
|||
| private void expandDots() throws IOException { | |||
| String field = delegate().currentName(); | |||
| String[] subpaths = field.split("\\."); | |||
| String[] subpaths = splitAndValidatePath(field); | |||
| if (subpaths.length == 0) { | |||
There was a problem hiding this comment.
@romseygeek this check is redundant now?
There was a problem hiding this comment.
It is, good catch
Detecting when a field name contains double dots, or starts with a dot, is currently
done by the
splitAndValidatePathmethod on DocumentParser. However it makesmore sense to do this as part of the DotExpandingXContentParser which actually
does the work of converting field names containing dots to XContent objects.