Enhance nested depth tracking when parsing queries (#90425)

When parsing queries on the coordinating node, there is currently no way to share state between the different parsing methods (`fromXContent`). The only query that supports a parse context is bool query, which uses the context to track nested depth of queries, added with #66204. Such nested depth tracking mechanism is not 100% accurate as it tracks bool queries only, while there's many more query types that can hold other queries hence potentially cause stack overflow when deeply nested.

This change removes the parsing context that's specific to bool query, introduced with #66204, in favour of generalizing the nested depth tracking to all query types.

The generic tracking is introduced by wrapping the parser and overriding the method that parses named objects through the xcontent registry. Another way would have been to require a context argument when parsing queries, which would mean adding a context argument to all the QueryBuilder#fromXContent static methods. That would be a breaking change for plugins that provide custom queries, hence I went for trying out a different approach.

One aspect that this change requires and introduces is the distinction between parsing a top level query (which will wrap the parser, or it would create the context if we had one), as opposed to parsing an inner query, which goes ahead with the given parser and context. We already have this distinction as we have two different static methods in `AbstractQueryBuilder` but in practice only bool query makes the distinction being the only context-aware query.

In addition to generalizing tracking nested depth when parsing queries, we should be able to adopt this same strategy to track queries usage as part #90176 .

Given that the depth check is now more restrictive, as it counts all compound queries and not only bool, we have decided to raise the default limit to `30` to ensure that users are not going to hit the limit due to this change.

docs/changelog/90425.yaml

@@ -0,0 +1,5 @@
+pr: 90425
+summary: Enhance nested depth tracking when parsing queries
+area: Search
+type: enhancement
+issues: []

docs/reference/modules/indices/search-settings.asciidoc

@@ -34,7 +34,7 @@ Requests that attempt to return more than this limit will return an error.
 
 [[indices-query-bool-max-nested-depth]]
 `indices.query.bool.max_nested_depth`::
-(<<static-cluster-setting,Static>>, integer) Maximum nested depth of bool queries. Defaults to `20`.
+(<<static-cluster-setting,Static>>, integer) Maximum nested depth of queries. Defaults to `30`.
 +
-This setting limits the nesting depth of bool queries. Deep nesting of boolean queries may lead to
-stack overflow.
+This setting limits the nesting depth of queries. Deep nesting of queries may lead to
+stack overflow errors.

modules/aggregations/src/main/java/org/elasticsearch/aggregations/bucket/adjacency/AdjacencyMatrixAggregator.java

@@ -37,7 +37,7 @@
 import java.util.Map;
 import java.util.Objects;
 
-import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
+import static org.elasticsearch.index.query.AbstractQueryBuilder.parseTopLevelQuery;
 
 /**
  * Aggregation for adjacency matrices.
@@ -54,7 +54,7 @@ protected static class KeyedFilter implements Writeable, ToXContentFragment {
         public static final NamedObjectParser<KeyedFilter, String> PARSER = (
             XContentParser p,
             String aggName,
-            String name) -> new KeyedFilter(name, parseInnerQueryBuilder(p));
+            String name) -> new KeyedFilter(name, parseTopLevelQuery(p));
 
         public KeyedFilter(String key, QueryBuilder filter) {
             if (key == null) {

modules/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessExecuteAction.java

@@ -166,7 +166,7 @@ static class ContextSetup implements Writeable, ToXContentObject {
                 }, DOCUMENT_FIELD);
                 PARSER.declareObject(
                     ConstructingObjectParser.optionalConstructorArg(),
-                    (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p),
+                    (p, c) -> AbstractQueryBuilder.parseTopLevelQuery(p),
                     QUERY_FIELD
                 );
             }

modules/parent-join/src/test/java/org/elasticsearch/join/query/HasChildQueryBuilderTests.java

@@ -154,6 +154,11 @@ protected HasChildQueryBuilder doCreateTestQueryBuilder() {
         return hqb;
     }
 
+    @Override
+    protected HasChildQueryBuilder createQueryWithInnerQuery(QueryBuilder queryBuilder) {
+        return new HasChildQueryBuilder("type", queryBuilder, ScoreMode.None);
+    }
+
     @Override
     protected void doAssertLuceneQuery(HasChildQueryBuilder queryBuilder, Query query, SearchExecutionContext context) throws IOException {
         assertThat(query, instanceOf(LateParsingQuery.class));

modules/parent-join/src/test/java/org/elasticsearch/join/query/HasParentQueryBuilderTests.java

@@ -123,6 +123,11 @@ protected HasParentQueryBuilder doCreateTestQueryBuilder() {
         return hqb;
     }
 
+    @Override
+    protected HasParentQueryBuilder createQueryWithInnerQuery(QueryBuilder queryBuilder) {
+        return new HasParentQueryBuilder("type", queryBuilder, randomBoolean());
+    }
+
     @Override
     protected void doAssertLuceneQuery(HasParentQueryBuilder queryBuilder, Query query, SearchExecutionContext context) throws IOException {
         assertThat(query, instanceOf(LateParsingQuery.class));

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java

@@ -64,7 +64,6 @@
 import org.elasticsearch.index.query.Rewriteable;
 import org.elasticsearch.index.query.SearchExecutionContext;
 import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder;
-import org.elasticsearch.xcontent.XContentLocation;
 import org.elasticsearch.xcontent.XContentParser;
 
 import java.io.ByteArrayOutputStream;
@@ -79,7 +78,7 @@
 import java.util.Objects;
 import java.util.function.Supplier;
 
-import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
+import static org.elasticsearch.index.query.AbstractQueryBuilder.parseTopLevelQuery;
 
 public class PercolatorFieldMapper extends FieldMapper {
 
@@ -400,7 +399,17 @@ public void parse(DocumentParserContext context) throws IOException {
         configureContext(executionContext, isMapUnmappedFieldAsText());
 
         XContentParser parser = context.parser();
-        QueryBuilder queryBuilder = parseQueryBuilder(parser, parser.getTokenLocation());
+        QueryBuilder queryBuilder;
+        try {
+            // make sure that we don't expand dots in field names while parsing, otherwise queries will
+            // fail parsing due to unsupported inner objects
+            context.path().setWithinLeafObject(true);
+            queryBuilder = parseTopLevelQuery(parser);
+        } catch (IOException e) {
+            throw new ParsingException(parser.getTokenLocation(), "Failed to parse", e);
+        } finally {
+            context.path().setWithinLeafObject(false);
+        }
         verifyQuery(queryBuilder);
         // Fetching of terms, shapes and indexed scripts happen during this rewrite:
         PlainActionFuture<QueryBuilder> future = new PlainActionFuture<>();
@@ -493,14 +502,6 @@ static void configureContext(SearchExecutionContext context, boolean mapUnmapped
         context.setMapUnmappedFieldAsString(mapUnmappedFieldsAsString);
     }
 
-    private static QueryBuilder parseQueryBuilder(XContentParser parser, XContentLocation location) {
-        try {
-            return parseInnerQueryBuilder(parser);
-        } catch (IOException e) {
-            throw new ParsingException(location, "Failed to parse", e);
-        }
-    }
-
     @Override
     public Iterator<Mapper> iterator() {
         return Arrays.<Mapper>asList(

modules/percolator/src/yamlRestTest/resources/rest-api-spec/test/10_basic.yml

@@ -69,7 +69,7 @@
               percolate:
                 field: query
                 index: documents_index
-                id: some_id 
+                id: some_id
   - match:  { hits.total:     1  }
 
   - do:
@@ -83,3 +83,46 @@
                 index: documents_index
                 id: some_id
   - match:  { responses.0.hits.total:     1  }
+---
+"Query against field with dotted name":
+  - do:
+      indices.create:
+        index: queries_index
+        body:
+          mappings:
+            properties:
+              query:
+                type: percolator
+              foo.bar:
+                type: keyword
+
+  - do:
+      indices.create:
+        index: documents_index
+        body:
+          mappings:
+            properties:
+              foo.bar:
+                type: keyword
+
+  - do:
+      index:
+        index: queries_index
+        id:   test_percolator
+        body:
+          query:
+            term:
+              foo.bar: value
+
+  - do:
+        indices.refresh: {}
+
+  - do:
+      search:
+        body:
+          - query:
+              percolate:
+                field: query
+                document:
+                  foo.bar: value
+  - match:  { hits.total.value: 1 }

server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java

@@ -28,7 +28,7 @@
 import java.util.Optional;
 import java.util.function.Function;
 
-import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;
+import static org.elasticsearch.index.query.AbstractQueryBuilder.parseTopLevelQuery;
 
 /**
  * Validator for an alias, to be used before adding an alias to the index metadata
@@ -158,7 +158,7 @@ public static void validateAliasFilter(
     }
 
     private static void validateAliasFilter(XContentParser parser, SearchExecutionContext searchExecutionContext) throws IOException {
-        QueryBuilder parseInnerQueryBuilder = parseInnerQueryBuilder(parser);
+        QueryBuilder parseInnerQueryBuilder = parseTopLevelQuery(parser);
         QueryBuilder queryBuilder = Rewriteable.rewrite(parseInnerQueryBuilder, searchExecutionContext, true);
         queryBuilder.toQuery(searchExecutionContext);
     }

server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java

@@ -19,6 +19,8 @@
 import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.common.xcontent.SuggestingErrorOnUnknown;
 import org.elasticsearch.xcontent.AbstractObjectParser;
+import org.elasticsearch.xcontent.FilterXContentParser;
+import org.elasticsearch.xcontent.FilterXContentParserWrapper;
 import org.elasticsearch.xcontent.NamedObjectNotFoundException;
 import org.elasticsearch.xcontent.ParseField;
 import org.elasticsearch.xcontent.XContentBuilder;
@@ -35,6 +37,8 @@
 import java.util.Map;
 import java.util.Objects;
 
+import static org.elasticsearch.search.SearchModule.INDICES_MAX_NESTED_DEPTH_SETTING;
+
 /**
  * Base class for all classes producing lucene queries.
  * Supports conversion to BytesReference and creation of lucene Query objects.
@@ -46,6 +50,8 @@ public abstract class AbstractQueryBuilder<QB extends AbstractQueryBuilder<QB>>
     public static final ParseField NAME_FIELD = new ParseField("_name");
     public static final ParseField BOOST_FIELD = new ParseField("boost");
 
+    private static int maxNestedDepth = 20;
+
     protected String queryName;
     protected float boost = DEFAULT_BOOST;
 
@@ -290,13 +296,41 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
     protected void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {}
 
     /**
-     * Parses a query excluding the query element that wraps it
+     * Parses and returns a query (excluding the query field that wraps it). To be called by API that support
+     * user provided queries. Note that the returned query may hold inner queries, and so on. Calling this method
+     * will initialize the tracking of nested depth to make sure that there's a limit to the number of queries
+     * that can be nested within one another (see {@link org.elasticsearch.search.SearchModule#INDICES_MAX_NESTED_DEPTH_SETTING}.
      */
-    public static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws IOException {
-        return parseInnerQueryBuilder(parser, Integer.valueOf(0));
+    public static QueryBuilder parseTopLevelQuery(XContentParser parser) throws IOException {
+        FilterXContentParser parserWrapper = new FilterXContentParserWrapper(parser) {
+            int nestedDepth;
+
+            @Override
+            public <T> T namedObject(Class<T> categoryClass, String name, Object context) throws IOException {
+                if (categoryClass.equals(QueryBuilder.class)) {
+                    nestedDepth++;
+                    if (nestedDepth > maxNestedDepth) {
+                        throw new IllegalArgumentException(
+                            "The nested depth of the query exceeds the maximum nested depth for queries set in ["
+                                + INDICES_MAX_NESTED_DEPTH_SETTING.getKey()
+                                + "]"
+                        );
+                    }
+                }
+                T namedObject = getXContentRegistry().parseNamedObject(categoryClass, name, this, context);
+                if (categoryClass.equals(QueryBuilder.class)) {
+                    nestedDepth--;
+                }
+                return namedObject;
+            }
+        };
+        return parseInnerQueryBuilder(parserWrapper);
     }
 
-    public static QueryBuilder parseInnerQueryBuilder(XContentParser parser, Integer nestedDepth) throws IOException {
+    /**
+     * Parses an inner query. To be called by query implementations that support inner queries.
+     */
+    protected static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws IOException {
         if (parser.currentToken() != XContentParser.Token.START_OBJECT) {
             if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
                 throw new ParsingException(parser.getTokenLocation(), "[_na] query malformed, must start with start_object");
@@ -316,7 +350,7 @@ public static QueryBuilder parseInnerQueryBuilder(XContentParser parser, Integer
         }
         QueryBuilder result;
         try {
-            result = parser.namedObject(QueryBuilder.class, queryName, nestedDepth);
+            result = parser.namedObject(QueryBuilder.class, queryName, null);
         } catch (NamedObjectNotFoundException e) {
             String message = String.format(
                 Locale.ROOT,
@@ -382,6 +416,17 @@ protected static void declareStandardFields(AbstractObjectParser<? extends Query
         parser.declareString(QueryBuilder::queryName, AbstractQueryBuilder.NAME_FIELD);
     }
 
+    /**
+     * Set the maximum nested depth of bool queries.
+     * Default value is 20.
+     */
+    public static void setMaxNestedDepth(int maxNestedDepth) {
+        if (maxNestedDepth < 1) {
+            throw new IllegalArgumentException("maxNestedDepth must be >= 1");
+        }
+        AbstractQueryBuilder.maxNestedDepth = maxNestedDepth;
+    }
+
     @Override
     public final String toString() {
         return Strings.toString(this, true, true);

server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java

@@ -32,7 +32,6 @@
 import java.util.function.Consumer;
 
 import static org.elasticsearch.common.lucene.search.Queries.fixNegativeQueryIfNeeded;
-import static org.elasticsearch.search.SearchModule.INDICES_MAX_NESTED_DEPTH_SETTING;
 
 /**
  * A Query that matches documents matching boolean combinations of other queries.
@@ -48,7 +47,6 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
     private static final ParseField MUST = new ParseField("must");
     private static final ParseField MINIMUM_SHOULD_MATCH = new ParseField("minimum_should_match");
     private static final ParseField ADJUST_PURE_NEGATIVE = new ParseField("adjust_pure_negative");
-    private static int maxNestedDepth = 20;
 
     private final List<QueryBuilder> mustClauses = new ArrayList<>();
 
@@ -80,17 +78,6 @@ public BoolQueryBuilder(StreamInput in) throws IOException {
         minimumShouldMatch = in.readOptionalString();
     }
 
-    /**
-     * Set the maximum nested depth of bool queries.
-     * Default value is 20.
-     */
-    public static void setMaxNestedDepth(int maxNestedDepth) {
-        if (maxNestedDepth < 1) {
-            throw new IllegalArgumentException("maxNestedDepth must be >= 1");
-        }
-        BoolQueryBuilder.maxNestedDepth = maxNestedDepth;
-    }
-
     @Override
     protected void doWriteTo(StreamOutput out) throws IOException {
         writeQueries(out, mustClauses);
@@ -273,22 +260,22 @@ private static void doXArrayContent(ParseField field, List<QueryBuilder> clauses
         builder.endArray();
     }
 
-    private static final ObjectParser<BoolQueryBuilder, Integer> PARSER = new ObjectParser<>("bool", BoolQueryBuilder::new);
+    private static final ObjectParser<BoolQueryBuilder, Void> PARSER = new ObjectParser<>("bool", BoolQueryBuilder::new);
     static {
-        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::must), (p, c) -> parseInnerQueryBuilder(p, c), MUST);
+        PARSER.declareObjectArrayOrNull((builder, clauses) -> clauses.forEach(builder::must), (p, c) -> parseInnerQueryBuilder(p), MUST);
         PARSER.declareObjectArrayOrNull(
             (builder, clauses) -> clauses.forEach(builder::should),
-            (p, c) -> parseInnerQueryBuilder(p, c),
+            (p, c) -> parseInnerQueryBuilder(p),
             SHOULD
         );
         PARSER.declareObjectArrayOrNull(
             (builder, clauses) -> clauses.forEach(builder::mustNot),
-            (p, c) -> parseInnerQueryBuilder(p, c),
+            (p, c) -> parseInnerQueryBuilder(p),
             MUST_NOT
         );
         PARSER.declareObjectArrayOrNull(
             (builder, clauses) -> clauses.forEach(builder::filter),
-            (p, c) -> parseInnerQueryBuilder(p, c),
+            (p, c) -> parseInnerQueryBuilder(p),
             FILTER
         );
         PARSER.declareBoolean(BoolQueryBuilder::adjustPureNegative, ADJUST_PURE_NEGATIVE);
@@ -302,16 +289,8 @@ private static void doXArrayContent(ParseField field, List<QueryBuilder> clauses
         PARSER.declareFloat(BoolQueryBuilder::boost, BOOST_FIELD);
     }
 
-    public static BoolQueryBuilder fromXContent(XContentParser parser, Integer nestedDepth) throws IOException, ParsingException {
-        nestedDepth++;
-        if (nestedDepth > maxNestedDepth) {
-            throw new IllegalArgumentException(
-                "The nested depth of the query exceeds the maximum nested depth for bool queries set in ["
-                    + INDICES_MAX_NESTED_DEPTH_SETTING.getKey()
-                    + "]"
-            );
-        }
-        return PARSER.parse(parser, nestedDepth);
+    public static BoolQueryBuilder fromXContent(XContentParser parser) throws IOException, ParsingException {
+        return PARSER.parse(parser, null);
     }
 
     @Override

server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java

@@ -61,7 +61,6 @@
 import org.elasticsearch.search.lookup.SearchLookup;
 import org.elasticsearch.search.lookup.SourceLookup;
 import org.elasticsearch.transport.RemoteClusterAware;
-import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.xcontent.XContentParserConfiguration;
 
 import java.io.IOException;
@@ -645,10 +644,6 @@ public Client getClient() {
         return client;
     }
 
-    public static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws IOException {
-        return AbstractQueryBuilder.parseInnerQueryBuilder(parser);
-    }
-
     @Override
     public final SearchExecutionContext convertToSearchExecutionContext() {
         return this;