Merge branch 'main' into fix-129148-take2

Author: albertzaharovits

docs/changelog/122497.yaml

@@ -0,0 +1,5 @@
+pr: 122497
+summary: Check if index patterns conform to valid format before validation
+area: CCS
+type: enhancement
+issues: []

docs/reference/elasticsearch/index-settings/slow-log.md

@@ -8,7 +8,7 @@ navigation_title: Slow log
 
 The slow log records database searching and indexing events that have execution durations above specified thresholds. You can use these logs to investigate analyze or troubleshoot your cluster’s historical search and indexing performance.
 
-Slow logs report task duration at the shard level for searches, and at the index level for indexing, but might not encompass the full task execution time observed on the client. For example, slow logs don’t surface HTTP network delays or the impact of [task queues](docs-content://troubleshoot/elasticsearch/task-queue-backlog.md).
+Slow logs report task duration at the shard level for searches, and at the index level for indexing, but might not encompass the full task execution time observed on the client. For example, slow logs don’t surface HTTP network delays or the impact of [task queues](docs-content://troubleshoot/elasticsearch/task-queue-backlog.md). For more information about the higher-level operations affecting response times, refer to [Reading and writing documents](docs-content://deploy-manage/distributed-architecture/reading-and-writing-documents.md).
 
 Events that meet the specified threshold are emitted into [{{es}} logging](docs-content://deploy-manage/monitor/logging-configuration/update-elasticsearch-logging-levels.md) under the `fileset.name` of `slowlog`. These logs can be viewed in the following locations:
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

@@ -20,17 +20,20 @@
 import org.elasticsearch.xpack.esql.parser.EsqlBaseParser.IndexStringContext;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
 import static org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR;
 import static org.elasticsearch.transport.RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR;
-import static org.elasticsearch.transport.RemoteClusterAware.isRemoteIndexName;
+import static org.elasticsearch.transport.RemoteClusterAware.splitIndexName;
 import static org.elasticsearch.xpack.esql.core.util.StringUtils.EXCLUSION;
 import static org.elasticsearch.xpack.esql.core.util.StringUtils.WILDCARD;
 import static org.elasticsearch.xpack.esql.parser.ParserUtils.source;
 
 abstract class IdentifierBuilder extends AbstractBuilder {
 
+    private static final String BLANK_INDEX_ERROR_MESSAGE = "Blank index specified in index pattern";
+
     @Override
     public String visitIdentifier(IdentifierContext ctx) {
         return ctx == null ? null : unquoteIdentifier(ctx.QUOTED_IDENTIFIER(), ctx.UNQUOTED_IDENTIFIER());
@@ -88,39 +91,21 @@ public String visitIndexPattern(List<EsqlBaseParser.IndexPatternContext> ctx) {
             String indexPattern = c.unquotedIndexString() != null ? c.unquotedIndexString().getText() : visitIndexString(c.indexString());
             String clusterString = visitClusterString(c.clusterString());
             String selectorString = visitSelectorString(c.selectorString());
-            // skip validating index on remote cluster, because the behavior of remote cluster is not consistent with local cluster
-            // For example, invalid#index is an invalid index name, however FROM *:invalid#index does not return an error
-            if (clusterString == null) {
-                hasSeenStar.set(indexPattern.contains(WILDCARD) || hasSeenStar.get());
-                validateIndexPattern(indexPattern, c, hasSeenStar.get());
-                // Other instances of Elasticsearch may have differing selectors so only validate selector string if remote cluster
-                // string is unset
-                if (selectorString != null) {
-                    try {
-                        // Ensures that the selector provided is one of the valid kinds
-                        IndexNameExpressionResolver.SelectorResolver.validateIndexSelectorString(indexPattern, selectorString);
-                    } catch (InvalidIndexNameException e) {
-                        throw new ParsingException(e, source(c), e.getMessage());
-                    }
-                }
-            } else {
-                validateClusterString(clusterString, c);
-                // Do not allow selectors on remote cluster expressions until they are supported
-                if (selectorString != null) {
-                    throwOnMixingSelectorWithCluster(reassembleIndexName(clusterString, indexPattern, selectorString), c);
-                }
-            }
+
+            hasSeenStar.set(hasSeenStar.get() || indexPattern.contains(WILDCARD));
+            validate(clusterString, indexPattern, selectorString, c, hasSeenStar.get());
             patterns.add(reassembleIndexName(clusterString, indexPattern, selectorString));
         });
         return Strings.collectionToDelimitedString(patterns, ",");
     }
 
+    private static void throwInvalidIndexNameException(String indexPattern, String message, EsqlBaseParser.IndexPatternContext ctx) {
+        var ie = new InvalidIndexNameException(indexPattern, message);
+        throw new ParsingException(ie, source(ctx), ie.getMessage());
+    }
+
     private static void throwOnMixingSelectorWithCluster(String indexPattern, EsqlBaseParser.IndexPatternContext c) {
-        InvalidIndexNameException ie = new InvalidIndexNameException(
-            indexPattern,
-            "Selectors are not yet supported on remote cluster patterns"
-        );
-        throw new ParsingException(ie, source(c), ie.getMessage());
+        throwInvalidIndexNameException(indexPattern, "Selectors are not yet supported on remote cluster patterns", c);
     }
 
     private static String reassembleIndexName(String clusterString, String indexPattern, String selectorString) {
@@ -144,59 +129,196 @@ protected static void validateClusterString(String clusterString, EsqlBaseParser
         }
     }
 
-    private static void validateIndexPattern(String indexPattern, EsqlBaseParser.IndexPatternContext ctx, boolean hasSeenStar) {
-        // multiple index names can be in the same double quote, e.g. indexPattern = "idx1, *, -idx2"
-        String[] indices = indexPattern.split(",");
-        boolean hasExclusion = false;
-        for (String index : indices) {
-            // Strip spaces off first because validation checks are not written to handle them
-            index = index.strip();
-            if (isRemoteIndexName(index)) { // skip the validation if there is remote cluster
-                // Ensure that there are no selectors as they are not yet supported
-                if (index.contains(SELECTOR_SEPARATOR)) {
-                    throwOnMixingSelectorWithCluster(index, ctx);
-                }
-                continue;
+    /**
+     * Takes the parsed constituent strings and validates them.
+     * @param clusterString Name of the remote cluster. Can be null.
+     * @param indexPattern Name of the index or pattern; can also have multiple patterns in case of quoting,
+     *                     e.g. {@code FROM """index*,-index1"""}.
+     * @param selectorString Selector string, i.e. "::data" or "::failures". Can be null.
+     * @param ctx Index Pattern Context for generating error messages with offsets.
+     * @param hasSeenStar If we've seen an asterisk so far.
+     */
+    private static void validate(
+        String clusterString,
+        String indexPattern,
+        String selectorString,
+        EsqlBaseParser.IndexPatternContext ctx,
+        boolean hasSeenStar
+    ) {
+        /*
+         * At this point, only 3 formats are possible:
+         * "index_pattern(s)",
+         * remote:index_pattern, and,
+         * index_pattern::selector_string.
+         *
+         * The grammar prohibits remote:"index_pattern(s)" or "index_pattern(s)"::selector_string as they're
+         * partially quoted. So if either of cluster string or selector string are present, there's no need
+         * to split the pattern by comma since comma requires partial quoting.
+         */
+
+        String[] patterns;
+        if (clusterString == null && selectorString == null) {
+            // Pattern could be quoted or is singular like "index_name".
+            patterns = indexPattern.split(",", -1);
+        } else {
+            // Either of cluster string or selector string is present. Pattern is unquoted.
+            patterns = new String[] { indexPattern };
+        }
+
+        patterns = Arrays.stream(patterns).map(String::strip).toArray(String[]::new);
+        if (Arrays.stream(patterns).anyMatch(String::isBlank)) {
+            throwInvalidIndexNameException(indexPattern, BLANK_INDEX_ERROR_MESSAGE, ctx);
+        }
+
+        // Edge case: happens when all the index names in a pattern are empty like "FROM ",,,,,"".
+        if (patterns.length == 0) {
+            throwInvalidIndexNameException(indexPattern, BLANK_INDEX_ERROR_MESSAGE, ctx);
+        } else if (patterns.length == 1) {
+            // Pattern is either an unquoted string or a quoted string with a single index (no comma sep).
+            validateSingleIndexPattern(clusterString, patterns[0], selectorString, ctx, hasSeenStar);
+        } else {
+            /*
+             * Presence of multiple patterns requires a comma and comma requires quoting. If quoting is present,
+             * cluster string and selector string cannot be present; they need to be attached within the quoting.
+             * So we attempt to extract them later.
+             */
+            for (String pattern : patterns) {
+                validateSingleIndexPattern(null, pattern, null, ctx, hasSeenStar);
             }
+        }
+    }
+
+    /**
+     * Validates the constituent strings. Will extract the cluster string and/or selector string from the index
+     * name if clubbed together inside a quoted string.
+     *
+     * @param clusterString Name of the remote cluster. Can be null.
+     * @param indexName Name of the index.
+     * @param selectorString Selector string, i.e. "::data" or "::failures". Can be null.
+     * @param ctx Index Pattern Context for generating error messages with offsets.
+     * @param hasSeenStar If we've seen an asterisk so far.
+     */
+    private static void validateSingleIndexPattern(
+        String clusterString,
+        String indexName,
+        String selectorString,
+        EsqlBaseParser.IndexPatternContext ctx,
+        boolean hasSeenStar
+    ) {
+        indexName = indexName.strip();
+
+        /*
+         * Precedence:
+         * 1. Cannot mix cluster and selector strings.
+         * 2. Cluster string must be valid.
+         * 3. Index name must be valid.
+         * 4. Selector string must be valid.
+         *
+         * Since cluster string and/or selector string can be clubbed with the index name, we must try to
+         * manually extract them before we attempt to do #2, #3, and #4.
+         */
+
+        // It is possible to specify a pattern like "remote_cluster:index_name". Try to extract such details from the index string.
+        if (clusterString == null && selectorString == null) {
             try {
-                Tuple<String, String> splitPattern = IndexNameExpressionResolver.splitSelectorExpression(index);
-                if (splitPattern.v2() != null) {
-                    index = splitPattern.v1();
-                }
-            } catch (InvalidIndexNameException e) {
-                // throws exception if the selector expression is invalid. Selector resolution does not complain about exclusions
+                var split = splitIndexName(indexName);
+                clusterString = split[0];
+                indexName = split[1];
+            } catch (IllegalArgumentException e) {
                 throw new ParsingException(e, source(ctx), e.getMessage());
             }
-            hasSeenStar = index.contains(WILDCARD) || hasSeenStar;
-            index = index.replace(WILDCARD, "").strip();
-            if (index.isBlank()) {
-                continue;
-            }
-            hasExclusion = index.startsWith(EXCLUSION);
-            index = removeExclusion(index);
-            String tempName;
-            try {
-                // remove the exclusion outside of <>, from index names with DateMath expression,
-                // e.g. -<-logstash-{now/d}> becomes <-logstash-{now/d}> before calling resolveDateMathExpression
-                tempName = IndexNameExpressionResolver.resolveDateMathExpression(index);
-            } catch (ElasticsearchParseException e) {
-                // throws exception if the DateMath expression is invalid, resolveDateMathExpression does not complain about exclusions
-                throw new ParsingException(e, source(ctx), e.getMessage());
+        }
+
+        // At the moment, selector strings for remote indices is not allowed.
+        if (clusterString != null && selectorString != null) {
+            throwOnMixingSelectorWithCluster(reassembleIndexName(clusterString, indexName, selectorString), ctx);
+        }
+
+        // Validation in the right precedence.
+        if (clusterString != null) {
+            clusterString = clusterString.strip();
+            validateClusterString(clusterString, ctx);
+        }
+
+        /*
+         * It is possible for selector string to be attached to the index: "index_name::selector_string".
+         * Try to extract the selector string.
+         */
+        try {
+            Tuple<String, String> splitPattern = IndexNameExpressionResolver.splitSelectorExpression(indexName);
+            if (splitPattern.v2() != null) {
+                // Cluster string too was clubbed with the index name like selector string.
+                if (clusterString != null) {
+                    throwOnMixingSelectorWithCluster(reassembleIndexName(clusterString, splitPattern.v1(), splitPattern.v2()), ctx);
+                } else {
+                    // We've seen a selectorString. Use it.
+                    selectorString = splitPattern.v2();
+                }
             }
-            hasExclusion = tempName.startsWith(EXCLUSION) || hasExclusion;
-            index = tempName.equals(index) ? index : removeExclusion(tempName);
+
+            indexName = splitPattern.v1();
+        } catch (InvalidIndexNameException e) {
+            throw new ParsingException(e, source(ctx), e.getMessage());
+        }
+
+        resolveAndValidateIndex(indexName, ctx, hasSeenStar);
+        if (selectorString != null) {
+            selectorString = selectorString.strip();
             try {
-                MetadataCreateIndexService.validateIndexOrAliasName(index, InvalidIndexNameException::new);
+                // Ensures that the selector provided is one of the valid kinds.
+                IndexNameExpressionResolver.SelectorResolver.validateIndexSelectorString(indexName, selectorString);
             } catch (InvalidIndexNameException e) {
-                // ignore invalid index name if it has exclusions and there is an index with wildcard before it
-                if (hasSeenStar && hasExclusion) {
-                    continue;
-                }
                 throw new ParsingException(e, source(ctx), e.getMessage());
             }
         }
     }
 
+    private static void resolveAndValidateIndex(String index, EsqlBaseParser.IndexPatternContext ctx, boolean hasSeenStar) {
+        // If index name is blank without any replacements, it was likely blank right from the beginning and is invalid.
+        if (index.isBlank()) {
+            throwInvalidIndexNameException(index, BLANK_INDEX_ERROR_MESSAGE, ctx);
+        }
+
+        hasSeenStar = hasSeenStar || index.contains(WILDCARD);
+        index = index.replace(WILDCARD, "").strip();
+        if (index.isBlank()) {
+            return;
+        }
+        var hasExclusion = index.startsWith(EXCLUSION);
+        index = removeExclusion(index);
+        String tempName;
+        try {
+            // remove the exclusion outside of <>, from index names with DateMath expression,
+            // e.g. -<-logstash-{now/d}> becomes <-logstash-{now/d}> before calling resolveDateMathExpression
+            tempName = IndexNameExpressionResolver.resolveDateMathExpression(index);
+        } catch (ElasticsearchParseException e) {
+            // throws exception if the DateMath expression is invalid, resolveDateMathExpression does not complain about exclusions
+            throw new ParsingException(e, source(ctx), e.getMessage());
+        }
+        hasExclusion = tempName.startsWith(EXCLUSION) || hasExclusion;
+        index = tempName.equals(index) ? index : removeExclusion(tempName);
+        try {
+            MetadataCreateIndexService.validateIndexOrAliasName(index, InvalidIndexNameException::new);
+        } catch (InvalidIndexNameException e) {
+            // ignore invalid index name if it has exclusions and there is an index with wildcard before it
+            if (hasSeenStar && hasExclusion) {
+                return;
+            }
+
+            InvalidIndexNameException errorToThrow = e;
+            /*
+             * We only modify this particular message because it mentions '*' as an invalid char.
+             * However, we do allow asterisk in the index patterns: wildcarded patterns. Let's not
+             * mislead the user by mentioning this char in the error message.
+             */
+            if (e.getMessage().contains("must not contain the following characters")) {
+                errorToThrow = new InvalidIndexNameException(index, e.getMessage().replace("'*',", ""));
+            }
+
+            throw new ParsingException(errorToThrow, source(ctx), errorToThrow.getMessage());
+        }
+    }
+
     private static String removeExclusion(String indexPattern) {
         return indexPattern.charAt(0) == EXCLUSION.charAt(0) ? indexPattern.substring(1) : indexPattern;
     }