From 0d7a30630ca1c604ccf0404a9556895c3c91ba9e Mon Sep 17 00:00:00 2001 From: Alan Woodward <romseygeek@gmail.com> 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 | 144 +++++++++--------- 3 files changed, 134 insertions(+), 98 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<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"}) @@ -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 4ab28aa9a80c5..d730372047fbf 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 @@ -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") @@ -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"); 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 c50ab2acf3053..96dadaa8c71e6 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,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; @@ -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(); @@ -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() @@ -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) @@ -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") @@ -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") @@ -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) @@ -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") @@ -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") @@ -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") @@ -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() @@ -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)