From 6bef1e1d8e2dda8d88c2c8f2a7875b492239a480 Mon Sep 17 00:00:00 2001 From: kkewwei Date: Thu, 19 Sep 2024 16:48:52 +0800 Subject: [PATCH] Fix infinite loop in nested agg (#15931) Signed-off-by: kkewwei --- CHANGELOG.md | 1 + .../search.aggregation/410_nested_aggs.yml | 111 ++++++++++++++++++ .../opensearch/index/mapper/ObjectMapper.java | 13 ++ .../bucket/nested/NestedAggregator.java | 14 +-- .../index/mapper/ObjectMapperTests.java | 47 ++++++++ 5 files changed, 174 insertions(+), 12 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/410_nested_aggs.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed85b630bbd4..636684af939bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/410_nested_aggs.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/410_nested_aggs.yml new file mode 100644 index 0000000000000..c115dd4751f8f --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/410_nested_aggs.yml @@ -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 } diff --git a/server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java index dd984373fc9df..b93c82d7a5c7c 100644 --- a/server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java @@ -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; + } } /** diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/nested/NestedAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/nested/NestedAggregator.java index 150efa878f866..db8979d611b4f 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/nested/NestedAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/nested/NestedAggregator.java @@ -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; @@ -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 * @@ -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); diff --git a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java index cb06bf23d9cbe..b415e1e657f7f 100644 --- a/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java @@ -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 { @@ -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> getPlugins() { return pluginList(InternalSettingsPlugin.class);