From 9fe8c5dc73398959b8731eb3529b30d2a486f9d2 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 1 Dec 2020 09:57:19 +0000 Subject: [PATCH] Correctly handle mixed object paths in XContentMapValues (#65539) 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 --- .../xcontent/support/XContentMapValues.java | 54 ++++--- .../support/XContentMapValuesTests.java | 34 ++++ .../fetch/subphase/FieldFetcherTests.java | 148 +++++++++--------- 3 files changed, 136 insertions(+), 100 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java index fa21a88fbee49..d5d71b8c3e470 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java @@ -60,25 +60,26 @@ private static void extractRawValues(List values, Map 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) 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) currentValue, pathElements, nextIndex); - } else if (currentValue instanceof List) { - extractRawValues(values, (List) currentValue, pathElements, nextIndex); - } else { - values.add(currentValue); - } } @SuppressWarnings({"unchecked"}) @@ -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; } diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java index 3d155aa7da92f..bc70eea1f7527 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java @@ -197,6 +197,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 map = parser.map(); + assertThat(XContentMapValues.extractValue("foo.bar", map), equalTo("baz")); + } + } + public void testExtractRawValue() throws Exception { XContentBuilder builder = XContentFactory.jsonBuilder().startObject() .field("test", "value") @@ -271,6 +282,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 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 map = parser.map(); + assertThat(XContentMapValues.extractRawValues("foo.bar", map), Matchers.containsInAnyOrder("meow", "baz")); + } + } + } + public void testPrefixedNamesFilteringTest() { Map map = new HashMap<>(); map.put("obj", "value"); diff --git a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java index 45de79b4f24b9..42ecf35058969 100644 --- a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java +++ b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java @@ -21,19 +21,21 @@ 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.HashSet; import java.util.List; import java.util.Map; @@ -42,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(); @@ -88,6 +90,24 @@ public void testObjectValues() throws IOException { assertThat(rangeField.getValue(), equalTo(org.elasticsearch.common.collect.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 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() @@ -181,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) @@ -231,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, MapperService.SINGLE_MAPPING_NAME, 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") @@ -258,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, MapperService.SINGLE_MAPPING_NAME, 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") @@ -290,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, MapperService.SINGLE_MAPPING_NAME, 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) @@ -323,24 +328,21 @@ 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, MapperService.SINGLE_MAPPING_NAME, 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") @@ -357,7 +359,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") @@ -370,18 +372,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, MapperService.SINGLE_MAPPING_NAME, 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") @@ -410,12 +405,13 @@ private static Map fetchFields(MapperService mapperServic SourceLookup sourceLookup = new SourceLookup(); sourceLookup.setSource(BytesReference.bytes(source)); - FieldFetcher fieldFetcher = FieldFetcher.create(createQueryShardContext(mapperService), null, fields); - return fieldFetcher.fetch(sourceLookup, org.elasticsearch.common.collect.Set.of()); + FieldFetcher fieldFetcher = FieldFetcher.create(newQueryShardContext(mapperService), null, fields); + return fieldFetcher.fetch(sourceLookup, new HashSet<>()); } 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() @@ -429,13 +425,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, MapperService.SINGLE_MAPPING_NAME, 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)