Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ambiguous object field name detection into DotExpandingXContentParser #82359

Merged
merged 4 commits into from
Jan 10, 2022
Merged

Move ambiguous object field name detection into DotExpandingXContentParser #82359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d775fd
Move ambiguous object path detection into DotExpandingXContentParser
romseygeek Jan 10, 2022
eb277d3
Merge remote-tracking branch 'origin/master' into mapper/field-name-v…
romseygeek Jan 10, 2022
82ef34c
precommit
romseygeek Jan 10, 2022
019f633
Merge branch 'master' into mapper/field-name-validation
elasticmachine Jan 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion libs/x-content/src/main/java/org/elasticsearch/xcontent/DotExpandingXContentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romseygeek this check is redundant now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, good catch

throw new IllegalArgumentException("field name cannot contain only dots: [" + field + "]");
}
Expand All @@ -71,6 +71,31 @@ protected XContentParser delegate() {
}
}

private static String[] splitAndValidatePath(String fullFieldPath) {
if (fullFieldPath.isEmpty()) {
throw new IllegalArgumentException("field name cannot be an empty string");
}
if (fullFieldPath.contains(".") == false) {
return new String[] { fullFieldPath };
}
String[] parts = fullFieldPath.split("\\.");
if (parts.length == 0) {
throw new IllegalArgumentException("field name cannot contain only dots");
}
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 + "']");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think that's a leftover, and it was probably inaccurate even before this change. It should be 'field name'

}
if (part.isBlank()) {
throw new IllegalArgumentException(
"object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]"
);
}
}
return parts;
}

/**
* Wraps an XContentParser such that it re-interprets dots in field names as an object structure
* @param in the parser to wrap
Expand Down
2 changes: 0 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok then I guess I am unclear on why the original method stays around, are there still places where we call it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep it's still called from createDynamicUpdate(). This gets completely refactored in #81449 so it will go away entirely there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks! Can we in the meantime share the code between the two then, or are there subtle differences?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

} else if (token == XContentParser.Token.START_OBJECT) {
parseObject(context, mapper, currentFieldName);
} else if (token == XContentParser.Token.START_ARRAY) {
Expand Down Expand Up @@ -649,7 +648,6 @@ private static void parseNonDynamicArray(
) throws IOException {
XContentParser parser = context.parser();
XContentParser.Token token;
splitAndValidatePath(lastFieldName);
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
if (token == XContentParser.Token.START_OBJECT) {
parseObject(context, mapper, lastFieldName);
Expand Down