Skip to content

Commit

Permalink
Fix infinite loop in nested agg (#15931)
Browse files Browse the repository at this point in the history
Signed-off-by: kkewwei <[email protected]>
  • Loading branch information
kkewwei authored Sep 19, 2024
1 parent 80ff07e commit 6bef1e1
Show file tree
Hide file tree
Showing 5 changed files with 174 additions and 12 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Fix wildcard query containing escaped character ([#15737](https://github.com/opensearch-project/OpenSearch/pull/15737))
- Fix case-insensitive query on wildcard field ([#15882](https://github.com/opensearch-project/OpenSearch/pull/15882))
- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501))
- Fix infinite loop in nested agg ([#15931](https://github.com/opensearch-project/OpenSearch/pull/15931))

### Security

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
---
# The test setup includes:
# - Create nested mapping for test_nested_agg_index index
# - Index two example documents
# - nested agg

setup:
- do:
indices.create:
index: test_nested_agg_index
body:
mappings:
properties:
a:
type: nested
properties:
b1:
type: keyword
b2:
type: nested
properties:
c:
type: nested
properties:
d:
type: keyword

- do:
bulk:
refresh: true
body: |
{"index": {"_index": "test_nested_agg_index", "_id": "0"}}
{"a": { "b1": "b11", "b2": { "c": { "d": "d1" } }}}
{"index": {"_index": "test_nested_agg_index", "_id": "1"}}
{"a": { "b1": "b12", "b2": { "c": { "d": "d2" } }}}
---
# Delete Index when connection is teardown
teardown:
- do:
indices.delete:
index: test_nested_agg_index

---
"Supported queries":
- skip:
version: " - 2.17.99"
reason: "fixed in 2.18.0"

# Verify Document Count
- do:
search:
body: {
query: {
match_all: { }
}
}

- length: { hits.hits: 2 }

# Verify nested aggregation
- do:
search:
body: {
aggs: {
nested_agg: {
nested: {
path: "a"
},
aggs: {
a_b1: {
terms: {
field: "a.b1"
},
aggs: {
"c": {
nested: {
path: "a.b2.c"
},
aggs: {
"d": {
terms: {
field: "a.b2.c.d"
}
}
}
}
}
}
}
}
}
}

- length: { hits.hits: 2 }
- match: { aggregations.nested_agg.doc_count: 2 }
- length: { aggregations.nested_agg.a_b1.buckets: 2 }

- match: { aggregations.nested_agg.a_b1.buckets.0.key: "b11" }
- match: { aggregations.nested_agg.a_b1.buckets.0.doc_count: 1 }
- match: { aggregations.nested_agg.a_b1.buckets.0.c.doc_count: 1 }
- length: { aggregations.nested_agg.a_b1.buckets.0.c.d.buckets: "1" }
- match: { aggregations.nested_agg.a_b1.buckets.0.c.d.buckets.0.key: "d1" }
- match: { aggregations.nested_agg.a_b1.buckets.0.c.d.buckets.0.doc_count: 1 }

- match: { aggregations.nested_agg.a_b1.buckets.1.key: "b12" }
- match: { aggregations.nested_agg.a_b1.buckets.1.doc_count: 1 }
- match: { aggregations.nested_agg.a_b1.buckets.1.c.doc_count: 1 }
- length: { aggregations.nested_agg.a_b1.buckets.1.c.d.buckets: "1" }
- match: { aggregations.nested_agg.a_b1.buckets.1.c.d.buckets.0.key: "d2" }
- match: { aggregations.nested_agg.a_b1.buckets.1.c.d.buckets.0.doc_count: 1 }
13 changes: 13 additions & 0 deletions server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,19 @@ public void setIncludeInParent(boolean value) {
public void setIncludeInRoot(boolean value) {
includeInRoot = new Explicit<>(value, true);
}

public static boolean isParent(ObjectMapper parentObjectMapper, ObjectMapper childObjectMapper, MapperService mapperService) {
if (parentObjectMapper == null || childObjectMapper == null) {
return false;
}

ObjectMapper parent = childObjectMapper.getParentObjectMapper(mapperService);
while (parent != null && parent != parentObjectMapper) {
childObjectMapper = parent;
parent = childObjectMapper.getParentObjectMapper(mapperService);
}
return parentObjectMapper == parent;
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.lucene.search.Queries;
import org.opensearch.core.ParseField;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.mapper.ObjectMapper;
import org.opensearch.search.aggregations.Aggregator;
import org.opensearch.search.aggregations.AggregatorFactories;
Expand All @@ -63,6 +62,8 @@
import java.util.List;
import java.util.Map;

import static org.opensearch.index.mapper.ObjectMapper.Nested.isParent;

/**
* Aggregate all docs that match a nested path
*
Expand Down Expand Up @@ -98,17 +99,6 @@ public class NestedAggregator extends BucketsAggregator implements SingleBucketA
this.collectsFromSingleBucket = cardinality.map(estimate -> estimate < 2);
}

private boolean isParent(ObjectMapper parentObjectMapper, ObjectMapper childObjectMapper, MapperService mapperService) {
if (parentObjectMapper == null) {
return false;
}
ObjectMapper parent;
do {
parent = childObjectMapper.getParentObjectMapper(mapperService);
} while (parent != null && parent != parentObjectMapper);
return parentObjectMapper == parent;
}

@Override
public LeafBucketCollector getLeafCollector(final LeafReaderContext ctx, final LeafBucketCollector sub) throws IOException {
IndexReaderContext topLevelContext = ReaderUtil.getTopLevelContext(ctx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,12 @@

import java.io.IOException;
import java.util.Collection;
import java.util.Collections;

import org.mockito.Mockito;

import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX;
import static org.opensearch.index.mapper.ObjectMapper.Nested.isParent;
import static org.hamcrest.Matchers.containsString;

public class ObjectMapperTests extends OpenSearchSingleNodeTestCase {
Expand Down Expand Up @@ -568,6 +572,49 @@ public void testCompositeFields() throws Exception {
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
}

public void testNestedIsParent() throws Exception {
String mapping = XContentFactory.jsonBuilder()
.startObject()
.startObject("properties")
.startObject("a")
.field("type", "nested")
.startObject("properties")
.field("b1", Collections.singletonMap("type", "keyword"))
.startObject("b2")
.field("type", "nested")
.startObject("properties")
.startObject("c")
.field("type", "nested")
.startObject("properties")
.field("d", Collections.singletonMap("type", "keyword"))
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.endObject()
.toString();

DocumentMapper documentMapper = createIndex("test").mapperService()
.documentMapperParser()
.parse("_doc", new CompressedXContent(mapping));

MapperService mapperService = Mockito.mock(MapperService.class);
Mockito.when(mapperService.getObjectMapper(("a"))).thenReturn(documentMapper.objectMappers().get("a"));
Mockito.when(mapperService.getObjectMapper(("a.b2"))).thenReturn(documentMapper.objectMappers().get("a.b2"));
Mockito.when(mapperService.getObjectMapper(("a.b2.c"))).thenReturn(documentMapper.objectMappers().get("a.b2.c"));

assertTrue(isParent(documentMapper.objectMappers().get("a"), documentMapper.objectMappers().get("a.b2.c"), mapperService));
assertTrue(isParent(documentMapper.objectMappers().get("a"), documentMapper.objectMappers().get("a.b2"), mapperService));
assertTrue(isParent(documentMapper.objectMappers().get("a.b2"), documentMapper.objectMappers().get("a.b2.c"), mapperService));

assertFalse(isParent(documentMapper.objectMappers().get("a.b2.c"), documentMapper.objectMappers().get("a"), mapperService));
assertFalse(isParent(documentMapper.objectMappers().get("a.b2"), documentMapper.objectMappers().get("a"), mapperService));
assertFalse(isParent(documentMapper.objectMappers().get("a.b2.c"), documentMapper.objectMappers().get("a.b2"), mapperService));
}

@Override
protected Collection<Class<? extends Plugin>> getPlugins() {
return pluginList(InternalSettingsPlugin.class);
Expand Down

0 comments on commit 6bef1e1

Please sign in to comment.