Skip to content
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

Correctly handle mixed object paths in XContentMapValues #65539

Merged
merged 7 commits into from
Dec 1, 2020
Merged

Correctly handle mixed object paths in XContentMapValues #65539

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6f00c3
Add failing test: bug is in XContentMapValues.extractRawValues
romseygeek Nov 26, 2020
4be09ec
Handle mixed object paths in XContentMapValues
romseygeek Nov 26, 2020
4d3e1f8
Merge remote-tracking branch 'origin/master' into bug/object-fields-f…
romseygeek Nov 26, 2020
7acb89f
Merge branch 'master' into bug/object-fields-fetch
elasticmachine Nov 26, 2020
0541b06
Merge remote-tracking branch 'origin/master' into bug/object-fields-f…
romseygeek Dec 1, 2020
a42e917
Add test for second case
romseygeek Dec 1, 2020
9434c93
Merge remote-tracking branch 'romseygeek/bug/object-fields-fetch' int…
romseygeek Dec 1, 2020
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
54 changes: 30 additions & 24 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 @@ -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
22 changes: 22 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 @@ -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,17 @@ 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"));
}
}

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
Open in desktop
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