Skip to content

Adjust DotExpandingXContentParser behaviour #83313

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

Merged
merged 7 commits into from
Jan 31, 2022
Merged

Adjust DotExpandingXContentParser behaviour #83313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4b19cff
Adjust DotExpandingXContentParser behaviour
javanna Jan 31, 2022
609b0d1
Update docs/changelog/83313.yaml
javanna Jan 31, 2022
586db02
Delete 83313.yaml
javanna Jan 31, 2022
836efd5
remove leftover
javanna Jan 31, 2022
ddebf13
address test
javanna Jan 31, 2022
11f8092
Merge branch 'main' into fix/dot_expanding_parser
javanna Jan 31, 2022
381bd87
address test
javanna Jan 31, 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
99 changes: 57 additions & 42 deletions 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 @@ -89,11 +89,11 @@ private static String[] splitAndValidatePath(String fullFieldPath) {
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 + "']");
throw new IllegalArgumentException("field name cannot contain only whitespace: ['" + fullFieldPath + "']");
}
if (part.isBlank()) {
throw new IllegalArgumentException(
"object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]"
"field name starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]"
);
}
}
Expand All @@ -110,16 +110,17 @@ public static XContentParser expandDots(XContentParser in) throws IOException {
}

private enum State {
PRE,
DURING,
POST
EXPANDING_START_OBJECT,
PARSING_ORIGINAL_CONTENT,
ENDING_EXPANDED_OBJECT
}

final String[] subPaths;
final XContentParser subparser;

int level = 0;
private State state = State.PRE;
private int expandedTokens = 0;
private int innerLevel = -1;
private State state = State.EXPANDING_START_OBJECT;

private DotExpandingXContentParser(XContentParser subparser, XContentParser root, String[] subPaths) {
super(root);
Expand All @@ -129,88 +130,102 @@ private DotExpandingXContentParser(XContentParser subparser, XContentParser root

@Override
public Token nextToken() throws IOException {
if (state == State.PRE) {
level++;
if (level == subPaths.length * 2 - 1) {
state = State.DURING;
return in.currentToken();
if (state == State.EXPANDING_START_OBJECT) {
expandedTokens++;
assert expandedTokens < subPaths.length * 2;
if (expandedTokens == subPaths.length * 2 - 1) {
state = State.PARSING_ORIGINAL_CONTENT;
Token token = in.currentToken();
if (token == Token.START_OBJECT || token == Token.START_ARRAY) {
innerLevel++;
}
return token;
}
if (level % 2 == 0) {
// The expansion consists of adding pairs of START_OBJECT and FIELD_NAME tokens
if (expandedTokens % 2 == 0) {
return Token.FIELD_NAME;
}
return Token.START_OBJECT;
}
if (state == State.DURING) {
if (state == State.PARSING_ORIGINAL_CONTENT) {
Token token = subparser.nextToken();
if (token == Token.START_OBJECT || token == Token.START_ARRAY) {
innerLevel++;
}
if (token == Token.END_OBJECT || token == Token.END_ARRAY) {
innerLevel--;
}
if (token != null) {
return token;
}
state = State.POST;
}
assert state == State.POST;
if (level >= 1) {
level -= 2;
state = State.ENDING_EXPANDED_OBJECT;
}
return level < 0 ? null : Token.END_OBJECT;
assert expandedTokens % 2 == 1;
expandedTokens -= 2;
return expandedTokens < 0 ? null : Token.END_OBJECT;
}

@Override
public Token currentToken() {
if (state == State.PRE) {
return level % 2 == 1 ? Token.START_OBJECT : Token.FIELD_NAME;
}
if (state == State.POST) {
if (level > 1) {
return Token.END_OBJECT;
}
}
return in.currentToken();
return switch (state) {
case EXPANDING_START_OBJECT -> expandedTokens % 2 == 1 ? Token.START_OBJECT : Token.FIELD_NAME;
case ENDING_EXPANDED_OBJECT -> Token.END_OBJECT;
case PARSING_ORIGINAL_CONTENT -> in.currentToken();
};
}

@Override
public String currentName() throws IOException {
if (state == State.DURING) {
return in.currentName();
}
if (state == State.POST) {
if (level <= 1) {
if (state == State.PARSING_ORIGINAL_CONTENT) {
assert expandedTokens == subPaths.length * 2 - 1;
// whenever we are parsing some inner object/array we can easily delegate to the inner parser
// e.g. field.with.dots: { obj:{ parsing here } }
if (innerLevel > 0) {
return in.currentName();
}
Token token = currentToken();
// if we are parsing the outer object/array, only at the start object/array we need to return
// e.g. dots instead of field.with.dots otherwise we can simply delegate to the inner parser
// which will do the right thing
if (innerLevel == 0 && token != Token.START_OBJECT && token != Token.START_ARRAY) {
return in.currentName();
}
throw new IllegalStateException("Can't get current name during END_OBJECT");
// note that innerLevel can be -1 if there are no inner object/array e.g. field.with.dots: value
// as well as while there is and we are parsing their END_OBJECT or END_ARRAY
}
return subPaths[level / 2];
return subPaths[expandedTokens / 2];
}

@Override
public void skipChildren() throws IOException {
if (state == State.PRE) {
if (state == State.EXPANDING_START_OBJECT) {
in.skipChildren();
state = State.POST;
state = State.ENDING_EXPANDED_OBJECT;
}
if (state == State.DURING) {
if (state == State.PARSING_ORIGINAL_CONTENT) {
subparser.skipChildren();
}
}

@Override
public String textOrNull() throws IOException {
if (state == State.PRE) {
if (state == State.EXPANDING_START_OBJECT) {
throw new IllegalStateException("Can't get text on a " + currentToken() + " at " + getTokenLocation());
}
return super.textOrNull();
}

@Override
public Number numberValue() throws IOException {
if (state == State.PRE) {
if (state == State.EXPANDING_START_OBJECT) {
throw new IllegalStateException("Can't get numeric value on a " + currentToken() + " at " + getTokenLocation());
}
return super.numberValue();
}

@Override
public boolean booleanValue() throws IOException {
if (state == State.PRE) {
if (state == State.EXPANDING_START_OBJECT) {
throw new IllegalStateException("Can't get boolean value on a " + currentToken() + " at " + getTokenLocation());
}
return super.booleanValue();
Expand Down
82 changes: 78 additions & 4 deletions libs/x-content/src/test/java/org/elasticsearch/xcontent/DotExpandingXContentParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,23 @@

public class DotExpandingXContentParserTests extends ESTestCase {

private void assertXContentMatches(String expected, String actual) throws IOException {
XContentParser inputParser = createParser(JsonXContent.jsonXContent, actual);
private void assertXContentMatches(String dotsExpanded, String withDots) throws IOException {
XContentParser inputParser = createParser(JsonXContent.jsonXContent, withDots);
XContentParser expandedParser = DotExpandingXContentParser.expandDots(inputParser);

XContentBuilder actualOutput = XContentBuilder.builder(JsonXContent.jsonXContent).copyCurrentStructure(expandedParser);
assertEquals(expected, Strings.toString(actualOutput));
assertEquals(dotsExpanded, Strings.toString(actualOutput));

XContentParser expectedParser = createParser(JsonXContent.jsonXContent, dotsExpanded);
XContentParser actualParser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, withDots));
XContentParser.Token currentToken;
while ((currentToken = actualParser.nextToken()) != null) {
assertEquals(currentToken, expectedParser.nextToken());
assertEquals(expectedParser.currentToken(), actualParser.currentToken());
assertEquals(actualParser.currentToken().name(), expectedParser.currentName(), actualParser.currentName());

}
assertNull(expectedParser.nextToken());
}

public void testEmbeddedObject() throws IOException {
Expand All @@ -33,7 +44,16 @@ public void testEmbeddedObject() throws IOException {
""");
}

public void testEmbeddedArray() throws IOException {
public void testEmbeddedObjects() throws IOException {

assertXContentMatches("""
{"test":{"with":{"dots":{"obj":{"field":"value","array":["value",{"field":"value"}]}}}},"nodots":"value2"}\
""", """
{"test.with.dots":{"obj":{"field":"value","array":["value",{"field":"value"}]}},"nodots":"value2"}\
""");
}

public void testEmbeddedArrayOfValues() throws IOException {

assertXContentMatches("""
{"test":{"with":{"dots":["field","value"]}},"nodots":"value2"}\
Expand All @@ -43,6 +63,26 @@ public void testEmbeddedArray() throws IOException {

}

public void testEmbeddedArrayOfObjects() throws IOException {

assertXContentMatches("""
{"test":{"with":{"dots":[{"field":"value"},{"field":"value"}]}},"nodots":"value2"}\
""", """
{"test.with.dots":[{"field":"value"},{"field":"value"}],"nodots":"value2"}\
""");

}

public void testEmbeddedArrayMixedContent() throws IOException {

assertXContentMatches("""
{"test":{"with":{"dots":["value",{"field":"value"}]}},"nodots":"value2"}\
""", """
{"test.with.dots":["value",{"field":"value"}],"nodots":"value2"}\
""");

}

public void testEmbeddedValue() throws IOException {

assertXContentMatches("""
Expand Down Expand Up @@ -85,6 +125,40 @@ public void testSkipChildren() throws IOException {
assertNull(parser.nextToken());
}

public void testSkipChildrenWithinInnerObject() throws IOException {
XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
{ "test.with.dots" : {"obj" : {"field":"value"}}, "nodots" : "value2" }"""));

parser.nextToken(); // start object
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
assertEquals("test", parser.currentName());
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.START_OBJECT, parser.currentToken());
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
assertEquals("with", parser.currentName());
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.START_OBJECT, parser.currentToken());
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
assertEquals("dots", parser.currentName());
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.START_OBJECT, parser.currentToken());
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
assertEquals("obj", parser.currentName());
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.START_OBJECT, parser.currentToken());
parser.skipChildren();
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
assertEquals("nodots", parser.currentName());
assertEquals(XContentParser.Token.VALUE_STRING, parser.nextToken());
assertEquals("value2", parser.text());
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
assertNull(parser.nextToken());
}

public void testNestedExpansions() throws IOException {
assertXContentMatches("""
{"first":{"dot":{"second":{"dot":"value"},"third":"value"}},"nodots":"value"}\
Expand Down
2 changes: 1 addition & 1 deletion server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ public void testDynamicRuntimeObjectFields() {
() -> client().prepareIndex("test").setSource("obj.runtime", "value").get()
);
assertEquals(
"object mapping for [obj.runtime] tried to parse field [obj.runtime] as object, but found a concrete value",
"object mapping for [obj.runtime] tried to parse field [runtime] as object, but found a concrete value",
exception.getMessage()
);

Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1787,7 +1787,7 @@ public void testDynamicFieldsStartingAndEndingWithDot() throws Exception {
{"top..foo.":{"a":1}}
""")));

assertThat(e.getCause().getMessage(), containsString("object field cannot contain only whitespace: ['top..foo.']"));
assertThat(e.getCause().getMessage(), containsString("field name cannot contain only whitespace: ['top..foo.']"));
}

public void testDynamicFieldsEmptyName() throws Exception {
Expand Down