ESQL: Disallow mixed quoted/unquoted patterns in FROM (elastic#127636)

Disallow partial quoting: individual quoting of constituent strings in an index pattern

---------

Co-authored-by: Pawan Kartik <[email protected]>
(cherry picked from commit 9021040)

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java
#	x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/IdentifierGenerator.java

Author: alex-spies

docs/changelog/127636.yaml

@@ -0,0 +1,17 @@
+pr: 127636
+summary: Disallow mixed quoted/unquoted patterns in FROM
+area: ES|QL
+type: breaking
+issues:
+ - 122651
+breaking:
+  title: Disallow mixed quoted/unquoted patterns in FROM
+  area: ES|QL
+  details: "Previously, the ES|QL grammar allowed users to individually quote constituent strings in index patterns\
+    \ such as \"remote_cluster\":\"index_name\". This would allow users to write complex malformed index patterns\
+    \ that often slip through grammar and the subsequent validation. This could result in runtime errors\
+    \ that can be misleading. This change simplifies the grammar to early reject such malformed index patterns\
+    \ at the parsing stage, allowing users to write simpler queries and see more relevant and meaningful\
+    \ errors."
+  impact: "Users can write queries with simpler index patterns and see more meaningful and relevant errors."
+  notable: false

x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java

@@ -251,10 +251,12 @@ static CsvSpecReader.CsvTestCase convertToRemoteIndices(CsvSpecReader.CsvTestCas
             String[] localIndices = fromStatement.substring("FROM ".length()).split(",");
             final String remoteIndices;
             if (canUseRemoteIndicesOnly() && randomBoolean()) {
-                remoteIndices = Arrays.stream(localIndices).map(index -> "*:" + index.trim()).collect(Collectors.joining(","));
+                remoteIndices = Arrays.stream(localIndices)
+                    .map(index -> unquoteAndRequoteAsRemote(index.trim(), true))
+                    .collect(Collectors.joining(","));
             } else {
                 remoteIndices = Arrays.stream(localIndices)
-                    .map(index -> "*:" + index.trim() + "," + index.trim())
+                    .map(index -> unquoteAndRequoteAsRemote(index.trim(), false))
                     .collect(Collectors.joining(","));
             }
             var newFrom = "FROM " + remoteIndices + " " + commands[0].substring(fromStatement.length());
@@ -265,9 +267,13 @@ static CsvSpecReader.CsvTestCase convertToRemoteIndices(CsvSpecReader.CsvTestCas
             assert parts.length >= 2 : commands[0];
             String[] indices = parts[1].split(",");
             if (canUseRemoteIndicesOnly() && randomBoolean()) {
-                parts[1] = Arrays.stream(indices).map(index -> "*:" + index.trim()).collect(Collectors.joining(","));
+                parts[1] = Arrays.stream(indices)
+                    .map(index -> unquoteAndRequoteAsRemote(index.trim(), true))
+                    .collect(Collectors.joining(","));
             } else {
-                parts[1] = Arrays.stream(indices).map(index -> "*:" + index.trim() + "," + index.trim()).collect(Collectors.joining(","));
+                parts[1] = Arrays.stream(indices)
+                    .map(index -> unquoteAndRequoteAsRemote(index.trim(), false))
+                    .collect(Collectors.joining(","));
             }
             String newNewMetrics = String.join(" ", parts);
             testCase.query = newNewMetrics + query.substring(first.length());
@@ -300,6 +306,40 @@ static boolean hasIndexMetadata(String query) {
         return false;
     }
 
+    /**
+     * Since partial quoting is prohibited, we need to take the index name, unquote it,
+     * convert it to a remote index, and then requote it. For example, "employees" is unquoted,
+     * turned into the remote index *:employees, and then requoted to get "*:employees".
+     * @param index Name of the index.
+     * @param asRemoteIndexOnly If the return needs to be in the form of "*:idx,idx" or "*:idx".
+     * @return A remote index pattern that's requoted.
+     */
+    private static String unquoteAndRequoteAsRemote(String index, boolean asRemoteIndexOnly) {
+        index = index.trim();
+
+        int numOfQuotes = 0;
+        for (; numOfQuotes < index.length(); numOfQuotes++) {
+            if (index.charAt(numOfQuotes) != '"') {
+                break;
+            }
+        }
+
+        String unquoted = unquote(index, numOfQuotes);
+        if (asRemoteIndexOnly) {
+            return quote("*:" + unquoted, numOfQuotes);
+        } else {
+            return quote("*:" + unquoted + "," + unquoted, numOfQuotes);
+        }
+    }
+
+    private static String quote(String index, int numOfQuotes) {
+        return "\"".repeat(numOfQuotes) + index + "\"".repeat(numOfQuotes);
+    }
+
+    private static String unquote(String index, int numOfQuotes) {
+        return index.substring(numOfQuotes, index.length() - numOfQuotes);
+    }
+
     @Override
     protected boolean enableRoundingDoubleValuesOnAsserting() {
         return true;

x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/EsqlActionIT.java

@@ -1071,8 +1071,6 @@ public void testDataStreamPatterns() throws Exception {
         testCases.put("test_ds_patterns*::data,test_ds_patterns*::failures,-test_ds_patterns_2*::data", 19L);
         testCases.put("test_ds_patterns*::data,test_ds_patterns*::failures,-test_ds_patterns_2*::failures", 21L);
 
-        testCases.put("\"test_ds_patterns_1,test_ds_patterns_2\"::failures", 8L);
-
         runDataStreamTest(testCases, new String[] { "test_ds_patterns_1", "test_ds_patterns_2", "test_ds_patterns_3" }, (key, value) -> {
             try (var results = run("from " + key + " | stats count(@timestamp)")) {
                 assertEquals(key, 1, getValuesList(results).size());
@@ -1097,7 +1095,7 @@ public void testDataStreamInvalidPatterns() throws Exception {
         // Only one selector separator is allowed per expression
         testCases.put("::::data", "mismatched input '::' expecting {");
         // Suffix case is not supported because there is no component named with the empty string
-        testCases.put("index::", "missing {QUOTED_STRING, UNQUOTED_SOURCE} at '|'");
+        testCases.put("index::", "missing UNQUOTED_SOURCE at '|'");
 
         runDataStreamTest(testCases, new String[] { "test_ds_patterns_1" }, (key, value) -> {
             logger.info(key);

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

@@ -149,18 +149,21 @@ fromCommand
     ;
 
 indexPattern
-    : (clusterString COLON)? indexString
-    | indexString (CAST_OP selectorString)?
+    : clusterString COLON unquotedIndexString
+    | unquotedIndexString CAST_OP selectorString
+    | indexString
     ;
 
 clusterString
     : UNQUOTED_SOURCE
-    | QUOTED_STRING
     ;
 
 selectorString
     : UNQUOTED_SOURCE
-    | QUOTED_STRING
+    ;
+
+unquotedIndexString
+    : UNQUOTED_SOURCE
     ;
 
 indexString