Fix infinite loop in nested agg (opensearch-project#15931)

Signed-off-by: kkewwei <[email protected]>
dk2k · Oct 16, 2024 · 47df984 · 47df984
1 parent 1dc87b9
commit 47df984
Show file tree

Hide file tree

Showing 5 changed files with 174 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<Class<? extends Plugin>> getPlugins() {
         return pluginList(InternalSettingsPlugin.class);