From 4dc5f4929f60ead83ebca1df8fba35777c154fd8 Mon Sep 17 00:00:00 2001 From: Mikhail Khludnev Date: Sat, 10 Feb 2024 23:50:20 +0300 Subject: [PATCH] Introduce index.query.max_nested_depth. fix #3268 (#11670) * Introduce index.query.max_nested_depth. fix #3268 Signed-off-by: Mikhail Khludnev * min index.query.max_nested_depth=1 Signed-off-by: Mikhail Khludnev * CHANGELOG.md Signed-off-by: Mikhail Khludnev * keep lines as they were Signed-off-by: Mikhail Khludnev --------- Signed-off-by: Mikhail Khludnev Signed-off-by: Mikhail Khludnev Co-authored-by: Mikhail Khludnev --- CHANGELOG.md | 1 + .../common/settings/IndexScopedSettings.java | 1 + .../org/opensearch/index/IndexSettings.java | 26 +++++ .../index/query/NestedQueryBuilder.java | 7 +- .../index/query/QueryShardContext.java | 4 +- .../index/query/support/NestedScope.java | 22 ++++- .../index/query/NestedQueryBuilderTests.java | 97 +++++++++++++++++++ 7 files changed, 153 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cf57f3f5af6d..2f3cd5d92e9bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Added - Add support for dependencies in plugin descriptor properties with semver range ([#11441](https://github.com/opensearch-project/OpenSearch/pull/11441)) - Add community_id ingest processor ([#12121](https://github.com/opensearch-project/OpenSearch/pull/12121)) +- Introduce query level setting `index.query.max_nested_depth` limiting nested queries ([#3268](https://github.com/opensearch-project/OpenSearch/issues/3268) ### Dependencies diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index cf502fe685fcf..49bb3abf1decd 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -149,6 +149,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { IndexSettings.MAX_ADJACENCY_MATRIX_FILTERS_SETTING, IndexSettings.MAX_ANALYZED_OFFSET_SETTING, IndexSettings.MAX_TERMS_COUNT_SETTING, + IndexSettings.MAX_NESTED_QUERY_DEPTH_SETTING, IndexSettings.INDEX_TRANSLOG_SYNC_INTERVAL_SETTING, IndexSettings.DEFAULT_FIELD_SETTING, IndexSettings.QUERY_STRING_LENIENT_SETTING, diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 43a3a49418fbf..36e48b2590a4e 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -271,6 +271,17 @@ public static IndexMergePolicy fromString(String text) { Property.IndexScope ); + /** + * Index setting describing the maximum number of nested scopes in queries. + * The default maximum of 20. 1 means once nesting. + */ + public static final Setting MAX_NESTED_QUERY_DEPTH_SETTING = Setting.intSetting( + "index.query.max_nested_depth", + 20, + 1, + Property.Dynamic, + Property.IndexScope + ); /** * Index setting describing for NGramTokenizer and NGramTokenFilter * the maximum difference between @@ -765,6 +776,8 @@ private void setRetentionLeaseMillis(final TimeValue retentionLease) { private volatile TimeValue searchIdleAfter; private volatile int maxAnalyzedOffset; private volatile int maxTermsCount; + + private volatile int maxNestedQueryDepth; private volatile String defaultPipeline; private volatile String requiredPipeline; private volatile boolean searchThrottled; @@ -930,6 +943,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti maxSlicesPerPit = scopedSettings.get(MAX_SLICES_PER_PIT); maxAnalyzedOffset = scopedSettings.get(MAX_ANALYZED_OFFSET_SETTING); maxTermsCount = scopedSettings.get(MAX_TERMS_COUNT_SETTING); + maxNestedQueryDepth = scopedSettings.get(MAX_NESTED_QUERY_DEPTH_SETTING); maxRegexLength = scopedSettings.get(MAX_REGEX_LENGTH_SETTING); this.tieredMergePolicyProvider = new TieredMergePolicyProvider(logger, this); this.logByteSizeMergePolicyProvider = new LogByteSizeMergePolicyProvider(logger, this); @@ -1042,6 +1056,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti scopedSettings.addSettingsUpdateConsumer(MAX_REFRESH_LISTENERS_PER_SHARD, this::setMaxRefreshListeners); scopedSettings.addSettingsUpdateConsumer(MAX_ANALYZED_OFFSET_SETTING, this::setHighlightMaxAnalyzedOffset); scopedSettings.addSettingsUpdateConsumer(MAX_TERMS_COUNT_SETTING, this::setMaxTermsCount); + scopedSettings.addSettingsUpdateConsumer(MAX_NESTED_QUERY_DEPTH_SETTING, this::setMaxNestedQueryDepth); scopedSettings.addSettingsUpdateConsumer(MAX_SLICES_PER_SCROLL, this::setMaxSlicesPerScroll); scopedSettings.addSettingsUpdateConsumer(MAX_SLICES_PER_PIT, this::setMaxSlicesPerPit); scopedSettings.addSettingsUpdateConsumer(DEFAULT_FIELD_SETTING, this::setDefaultFields); @@ -1557,6 +1572,17 @@ private void setMaxTermsCount(int maxTermsCount) { this.maxTermsCount = maxTermsCount; } + /** + * @return max level of nested queries and documents + */ + public int getMaxNestedQueryDepth() { + return this.maxNestedQueryDepth; + } + + private void setMaxNestedQueryDepth(int maxNestedQueryDepth) { + this.maxNestedQueryDepth = maxNestedQueryDepth; + } + /** * Returns the maximum number of allowed script_fields to retrieve in a search request */ diff --git a/server/src/main/java/org/opensearch/index/query/NestedQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/NestedQueryBuilder.java index ac4fde7f06b16..3f97b3918a126 100644 --- a/server/src/main/java/org/opensearch/index/query/NestedQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/NestedQueryBuilder.java @@ -322,10 +322,13 @@ protected Query doToQuery(QueryShardContext context) throws IOException { try { context.setParentFilter(parentFilter); context.nestedScope().nextLevel(nestedObjectMapper); - innerQuery = this.query.toQuery(context); + try { + innerQuery = this.query.toQuery(context); + } finally { + context.nestedScope().previousLevel(); + } } finally { context.setParentFilter(previousParentFilter); - context.nestedScope().previousLevel(); } // ToParentBlockJoinQuery requires that the inner query only matches documents diff --git a/server/src/main/java/org/opensearch/index/query/QueryShardContext.java b/server/src/main/java/org/opensearch/index/query/QueryShardContext.java index eba3ed076e82b..5069f055a89bb 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryShardContext.java +++ b/server/src/main/java/org/opensearch/index/query/QueryShardContext.java @@ -256,7 +256,7 @@ private QueryShardContext( this.bitsetFilterCache = bitsetFilterCache; this.indexFieldDataService = indexFieldDataLookup; this.allowUnmappedFields = indexSettings.isDefaultAllowUnmappedFields(); - this.nestedScope = new NestedScope(); + this.nestedScope = new NestedScope(indexSettings); this.scriptService = scriptService; this.indexSettings = indexSettings; this.searcher = searcher; @@ -270,7 +270,7 @@ private void reset() { allowUnmappedFields = indexSettings.isDefaultAllowUnmappedFields(); this.lookup = null; this.namedQueries.clear(); - this.nestedScope = new NestedScope(); + this.nestedScope = new NestedScope(indexSettings); } public IndexAnalyzers getIndexAnalyzers() { diff --git a/server/src/main/java/org/opensearch/index/query/support/NestedScope.java b/server/src/main/java/org/opensearch/index/query/support/NestedScope.java index 51abe389ad686..488768c32d17f 100644 --- a/server/src/main/java/org/opensearch/index/query/support/NestedScope.java +++ b/server/src/main/java/org/opensearch/index/query/support/NestedScope.java @@ -33,6 +33,7 @@ package org.opensearch.index.query.support; import org.opensearch.common.annotation.PublicApi; +import org.opensearch.index.IndexSettings; import org.opensearch.index.mapper.ObjectMapper; import java.util.Deque; @@ -47,6 +48,11 @@ public final class NestedScope { private final Deque levelStack = new LinkedList<>(); + private final IndexSettings indexSettings; + + public NestedScope(IndexSettings indexSettings) { + this.indexSettings = indexSettings; + } /** * @return For the current nested level returns the object mapper that belongs to that @@ -60,7 +66,21 @@ public ObjectMapper getObjectMapper() { */ public ObjectMapper nextLevel(ObjectMapper level) { ObjectMapper previous = levelStack.peek(); - levelStack.push(level); + if (levelStack.size() < indexSettings.getMaxNestedQueryDepth()) { + levelStack.push(level); + } else { + throw new IllegalArgumentException( + "The depth of Nested Query is [" + + (levelStack.size() + 1) + + "] has exceeded " + + "the allowed maximum of [" + + indexSettings.getMaxNestedQueryDepth() + + "]. " + + "This maximum can be set by changing the [" + + IndexSettings.MAX_NESTED_QUERY_DEPTH_SETTING.getKey() + + "] index level setting." + ); + } return previous; } diff --git a/server/src/test/java/org/opensearch/index/query/NestedQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/NestedQueryBuilderTests.java index 62337264bc0b1..29efd64e5c751 100644 --- a/server/src/test/java/org/opensearch/index/query/NestedQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/NestedQueryBuilderTests.java @@ -34,12 +34,16 @@ import com.carrotsearch.randomizedtesting.generators.RandomPicks; +import org.apache.lucene.search.BooleanClause; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.join.ScoreMode; import org.opensearch.OpenSearchException; import org.opensearch.Version; import org.opensearch.action.admin.indices.mapping.put.PutMappingRequest; +import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.compress.CompressedXContent; import org.opensearch.common.settings.Settings; import org.opensearch.index.IndexSettings; @@ -58,6 +62,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import static org.opensearch.index.IndexSettingsTests.newIndexMeta; import static org.opensearch.index.query.InnerHitBuilderTests.randomNestedInnerHits; @@ -431,4 +436,96 @@ public void testSetParentFilterInContext() throws Exception { assertNull(queryShardContext.getParentFilter()); verify(innerQueryBuilder).toQuery(queryShardContext); } + + public void testNestedDepthProhibited() throws Exception { + assertThrows(IllegalArgumentException.class, () -> doWithDepth(0, context -> fail("won't call"))); + } + + public void testNestedDepthAllowed() throws Exception { + ThrowingConsumer check = (context) -> { + NestedQueryBuilder queryBuilder = new NestedQueryBuilder("nested1", new MatchAllQueryBuilder(), ScoreMode.None); + OpenSearchToParentBlockJoinQuery blockJoinQuery = (OpenSearchToParentBlockJoinQuery) queryBuilder.toQuery(context); + Optional childLeg = ((BooleanQuery) blockJoinQuery.getChildQuery()).clauses() + .stream() + .filter(c -> c.getOccur() == BooleanClause.Occur.MUST) + .findFirst(); + assertTrue(childLeg.isPresent()); + assertEquals(new MatchAllDocsQuery(), childLeg.get().getQuery()); + }; + check.accept(createShardContext()); + doWithDepth(randomIntBetween(1, 20), check); + } + + public void testNestedDepthOnceOnly() throws Exception { + doWithDepth(1, this::checkOnceNested); + } + + public void testNestedDepthDefault() throws Exception { + assertEquals(20, createShardContext().getIndexSettings().getMaxNestedQueryDepth()); + } + + private void checkOnceNested(QueryShardContext ctx) throws Exception { + { + NestedQueryBuilder depth2 = new NestedQueryBuilder( + "nested1", + new NestedQueryBuilder("nested1", new MatchAllQueryBuilder(), ScoreMode.None), + ScoreMode.None + ); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> depth2.toQuery(ctx)); + assertEquals( + "The depth of Nested Query is [2] has exceeded the allowed maximum of [1]. This maximum can be set by changing the [index.query.max_nested_depth] index level setting.", + e.getMessage() + ); + } + { + QueryBuilder mustBjqMustBjq = new BoolQueryBuilder().must( + new NestedQueryBuilder("nested1", new MatchAllQueryBuilder(), ScoreMode.None) + ).must(new NestedQueryBuilder("nested1", new MatchAllQueryBuilder(), ScoreMode.None)); + BooleanQuery bool = (BooleanQuery) mustBjqMustBjq.toQuery(ctx); + assertEquals( + "Can parse joins one by one without breaching depth limit", + 2, + bool.clauses().stream().filter(c -> c.getQuery() instanceof OpenSearchToParentBlockJoinQuery).count() + ); + } + } + + public void testUpdateMaxDepthSettings() throws Exception { + doWithDepth(2, (ctx) -> { + assertEquals(ctx.getIndexSettings().getMaxNestedQueryDepth(), 2); + NestedQueryBuilder depth2 = new NestedQueryBuilder( + "nested1", + new NestedQueryBuilder("nested1", new MatchAllQueryBuilder(), ScoreMode.None), + ScoreMode.None + ); + Query depth2Query = depth2.toQuery(ctx); + assertTrue(depth2Query instanceof OpenSearchToParentBlockJoinQuery); + }); + } + + void doWithDepth(int depth, ThrowingConsumer test) throws Exception { + QueryShardContext context = createShardContext(); + int defLimit = context.getIndexSettings().getMaxNestedQueryDepth(); + assertTrue(defLimit > 0); + Settings updateSettings = Settings.builder() + .put(context.getIndexSettings().getSettings()) + .put("index.query.max_nested_depth", depth) + .build(); + context.getIndexSettings().updateIndexMetadata(IndexMetadata.builder("index").settings(updateSettings).build()); + try { + test.accept(context); + } finally { + context.getIndexSettings() + .updateIndexMetadata( + IndexMetadata.builder("index") + .settings( + Settings.builder() + .put(context.getIndexSettings().getSettings()) + .put("index.query.max_nested_depth", defLimit) + .build() + ) + .build() + ); + } + } }