Skip to content

Do not allow inner hits that fetch _source and have a non nested object field as parent #25749

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 4 commits into from
Sep 19, 2017
Merged

Do not allow inner hits that fetch _source and have a non nested object field as parent #25749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d05aee7
inner hits: Do not allow inner hits that use _source and have a non n…
martijnvg Jul 17, 2017
f782f61
Moved the check to fetch phase. This basically means that we throw
martijnvg Jul 31, 2017
a3a6ce6
fix line length violation
martijnvg Jul 31, 2017
6c46a67
added comment
martijnvg Aug 9, 2017
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
2 changes: 1 addition & 1 deletion core/src/main/java/org/elasticsearch/index/cache/bitset/BitsetFilterCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ public IndexWarmer.TerminationHandle warmReader(final IndexShard indexShard, fin
hasNested = true;
for (ObjectMapper objectMapper : docMapper.objectMappers().values()) {
if (objectMapper.nested().isNested()) {
ObjectMapper parentObjectMapper = docMapper.findParentObjectMapper(objectMapper);
ObjectMapper parentObjectMapper = objectMapper.getParentObjectMapper(mapperService);
if (parentObjectMapper != null && parentObjectMapper.nested().isNested()) {
warmUp.add(parentObjectMapper.nestedTypeFilter());
}
Expand Down
15 changes: 0 additions & 15 deletions core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,21 +292,6 @@ public ObjectMapper findNestedObjectMapper(int nestedDocId, SearchContext sc, Le
return nestedObjectMapper;
}

/**
* Returns the parent {@link ObjectMapper} instance of the specified object mapper or <code>null</code> if there
* isn't any.
*/
// TODO: We should add: ObjectMapper#getParentObjectMapper()
public ObjectMapper findParentObjectMapper(ObjectMapper objectMapper) {
int indexOfLastDot = objectMapper.fullPath().lastIndexOf('.');
if (indexOfLastDot != -1) {
String parentNestObjectPath = objectMapper.fullPath().substring(0, indexOfLastDot);
return objectMappers().get(parentNestObjectPath);
} else {
return null;
}
}

public boolean isParent(String type) {
return mapperService.getParentTypes().contains(type);
}
Expand Down
29 changes: 29 additions & 0 deletions core/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,35 @@ public final Dynamic dynamic() {
return dynamic;
}

/**
* Returns the parent {@link ObjectMapper} instance of the specified object mapper or <code>null</code> if there
* isn't any.
*/
public ObjectMapper getParentObjectMapper(MapperService mapperService) {
int indexOfLastDot = fullPath().lastIndexOf('.');
if (indexOfLastDot != -1) {
String parentNestObjectPath = fullPath().substring(0, indexOfLastDot);
return mapperService.getObjectMapper(parentNestObjectPath);
} else {
return null;
}
}

/**
* Returns whether all parent objects fields are nested too.
*/
public boolean parentObjectMapperAreNested(MapperService mapperService) {
for (ObjectMapper parent = getParentObjectMapper(mapperService);
parent != null;
parent = parent.getParentObjectMapper(mapperService)) {

if (parent.nested().isNested() == false) {
return false;
}
}
return true;
}

@Override
public ObjectMapper merge(Mapper mergeWith, boolean updateAllTypes) {
if (!(mergeWith instanceof ObjectMapper)) {
Expand Down
26 changes: 19 additions & 7 deletions core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.elasticsearch.index.fieldvisitor.FieldsVisitor;
import org.elasticsearch.index.mapper.DocumentMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.ObjectMapper;
import org.elasticsearch.index.mapper.SourceFieldMapper;
import org.elasticsearch.index.mapper.Uid;
Expand Down Expand Up @@ -246,7 +247,7 @@ private SearchHit createNestedSearchHit(SearchContext context, int nestedTopDocI
ObjectMapper nestedObjectMapper = documentMapper.findNestedObjectMapper(nestedSubDocId, context, subReaderContext);
assert nestedObjectMapper != null;
SearchHit.NestedIdentity nestedIdentity =
getInternalNestedIdentity(context, nestedSubDocId, subReaderContext, documentMapper, nestedObjectMapper);
getInternalNestedIdentity(context, nestedSubDocId, subReaderContext, context.mapperService(), nestedObjectMapper);

if (source != null) {
Tuple<XContentType, Map<String, Object>> tuple = XContentHelper.convertToMap(source, true);
Expand All @@ -262,18 +263,28 @@ private SearchHit createNestedSearchHit(SearchContext context, int nestedTopDocI
String nestedPath = nested.getField().string();
current.put(nestedPath, new HashMap<>());
Object extractedValue = XContentMapValues.extractValue(nestedPath, sourceAsMap);
List<Map<String, Object>> nestedParsedSource;
List<?> nestedParsedSource;
if (extractedValue instanceof List) {
// nested field has an array value in the _source
nestedParsedSource = (List<Map<String, Object>>) extractedValue;
nestedParsedSource = (List<?>) extractedValue;
} else if (extractedValue instanceof Map) {
// nested field has an object value in the _source. This just means the nested field has just one inner object,
// which is valid, but uncommon.
nestedParsedSource = Collections.singletonList((Map<String, Object>) extractedValue);
nestedParsedSource = Collections.singletonList(extractedValue);
} else {
throw new IllegalStateException("extracted source isn't an object or an array");
}
sourceAsMap = nestedParsedSource.get(nested.getOffset());
if ((nestedParsedSource.get(0) instanceof Map) == false &&
nestedObjectMapper.parentObjectMapperAreNested(context.mapperService()) == false) {
// When one of the parent objects are not nested then XContentMapValues.extractValue(...) extracts the values
// from two or more layers resulting in a list of list being returned. This is because nestedPath
// encapsulates two or more object layers in the _source.
//
// This is why only the first element of nestedParsedSource needs to be checked.
throw new IllegalArgumentException("Cannot execute inner hits. One or more parent object fields of nested field [" +
nestedObjectMapper.name() + "] are not nested. All parent fields need to be nested fields too");
}
sourceAsMap = (Map<String, Object>) nestedParsedSource.get(nested.getOffset());
if (nested.getChild() == null) {
current.put(nestedPath, sourceAsMap);
} else {
Expand Down Expand Up @@ -312,7 +323,8 @@ private Map<String, DocumentField> getSearchFields(SearchContext context, int ne
}

private SearchHit.NestedIdentity getInternalNestedIdentity(SearchContext context, int nestedSubDocId,
LeafReaderContext subReaderContext, DocumentMapper documentMapper,
LeafReaderContext subReaderContext,
MapperService mapperService,
ObjectMapper nestedObjectMapper) throws IOException {
int currentParent = nestedSubDocId;
ObjectMapper nestedParentObjectMapper;
Expand All @@ -321,7 +333,7 @@ private SearchHit.NestedIdentity getInternalNestedIdentity(SearchContext context
SearchHit.NestedIdentity nestedIdentity = null;
do {
Query parentFilter;
nestedParentObjectMapper = documentMapper.findParentObjectMapper(current);
nestedParentObjectMapper = current.getParentObjectMapper(mapperService);
if (nestedParentObjectMapper != null) {
if (nestedParentObjectMapper.nested().isNested() == false) {
current = nestedParentObjectMapper;
Expand Down
32 changes: 32 additions & 0 deletions core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Collections;
import java.util.function.Function;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;
Expand Down Expand Up @@ -428,4 +429,35 @@ public void testLimitOfNestedFieldsWithMultiTypePerIndex() throws Exception {
createIndex("test5", Settings.builder().put(MapperService.INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING.getKey(), 0).build())
.mapperService().merge("type", new CompressedXContent(mapping.apply("type")), MergeReason.MAPPING_RECOVERY, false);
}

public void testParentObjectMapperAreNested() throws Exception {
MapperService mapperService = createIndex("index1", Settings.EMPTY, "doc", jsonBuilder().startObject()
.startObject("properties")
.startObject("comments")
.field("type", "nested")
.startObject("properties")
.startObject("messages")
.field("type", "nested").endObject()
.endObject()
.endObject()
.endObject()
.endObject()).mapperService();
ObjectMapper objectMapper = mapperService.getObjectMapper("comments.messages");
assertTrue(objectMapper.parentObjectMapperAreNested(mapperService));

mapperService = createIndex("index2", Settings.EMPTY, "doc", jsonBuilder().startObject()
.startObject("properties")
.startObject("comments")
.field("type", "object")
.startObject("properties")
.startObject("messages")
.field("type", "nested").endObject()
.endObject()
.endObject()
.endObject()
.endObject()).mapperService();
objectMapper = mapperService.getObjectMapper("comments.messages");
assertFalse(objectMapper.parentObjectMapperAreNested(mapperService));
}

}
44 changes: 33 additions & 11 deletions core/src/test/java/org/elasticsearch/search/fetch/subphase/InnerHitsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,32 +402,54 @@ public void testInnerHitsWithObjectFieldThatHasANestedField() throws Exception {
List<IndexRequestBuilder> requests = new ArrayList<>();
requests.add(client().prepareIndex("articles", "article", "1").setSource(jsonBuilder().startObject()
.field("title", "quick brown fox")
.startObject("comments")
.startArray("messages")
.startObject().field("message", "fox eat quick").endObject()
.startObject().field("message", "bear eat quick").endObject()
.startArray("comments")
.startObject()
.startArray("messages")
.startObject().field("message", "fox eat quick").endObject()
.startObject().field("message", "bear eat quick").endObject()
.endArray()
.endObject()
.startObject()
.startArray("messages")
.startObject().field("message", "no fox").endObject()
.endArray()
.endObject()
.endArray()
.endObject()
.endObject()));
indexRandom(true, requests);

SearchResponse response = client().prepareSearch("articles")
SearchResponse response = client().prepareSearch("articles").setQuery(nestedQuery("comments.messages",
matchQuery("comments.messages.message", "fox"), ScoreMode.Avg).innerHit(new InnerHitBuilder())).get();
assertEquals("Cannot execute inner hits. One or more parent object fields of nested field [comments.messages] are " +
"not nested. All parent fields need to be nested fields too", response.getShardFailures()[0].getCause().getMessage());

response = client().prepareSearch("articles").setQuery(nestedQuery("comments.messages",
matchQuery("comments.messages.message", "fox"), ScoreMode.Avg).innerHit(new InnerHitBuilder()
.setFetchSourceContext(new FetchSourceContext(true)))).get();
assertEquals("Cannot execute inner hits. One or more parent object fields of nested field [comments.messages] are " +
"not nested. All parent fields need to be nested fields too", response.getShardFailures()[0].getCause().getMessage());

response = client().prepareSearch("articles")
.setQuery(nestedQuery("comments.messages", matchQuery("comments.messages.message", "fox"), ScoreMode.Avg)
.innerHit(new InnerHitBuilder())).get();
.innerHit(new InnerHitBuilder().setFetchSourceContext(new FetchSourceContext(false)))).get();
assertNoFailures(response);
assertHitCount(response, 1);
SearchHit hit = response.getHits().getAt(0);
assertThat(hit.getId(), equalTo("1"));
SearchHits messages = hit.getInnerHits().get("comments.messages");
assertThat(messages.getTotalHits(), equalTo(1L));
assertThat(messages.getTotalHits(), equalTo(2L));
assertThat(messages.getAt(0).getId(), equalTo("1"));
assertThat(messages.getAt(0).getNestedIdentity().getField().string(), equalTo("comments.messages"));
assertThat(messages.getAt(0).getNestedIdentity().getOffset(), equalTo(0));
assertThat(messages.getAt(0).getNestedIdentity().getOffset(), equalTo(2));
assertThat(messages.getAt(0).getNestedIdentity().getChild(), nullValue());
assertThat(messages.getAt(1).getId(), equalTo("1"));
assertThat(messages.getAt(1).getNestedIdentity().getField().string(), equalTo("comments.messages"));
assertThat(messages.getAt(1).getNestedIdentity().getOffset(), equalTo(0));
assertThat(messages.getAt(1).getNestedIdentity().getChild(), nullValue());

response = client().prepareSearch("articles")
.setQuery(nestedQuery("comments.messages", matchQuery("comments.messages.message", "bear"), ScoreMode.Avg)
.innerHit(new InnerHitBuilder())).get();
.innerHit(new InnerHitBuilder().setFetchSourceContext(new FetchSourceContext(false)))).get();
assertNoFailures(response);
assertHitCount(response, 1);
hit = response.getHits().getAt(0);
Expand All @@ -448,7 +470,7 @@ public void testInnerHitsWithObjectFieldThatHasANestedField() throws Exception {
indexRandom(true, requests);
response = client().prepareSearch("articles")
.setQuery(nestedQuery("comments.messages", matchQuery("comments.messages.message", "fox"), ScoreMode.Avg)
.innerHit(new InnerHitBuilder())).get();
.innerHit(new InnerHitBuilder().setFetchSourceContext(new FetchSourceContext(false)))).get();
assertNoFailures(response);
assertHitCount(response, 1);
hit = response.getHits().getAt(0);;
Expand Down