Skip to content

Collect multiple paths from XContentMapValues #68540

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 8 commits into from
Feb 11, 2021
Merged

Collect multiple paths from XContentMapValues #68540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c10c884
Collect multiple paths from XContentMapValues
Feb 4, 2021
f72df81
iter
Feb 4, 2021
52d2d90
iter
Feb 9, 2021
af5407c
Revert "iter"
Feb 10, 2021
96e7bb4
Add tests on FieldFetcher level
Feb 10, 2021
eb98e27
forbiddenapis
Feb 10, 2021
e4d1e4e
Merge branch 'master' into fix-68215
elasticmachine Feb 10, 2021
89972eb
Merge branch 'master' into fix-68215
elasticmachine Feb 10, 2021
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
20 changes: 14 additions & 6 deletions server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,19 +180,27 @@ private static Object extractValue(String[] pathElements,
Map<?, ?> map = (Map<?, ?>) currentValue;
String key = pathElements[index];
int nextIndex = index + 1;
List<Object> extractedValues = new ArrayList<>();
while (true) {
if (map.containsKey(key)) {
Object mapValue = map.get(key);
if (mapValue == null) {
return nullValue;
}
Object val = extractValue(pathElements, nextIndex, mapValue, nullValue);
if (val != null) {
return val;
extractedValues.add(nullValue);
} else {
Object val = extractValue(pathElements, nextIndex, mapValue, nullValue);
if (val != null) {
extractedValues.add(val);
}
}
}
if (nextIndex == pathElements.length) {
return null;
if (extractedValues.size() == 0) {
return null;
} else if (extractedValues.size() == 1) {
return extractedValues.get(0);
} else {
return extractedValues;
}
}
key += "." + pathElements[nextIndex];
nextIndex++;
Expand Down
12 changes: 12 additions & 0 deletions server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static org.elasticsearch.common.xcontent.XContentHelper.convertToMap;
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.hasKey;
import static org.hamcrest.Matchers.hasSize;
Expand Down Expand Up @@ -195,6 +196,17 @@ public void testExtractValueMixedObjects() throws IOException {
}
}

public void testExtractValueMixedDottedObjectNotation() throws IOException {
XContentBuilder builder = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("cat", "meow").endObject()
.field("foo.cat", "miau")
.endObject();
try (XContentParser parser = createParser(JsonXContent.jsonXContent, Strings.toString(builder))) {
Map<String, Object> map = parser.map();
assertThat((List<?>) XContentMapValues.extractValue("foo.cat", map), containsInAnyOrder("meow", "miau"));
}
Copy link
Contributor

@romseygeek romseygeek Feb 9, 2021

Choose a reason for hiding this comment

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

What happens if we have "foo.cat" : ["miaw", "purr"] here? I guess we get a List that contains a String and a further List, which is probably fine given that this is an edge case already.

Copy link
Member Author

@cbuescher cbuescher Feb 9, 2021

Choose a reason for hiding this comment

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

Good point. I think flattening out the list in this case makes sense and is doable. In the end we only need to handle single values and lists here. I added a commit along those lines and extended the test.

Copy link
Contributor

@romseygeek romseygeek Feb 10, 2021

Choose a reason for hiding this comment

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

This looks like it breaks a bunch of assumptions in tests - let's leave it as it was previously, with the mix of values? As I said, I think it's a sufficiently rare case that having an odd-looking response in fetch is fine.

Copy link
Member Author

@cbuescher cbuescher Feb 10, 2021

Choose a reason for hiding this comment

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

Saw the tests, probably just a class cast issue that I overlooked. Will see how easy it is to fix

Copy link
Member Author

@cbuescher cbuescher Feb 10, 2021

Choose a reason for hiding this comment

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

Okay, I can see how this gets tricky now, rolled back the last change. I added a test to the FieldFetcherTests though that shows that although we might return a List that contains a String and a further List in this case via XContentMapValues (which can even be considered to be correct, depending on the viewpoint), we unpack this e.g. when retrieving fields values to a simple list.

}

public void testExtractRawValue() throws Exception {
XContentBuilder builder = XContentFactory.jsonBuilder().startObject()
.field("test", "value")
Expand Down
43 changes: 43 additions & 0 deletions server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,49 @@ public void testMixedObjectValues() throws IOException {
DocumentField field = fields.get("foo.bar");
assertThat(field.getValues().size(), equalTo(1));
assertThat(field.getValue(), equalTo("baz"));

source = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("cat", "meow").endObject()
.field("foo.cat", "miau")
.endObject();

doc = mapperService.documentMapper().parse(source(Strings.toString(source)));

fields = fetchFields(mapperService, source, "foo.cat");
assertThat(fields.size(), equalTo(1));

field = fields.get("foo.cat");
assertThat(field.getValues().size(), equalTo(2));
assertThat(field.getValues(), containsInAnyOrder("meow", "miau"));

source = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("cat", "meow").endObject()
.array("foo.cat", "miau", "purr")
.endObject();

doc = mapperService.documentMapper().parse(source(Strings.toString(source)));

fields = fetchFields(mapperService, source, "foo.cat");
assertThat(fields.size(), equalTo(1));

field = fields.get("foo.cat");
assertThat(field.getValues().size(), equalTo(3));
assertThat(field.getValues(), containsInAnyOrder("meow", "miau", "purr"));
}

public void testMixedDottedObjectSyntax() throws IOException {
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.startObject("object").field("field", "value").endObject()
.field("object.field", "value2")
.endObject();

Map<String, DocumentField> fields = fetchFields(mapperService, source, "*");
assertThat(fields.size(), equalTo(1));

DocumentField field = fields.get("object.field");
assertThat(field.getValues().size(), equalTo(2));
assertThat(field.getValues(), containsInAnyOrder("value", "value2"));
}

public void testNonExistentField() throws IOException {
Expand Down