From 64ae0aea45036be62aec076717816a27aa006437 Mon Sep 17 00:00:00 2001 From: Fang Xing <155562079+fang-xing-esql@users.noreply.github.com> Date: Wed, 16 Oct 2024 15:46:12 -0400 Subject: [PATCH] [ES|QL] Skip validating remote cluster index names in parser (#114271) * skip validating remote cluster index names in parser --- docs/changelog/114271.yaml | 5 + .../xpack/esql/parser/IdentifierBuilder.java | 17 +- .../parser/AbstractStatementParserTests.java | 11 +- .../esql/parser/StatementParserTests.java | 289 ++++++------------ 4 files changed, 119 insertions(+), 203 deletions(-) create mode 100644 docs/changelog/114271.yaml diff --git a/docs/changelog/114271.yaml b/docs/changelog/114271.yaml new file mode 100644 index 0000000000000..7b47b922ff811 --- /dev/null +++ b/docs/changelog/114271.yaml @@ -0,0 +1,5 @@ +pr: 114271 +summary: "[ES|QL] Skip validating remote cluster index names in parser" +area: ES|QL +type: bug +issues: [] diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java index 1872fa6e8f1f0..ae2379318474b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java @@ -21,6 +21,7 @@ import java.util.List; import static org.elasticsearch.transport.RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR; +import static org.elasticsearch.transport.RemoteClusterAware.isRemoteIndexName; 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; @@ -61,11 +62,14 @@ public String visitIndexPattern(List ctx) { Holder hasSeenStar = new Holder<>(false); ctx.forEach(c -> { String indexPattern = visitIndexString(c.indexString()); - hasSeenStar.set(indexPattern.contains(WILDCARD) || hasSeenStar.get()); - validateIndexPattern(indexPattern, c, hasSeenStar.get()); - patterns.add( - c.clusterString() != null ? c.clusterString().getText() + REMOTE_CLUSTER_INDEX_SEPARATOR + indexPattern : indexPattern - ); + String clusterString = c.clusterString() != null ? c.clusterString().getText() : null; + // 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()); + } + patterns.add(clusterString != null ? clusterString + REMOTE_CLUSTER_INDEX_SEPARATOR + indexPattern : indexPattern); }); return Strings.collectionToDelimitedString(patterns, ","); } @@ -75,6 +79,9 @@ private static void validateIndexPattern(String indexPattern, EsqlBaseParser.Ind String[] indices = indexPattern.split(","); boolean hasExclusion = false; for (String index : indices) { + if (isRemoteIndexName(index)) { // skip the validation if there is remote cluster + continue; + } hasSeenStar = index.contains(WILDCARD) || hasSeenStar; index = index.replace(WILDCARD, "").strip(); if (index.isBlank()) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java index a1bcdec2b7c5c..e6fef186721a0 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java @@ -144,8 +144,15 @@ void expectError(String query, List params, String errorMessage) { assertThat(e.getMessage(), containsString(errorMessage)); } - void expectInvalidIndexNameErrorWithLineNumber(String query, String arg, String lineNumber, String indexString) { - expectError(LoggerMessageFormat.format(null, query, arg), lineNumber + "Invalid index name [" + indexString); + void expectInvalidIndexNameErrorWithLineNumber(String query, String indexString, String lineNumber) { + if ((indexString.contains("|") || indexString.contains(" ")) == false) { + expectInvalidIndexNameErrorWithLineNumber(query, indexString, lineNumber, indexString); + } + expectInvalidIndexNameErrorWithLineNumber(query, "\"" + indexString + "\"", lineNumber, indexString); + } + + void expectInvalidIndexNameErrorWithLineNumber(String query, String indexString, String lineNumber, String error) { + expectError(LoggerMessageFormat.format(null, query, indexString), lineNumber + "Invalid index name [" + error); } void expectDateMathErrorWithLineNumber(String query, String arg, String lineNumber, String error) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java index 0aeb25ac5c375..094d301875d8e 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java @@ -59,6 +59,7 @@ import org.elasticsearch.xpack.esql.plan.logical.Row; import org.elasticsearch.xpack.esql.plan.logical.UnresolvedRelation; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -425,7 +426,12 @@ public void testInlineStatsWithoutGroups() { } public void testStringAsIndexPattern() { - for (String command : List.of("FROM", "METRICS")) { + List commands = new ArrayList<>(); + commands.add("FROM"); + if (Build.current().isSnapshot()) { + commands.add("METRICS"); + } + for (String command : commands) { assertStringAsIndexPattern("foo", command + " \"foo\""); assertStringAsIndexPattern("foo,test-*", command + """ "foo","test-*" @@ -457,15 +463,23 @@ public void testStringAsIndexPattern() { assertStringAsIndexPattern("`backtick`,``multiple`back``ticks```", command + " `backtick`, ``multiple`back``ticks```"); assertStringAsIndexPattern("test,metadata,metaata,.metadata", command + " test,\"metadata\", metaata, .metadata"); assertStringAsIndexPattern(".dot", command + " .dot"); - assertStringAsIndexPattern("cluster:index", command + " cluster:index"); - assertStringAsIndexPattern("cluster:.index", command + " cluster:.index"); - assertStringAsIndexPattern("cluster*:index*", command + " cluster*:index*"); - assertStringAsIndexPattern("cluster*:*", command + " cluster*:*"); - assertStringAsIndexPattern("*:index*", command + " *:index*"); - assertStringAsIndexPattern("*:*", command + " *:*"); + assertStringAsIndexPattern("cluster:index|pattern", command + " cluster:\"index|pattern\""); + assertStringAsIndexPattern("*:index|pattern", command + " \"*:index|pattern\""); + clusterAndIndexAsIndexPattern(command, "cluster:index"); + clusterAndIndexAsIndexPattern(command, "cluster:.index"); + clusterAndIndexAsIndexPattern(command, "cluster*:index*"); + clusterAndIndexAsIndexPattern(command, "cluster*:*"); + clusterAndIndexAsIndexPattern(command, "cluster*:*"); + clusterAndIndexAsIndexPattern(command, "*:index*"); + clusterAndIndexAsIndexPattern(command, "*:*"); } } + private void clusterAndIndexAsIndexPattern(String command, String clusterAndIndex) { + assertStringAsIndexPattern(clusterAndIndex, command + " " + clusterAndIndex); + assertStringAsIndexPattern(clusterAndIndex, command + " \"" + clusterAndIndex + "\""); + } + public void testStringAsLookupIndexPattern() { assumeTrue("requires snapshot build", Build.current().isSnapshot()); assertStringAsLookupIndexPattern("foo", "ROW x = 1 | LOOKUP \"foo\" ON j"); @@ -487,221 +501,104 @@ public void testStringAsLookupIndexPattern() { assertStringAsLookupIndexPattern("`backtick`", "ROW x = 1 | LOOKUP `backtick` ON j"); assertStringAsLookupIndexPattern("``multiple`back``ticks```", "ROW x = 1 | LOOKUP ``multiple`back``ticks``` ON j"); assertStringAsLookupIndexPattern(".dot", "ROW x = 1 | LOOKUP .dot ON j"); - assertStringAsLookupIndexPattern("cluster:index", "ROW x = 1 | LOOKUP cluster:index ON j"); - assertStringAsLookupIndexPattern("cluster:.index", "ROW x = 1 | LOOKUP cluster:.index ON j"); - assertStringAsLookupIndexPattern("cluster*:index*", "ROW x = 1 | LOOKUP cluster*:index* ON j"); - assertStringAsLookupIndexPattern("cluster*:*", "ROW x = 1 | LOOKUP cluster*:* ON j"); - assertStringAsLookupIndexPattern("*:index*", "ROW x = 1 | LOOKUP *:index* ON j"); - assertStringAsLookupIndexPattern("*:*", "ROW x = 1 | LOOKUP *:* ON j"); + clusterAndIndexAsLookupIndexPattern("cluster:index"); + clusterAndIndexAsLookupIndexPattern("cluster:.index"); + clusterAndIndexAsLookupIndexPattern("cluster*:index*"); + clusterAndIndexAsLookupIndexPattern("cluster*:*"); + clusterAndIndexAsLookupIndexPattern("*:index*"); + clusterAndIndexAsLookupIndexPattern("*:*"); + } + + private void clusterAndIndexAsLookupIndexPattern(String clusterAndIndex) { + assertStringAsLookupIndexPattern(clusterAndIndex, "ROW x = 1 | LOOKUP " + clusterAndIndex + " ON j"); + assertStringAsLookupIndexPattern(clusterAndIndex, "ROW x = 1 | LOOKUP \"" + clusterAndIndex + "\"" + " ON j"); } public void testInvalidCharacterInIndexPattern() { Map commands = new HashMap<>(); - commands.put("FROM {}", "line 1:8: "); + commands.put("FROM {}", "line 1:7: "); if (Build.current().isSnapshot()) { - commands.put("METRICS {}", "line 1:11: "); - commands.put("ROW x = 1 | LOOKUP {} ON j", "line 1:22: "); + commands.put("METRICS {}", "line 1:10: "); + commands.put("ROW x = 1 | LOOKUP {} ON j", "line 1:21: "); } - List clusterStrings = List.of(" ", " *:", " cluster:"); String lineNumber; for (String command : commands.keySet()) { lineNumber = commands.get(command); - for (String clusterString : clusterStrings) { - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"index|pattern\"", lineNumber, "index|pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"index pattern\"", lineNumber, "index pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"index#pattern\"", lineNumber, "index#pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "index#pattern", lineNumber, "index#pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"index?pattern\"", lineNumber, "index?pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "index?pattern", lineNumber, "index?pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"index>pattern\"", lineNumber, "index>pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "index>pattern", lineNumber, "index>pattern"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"index\"", - lineNumber, - "-logstash-" - ); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "--", - lineNumber, - "-" - ); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"\"", lineNumber, "logstash#"); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "", lineNumber, "logstash#"); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "\"+\"", - lineNumber, - "+" - ); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "+", - lineNumber, - "+" - ); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "\"_\"", - lineNumber, - "_" - ); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "_", - lineNumber, - "_" - ); - expectInvalidIndexNameErrorWithLineNumber(command, clusterString + "\"<>\"", lineNumber, ">", lineNumber, ">>\"", lineNumber, ">>", lineNumber, "\"", - lineNumber, - "logstash- " - ); - } + expectInvalidIndexNameErrorWithLineNumber(command, "index|pattern", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "index pattern", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "index#pattern", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "index?pattern", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "index>pattern", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "index", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "_", lineNumber); + expectInvalidIndexNameErrorWithLineNumber(command, "index\\pattern", lineNumber, "index\\pattern"); + expectInvalidIndexNameErrorWithLineNumber(command, "\"index\\\\pattern\"", lineNumber, "index\\pattern"); + expectInvalidIndexNameErrorWithLineNumber(command, "\"--indexpattern\"", lineNumber, "-indexpattern"); + expectInvalidIndexNameErrorWithLineNumber(command, "--indexpattern", lineNumber, "-indexpattern"); + expectInvalidIndexNameErrorWithLineNumber(command, "<--logstash-{now/M{yyyy.MM}}>", lineNumber, "-logstash-"); + expectInvalidIndexNameErrorWithLineNumber( + command, + "\"--\"", + lineNumber, + "-" + ); + expectInvalidIndexNameErrorWithLineNumber(command, "", lineNumber, "logstash#"); + expectInvalidIndexNameErrorWithLineNumber(command, "\"\"", lineNumber, "logstash#"); + expectInvalidIndexNameErrorWithLineNumber(command, "<>", lineNumber, ">\"", lineNumber, ">>", lineNumber, ">>\"", lineNumber, "\"", lineNumber, "logstash- "); } - // comma separated indices + // comma separated indices, with exclusions // Invalid index names after removing exclusion fail, when there is no index name with wildcard before it for (String command : commands.keySet()) { if (command.contains("LOOKUP")) { continue; } - for (String clusterString : clusterStrings) { - lineNumber = command.contains("FROM") - ? "line 1:" + (22 + clusterString.length() - 1) + ": " - : "line 1:" + (25 + clusterString.length() - 1) + ": "; - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "indexpattern, --indexpattern", - lineNumber, - "-indexpattern" - ); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "indexpattern, \"--indexpattern\"", - lineNumber, - "-indexpattern" - ); - expectInvalidIndexNameErrorWithLineNumber( - command, - clusterString + "\"indexpattern, --indexpattern\"", - commands.get(command), - "-indexpattern" - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "indexpattern,-indexpattern"), - statement(command, clusterString + "indexpattern, -indexpattern") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "indexpattern,-indexpattern"), - statement(command, clusterString + "indexpattern, \"-indexpattern\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "indexpattern, -indexpattern"), - statement(command, clusterString + "\"indexpattern, -indexpattern\"") - ); - } + + lineNumber = command.contains("FROM") ? "line 1:21: " : "line 1:24: "; + expectInvalidIndexNameErrorWithLineNumber(command, "indexpattern, --indexpattern", lineNumber, "-indexpattern"); + expectInvalidIndexNameErrorWithLineNumber(command, "indexpattern, \"--indexpattern\"", lineNumber, "-indexpattern"); + expectInvalidIndexNameErrorWithLineNumber(command, "\"indexpattern, --indexpattern\"", commands.get(command), "-indexpattern"); + clustersAndIndices(command, "indexpattern", "-indexpattern"); } // Invalid index names, except invalid DateMath, are ignored if there is an index name with wildcard before it + String dateMathError = "unit [D] not supported for date math [/D]"; for (String command : commands.keySet()) { if (command.contains("LOOKUP")) { continue; } - for (String clusterString : clusterStrings) { - lineNumber = command.contains("FROM") - ? "line 1:" + (11 + clusterString.length() - 1) + ": " - : "line 1:" + (14 + clusterString.length() - 1) + ": "; - assertEquals( - unresolvedRelation(clusterString.strip() + "*,-index#pattern"), - statement(command, clusterString + "*, \"-index#pattern\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "*,-index#pattern"), - statement(command, clusterString + "*, -index#pattern") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "*, -index#pattern"), - statement(command, clusterString + "\"*, -index#pattern\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "index*,-index#pattern"), - statement(command, clusterString + "index*, \"-index#pattern\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "index*,-index#pattern"), - statement(command, clusterString + "index*, -index#pattern") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "index*, -index#pattern"), - statement(command, clusterString + "\"index*, -index#pattern\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "*,-<--logstash-{now/M{yyyy.MM}}>"), - statement(command, clusterString + "*, \"-<--logstash-{now/M{yyyy.MM}}>\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "*,-<--logstash-{now/M{yyyy.MM}}>"), - statement(command, clusterString + "*, -<--logstash-{now/M{yyyy.MM}}>") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "*, -<--logstash-{now/M{yyyy.MM}}>"), - statement(command, clusterString + "\"*, -<--logstash-{now/M{yyyy.MM}}>\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "index*,-<--logstash#-{now/M{yyyy.MM}}>"), - statement(command, clusterString + "index*, \"-<--logstash#-{now/M{yyyy.MM}}>\"") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "index*,-<--logstash#-{now/M{yyyy.MM}}>"), - statement(command, clusterString + "index*, -<--logstash#-{now/M{yyyy.MM}}>") - ); - assertEquals( - unresolvedRelation(clusterString.strip() + "index*, -<--logstash#-{now/M{yyyy.MM}}>"), - statement(command, clusterString + "\"index*, -<--logstash#-{now/M{yyyy.MM}}>\"") - ); - expectDateMathErrorWithLineNumber( - command, - clusterString + "*, \"-<-logstash-{now/D}>\"", - lineNumber, - "unit [D] not supported for date math [/D]" - ); - expectDateMathErrorWithLineNumber( - command, - clusterString + "*, -<-logstash-{now/D}>", - lineNumber, - "unit [D] not supported for date math [/D]" - ); - expectDateMathErrorWithLineNumber( - command, - clusterString + "\"*, -<-logstash-{now/D}>\"", - commands.get(command), - "unit [D] not supported for date math [/D]" - ); - } + lineNumber = command.contains("FROM") ? "line 1:10: " : "line 1:13: "; + clustersAndIndices(command, "*", "-index#pattern"); + clustersAndIndices(command, "index*", "-index#pattern"); + clustersAndIndices(command, "*", "-<--logstash-{now/M{yyyy.MM}}>"); + clustersAndIndices(command, "index*", "-<--logstash#-{now/M{yyyy.MM}}>"); + expectDateMathErrorWithLineNumber(command, "*, \"-<-logstash-{now/D}>\"", lineNumber, dateMathError); + expectDateMathErrorWithLineNumber(command, "*, -<-logstash-{now/D}>", lineNumber, dateMathError); + expectDateMathErrorWithLineNumber(command, "\"*, -<-logstash-{now/D}>\"", commands.get(command), dateMathError); } } + private void clustersAndIndices(String command, String indexString1, String indexString2) { + assertEquals(unresolvedRelation(indexString1 + "," + indexString2), statement(command, indexString1 + ", " + indexString2)); + assertEquals( + unresolvedRelation(indexString1 + "," + indexString2), + statement(command, indexString1 + ", \"" + indexString2 + "\"") + ); + assertEquals( + unresolvedRelation(indexString1 + ", " + indexString2), + statement(command, "\"" + indexString1 + ", " + indexString2 + "\"") + ); + } + public void testInvalidQuotingAsFromIndexPattern() { expectError("FROM \"foo", ": token recognition error at: '\"foo'"); expectError("FROM \"foo | LIMIT 1", ": token recognition error at: '\"foo | LIMIT 1'");