Skip to content

Commit

Permalink
Correctly handle mixed object paths in XContentMapValues (#65539)
Browse files Browse the repository at this point in the history 
When we have an object that looks like this:

```
{
  "foo" : {
    "cat": "meow"
  },
  "foo.bar" : "baz"
```

where the inner objects of foo are defined both as json objects and via
dot notation, then XContentMapValues.extractValue() will only descend
through the json object and will not collect values from the dot notated paths.
In these cases, the foo.bar values will not be returned in the fields
section of a search response.

This commit adds the ability to check both paths, ensuring that all relevant
values get returned as part of fields.

Fixes #65499
  • Loading branch information
@romseygeek
romseygeek authored Dec 1, 2020
1 parent f5fa0e3 commit 0d7a306
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 98 deletions.
54 changes: 30 additions & 24 deletions server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,26 @@ private static void extractRawValues(List values, Map<String, Object> part, Stri
}

String key = pathElements[index];
Object currentValue = part.get(key);
Object currentValue;
int nextIndex = index + 1;
while (currentValue == null && nextIndex != pathElements.length) {
key += "." + pathElements[nextIndex];

while (true) {
currentValue = part.get(key);
if (currentValue != null) {
if (currentValue instanceof Map) {
extractRawValues(values, (Map<String, Object>) currentValue, pathElements, nextIndex);
} else if (currentValue instanceof List) {
extractRawValues(values, (List) currentValue, pathElements, nextIndex);
} else {
values.add(currentValue);
}
}
if (nextIndex == pathElements.length) {
return;
}
key += "." + pathElements[nextIndex];
nextIndex++;
}

if (currentValue == null) {
return;
}

if (currentValue instanceof Map) {
extractRawValues(values, (Map<String, Object>) currentValue, pathElements, nextIndex);
} else if (currentValue instanceof List) {
extractRawValues(values, (List) currentValue, pathElements, nextIndex);
} else {
values.add(currentValue);
}
}

@SuppressWarnings({"unchecked"})
Expand Down Expand Up @@ -163,19 +164,24 @@ private static Object extractValue(String[] pathElements,
if (currentValue instanceof Map) {
Map<?, ?> map = (Map<?, ?>) currentValue;
String key = pathElements[index];
Object mapValue = map.get(key);
int nextIndex = index + 1;
while (mapValue == null && nextIndex != pathElements.length) {
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;
}
}
if (nextIndex == pathElements.length) {
return null;
}
key += "." + pathElements[nextIndex];
mapValue = map.get(key);
nextIndex++;
}

if (map.containsKey(key) == false) {
return null;
}

return extractValue(pathElements, nextIndex, mapValue, nullValue);
}
return null;
}
Expand Down
34 changes: 34 additions & 0 deletions server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,17 @@ public void testExtractValueWithNullValue() throws Exception {
assertEquals("NULL", XContentMapValues.extractValue("object1.object2.field", map, "NULL"));
}

public void testExtractValueMixedObjects() throws IOException {
XContentBuilder builder = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("cat", "meow").endObject()
.field("foo.bar", "baz")
.endObject();
try (XContentParser parser = createParser(JsonXContent.jsonXContent, Strings.toString(builder))) {
Map<String, Object> map = parser.map();
assertThat(XContentMapValues.extractValue("foo.bar", map), equalTo("baz"));
}
}

public void testExtractRawValue() throws Exception {
XContentBuilder builder = XContentFactory.jsonBuilder().startObject()
.field("test", "value")
Expand Down Expand Up @@ -269,6 +280,29 @@ public void testExtractRawValueLeafOnly() throws IOException {
assertThat(XContentMapValues.extractRawValues("path1.path2", map), Matchers.contains("value"));
}

public void testExtractRawValueMixedObjects() throws IOException {
{
XContentBuilder builder = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("cat", "meow").endObject()
.field("foo.bar", "baz")
.endObject();
try (XContentParser parser = createParser(JsonXContent.jsonXContent, Strings.toString(builder))) {
Map<String, Object> map = parser.map();
assertThat(XContentMapValues.extractRawValues("foo.bar", map), Matchers.contains("baz"));
}
}
{
XContentBuilder builder = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("bar", "meow").endObject()
.field("foo.bar", "baz")
.endObject();
try (XContentParser parser = createParser(JsonXContent.jsonXContent, Strings.toString(builder))) {
Map<String, Object> map = parser.map();
assertThat(XContentMapValues.extractRawValues("foo.bar", map), Matchers.containsInAnyOrder("meow", "baz"));
}
}
}

public void testPrefixedNamesFilteringTest() {
Map<String, Object> map = new HashMap<>();
map.put("obj", "value");
Expand Down
144 changes: 70 additions & 74 deletions server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@

import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.document.DocumentField;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MapperServiceTestCase;
import org.elasticsearch.index.mapper.ParsedDocument;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.search.lookup.SourceLookup;
import org.elasticsearch.test.ESSingleNodeTestCase;

import java.io.IOException;
import java.util.List;
Expand All @@ -43,7 +44,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItems;

public class FieldFetcherTests extends ESSingleNodeTestCase {
public class FieldFetcherTests extends MapperServiceTestCase {

public void testLeafValues() throws IOException {
MapperService mapperService = createMapperService();
Expand Down Expand Up @@ -89,6 +90,24 @@ public void testObjectValues() throws IOException {
assertThat(rangeField.getValue(), equalTo(Map.of("gte", 0.0f, "lte", 2.718f)));
}

public void testMixedObjectValues() throws IOException {
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.startObject("foo").field("cat", "meow").endObject()
.field("foo.bar", "baz")
.endObject();

ParsedDocument doc = mapperService.documentMapper().parse(source(Strings.toString(source)));
merge(mapperService, dynamicMapping(doc.dynamicMappingsUpdate()));

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

DocumentField field = fields.get("foo.bar");
assertThat(field.getValues().size(), equalTo(1));
assertThat(field.getValue(), equalTo("baz"));
}

public void testNonExistentField() throws IOException {
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
Expand Down Expand Up @@ -182,7 +201,7 @@ public void testArrayValueMappers() throws IOException {
}

public void testFieldNamesWithWildcard() throws IOException {
MapperService mapperService = createMapperService();;
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.array("field", "first", "second")
.field("integer_field", 333)
Expand Down Expand Up @@ -232,17 +251,10 @@ public void testDateFormat() throws IOException {
}

public void testIgnoreAbove() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("properties")
.startObject("field")
.field("type", "keyword")
.field("ignore_above", 20)
.endObject()
.endObject()
.endObject();

IndexService indexService = createIndex("index", Settings.EMPTY, mapping);
MapperService mapperService = indexService.mapperService();
MapperService mapperService = createMapperService(fieldMapping(b -> {
b.field("type", "keyword");
b.field("ignore_above", 20);
}));

XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.array("field", "value", "other_value", "really_really_long_value")
Expand All @@ -259,18 +271,15 @@ public void testIgnoreAbove() throws IOException {
}

public void testFieldAliases() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("properties")
.startObject("field").field("type", "keyword").endObject()
.startObject("alias_field")
.field("type", "alias")
.field("path", "field")
.endObject()
.endObject()
.endObject();

IndexService indexService = createIndex("index", Settings.EMPTY, mapping);
MapperService mapperService = indexService.mapperService();
MapperService mapperService = createMapperService(mapping(b -> {
b.startObject("field").field("type", "keyword").endObject();
b.startObject("alias_field");
{
b.field("type", "alias");
b.field("path", "field");
}
b.endObject();
}));

XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.field("field", "value")
Expand All @@ -291,19 +300,14 @@ public void testFieldAliases() throws IOException {
}

public void testMultiFields() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("properties")
.startObject("field")
.field("type", "integer")
.startObject("fields")
.startObject("keyword").field("type", "keyword").endObject()
.endObject()
.endObject()
.endObject()
.endObject();

IndexService indexService = createIndex("index", Settings.EMPTY, mapping);
MapperService mapperService = indexService.mapperService();
MapperService mapperService = createMapperService(fieldMapping(b -> {
b.field("type", "integer");
b.startObject("fields");
{
b.startObject("keyword").field("type", "keyword").endObject();
}
b.endObject();
}));

XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.field("field", 42)
Expand All @@ -324,24 +328,22 @@ public void testMultiFields() throws IOException {
}

public void testCopyTo() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("properties")
.startObject("field")
.field("type", "keyword")
.endObject()
.startObject("other_field")
.field("type", "integer")
.field("copy_to", "field")
.endObject()
.startObject("yet_another_field")
.field("type", "keyword")
.field("copy_to", "field")
.endObject()
.endObject()
.endObject();

IndexService indexService = createIndex("index", Settings.EMPTY, mapping);
MapperService mapperService = indexService.mapperService();
MapperService mapperService = createMapperService(mapping(b -> {
b.startObject("field").field("type", "keyword").endObject();
b.startObject("other_field");
{
b.field("type", "integer");
b.field("copy_to", "field");
}
b.endObject();
b.startObject("yet_another_field");
{
b.field("type", "keyword");
b.field("copy_to", "field");
}
b.endObject();
}));

XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.array("field", "one", "two", "three")
Expand All @@ -358,7 +360,7 @@ public void testCopyTo() throws IOException {
}

public void testObjectFields() throws IOException {
MapperService mapperService = createMapperService();;
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.array("field", "first", "second")
.startObject("object")
Expand All @@ -371,18 +373,11 @@ public void testObjectFields() throws IOException {
}

public void testTextSubFields() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("properties")
.startObject("field")
.field("type", "text")
.startObject("index_prefixes").endObject()
.field("index_phrases", true)
.endObject()
.endObject()
.endObject();

IndexService indexService = createIndex("index", Settings.EMPTY, mapping);
MapperService mapperService = indexService.mapperService();
MapperService mapperService = createMapperService(fieldMapping(b -> {
b.field("type", "text");
b.startObject("index_prefixes").endObject();
b.field("index_phrases", true);
}));

XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.array("field", "some text")
Expand Down Expand Up @@ -411,12 +406,13 @@ private static Map<String, DocumentField> fetchFields(MapperService mapperServic
SourceLookup sourceLookup = new SourceLookup();
sourceLookup.setSource(BytesReference.bytes(source));

FieldFetcher fieldFetcher = FieldFetcher.create(createQueryShardContext(mapperService), null, fields);
FieldFetcher fieldFetcher = FieldFetcher.create(newQueryShardContext(mapperService), null, fields);
return fieldFetcher.fetch(sourceLookup, Set.of());
}

public MapperService createMapperService() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("_doc")
.startObject("properties")
.startObject("field").field("type", "keyword").endObject()
.startObject("integer_field").field("type", "integer").endObject()
Expand All @@ -430,13 +426,13 @@ public MapperService createMapperService() throws IOException {
.endObject()
.startObject("field_that_does_not_match").field("type", "keyword").endObject()
.endObject()
.endObject()
.endObject();

IndexService indexService = createIndex("index", Settings.EMPTY, mapping);
return indexService.mapperService();
return createMapperService(mapping);
}

private static QueryShardContext createQueryShardContext(MapperService mapperService) {
private static QueryShardContext newQueryShardContext(MapperService mapperService) {
Settings settings = Settings.builder().put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0)
Expand Down

0 comments on commit 0d7a306

Please sign in to comment.