Skip to content

Commit 9c16afb

Browse files
pawankartik-elasticpawankartik-elastic
committed
Apply suggestions from review
1 parent 8363b70 commit 9c16afb

File tree

2 files changed

+76
-18
lines changed

2 files changed

+76
-18
lines changed

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

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.elasticsearch.xpack.esql.parser.EsqlBaseParser.IndexStringContext;
2121

2222
import java.util.ArrayList;
23+
import java.util.Arrays;
2324
import java.util.List;
2425

2526
import static org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR;
@@ -31,6 +32,8 @@
3132

3233
abstract class IdentifierBuilder extends AbstractBuilder {
3334

35+
private static final String BLANK_INDEX_ERROR_MESSAGE = "Blank index specified in index pattern";
36+
3437
@Override
3538
public String visitIdentifier(IdentifierContext ctx) {
3639
return ctx == null ? null : unquoteIdentifier(ctx.QUOTED_IDENTIFIER(), ctx.UNQUOTED_IDENTIFIER());
@@ -96,12 +99,13 @@ public String visitIndexPattern(List<EsqlBaseParser.IndexPatternContext> ctx) {
9699
return Strings.collectionToDelimitedString(patterns, ",");
97100
}
98101

102+
private static void throwInvalidIndexNameException(String indexPattern, String message, EsqlBaseParser.IndexPatternContext ctx) {
103+
var ie = new InvalidIndexNameException(indexPattern, message);
104+
throw new ParsingException(ie, source(ctx), ie.getMessage());
105+
}
106+
99107
private static void throwOnMixingSelectorWithCluster(String indexPattern, EsqlBaseParser.IndexPatternContext c) {
100-
InvalidIndexNameException ie = new InvalidIndexNameException(
101-
indexPattern,
102-
"Selectors are not yet supported on remote cluster patterns"
103-
);
104-
throw new ParsingException(ie, source(c), ie.getMessage());
108+
throwInvalidIndexNameException(indexPattern, "Selectors are not yet supported on remote cluster patterns", c);
105109
}
106110

107111
private static String reassembleIndexName(String clusterString, String indexPattern, String selectorString) {
@@ -160,7 +164,16 @@ private static void validate(
160164
patterns = new String[] { indexPattern };
161165
}
162166

163-
if (patterns.length == 1) {
167+
patterns = Arrays.stream(patterns).map(String::strip).toArray(String[]::new);
168+
if (Arrays.stream(patterns).anyMatch(String::isBlank)) {
169+
throwInvalidIndexNameException(indexPattern, BLANK_INDEX_ERROR_MESSAGE, ctx);
170+
}
171+
172+
// Edge case: happens when all the index names in a pattern are empty.
173+
if (patterns.length == 0) {
174+
throwInvalidIndexNameException(indexPattern, BLANK_INDEX_ERROR_MESSAGE, ctx);
175+
} else if (patterns.length == 1) {
176+
// Pattern is an unquoted string.
164177
validateSingleIndexPattern(clusterString, patterns[0], selectorString, ctx, hasSeenStar);
165178
} else {
166179
/*
@@ -260,6 +273,11 @@ private static void validateSingleIndexPattern(
260273
}
261274

262275
private static void resolveAndValidateIndex(String index, EsqlBaseParser.IndexPatternContext ctx, boolean hasSeenStar) {
276+
// If index name is blank without any replacements, it was likely blank right from the beginning and is invalid.
277+
if (index.isBlank()) {
278+
throwInvalidIndexNameException(index, BLANK_INDEX_ERROR_MESSAGE, ctx);
279+
}
280+
263281
hasSeenStar = hasSeenStar || index.contains(WILDCARD);
264282
index = index.replace(WILDCARD, "").strip();
265283
if (index.isBlank()) {

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java‎

Lines changed: 52 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -472,10 +472,12 @@ public void testStringAsIndexPattern() {
472472
"foo, test-*, abc, xyz", test123
473473
""");
474474
assertStringAsIndexPattern("foo,test,xyz", command + " foo, test,xyz");
475+
assertStringAsIndexPattern("<logstash-{now/M{yyyy.MM}}>", command + " <logstash-{now/M{yyyy.MM}}>");
475476
assertStringAsIndexPattern(
476477
"<logstash-{now/M{yyyy.MM}}>,<logstash-{now/d{yyyy.MM.dd|+12:00}}>",
477478
command + " <logstash-{now/M{yyyy.MM}}>, \"<logstash-{now/d{yyyy.MM.dd|+12:00}}>\""
478479
);
480+
assertStringAsIndexPattern("<logstash-{now/d{yyyy.MM.dd|+12:00}}>", command + " \"<logstash-{now/d{yyyy.MM.dd|+12:00}}>\"");
479481
assertStringAsIndexPattern(
480482
"-<logstash-{now/M{yyyy.MM}}>,-<-logstash-{now/M{yyyy.MM}}>,"
481483
+ "-<logstash-{now/d{yyyy.MM.dd|+12:00}}>,-<-logstash-{now/d{yyyy.MM.dd|+12:00}}>",
@@ -500,6 +502,7 @@ public void testStringAsIndexPattern() {
500502
clusterAndIndexAsIndexPattern(command, "cluster:index");
501503
clusterAndIndexAsIndexPattern(command, "cluster:.index");
502504
clusterAndIndexAsIndexPattern(command, "cluster*:index*");
505+
clusterAndIndexAsIndexPattern(command, "cluster*:<logstash-{now/d}>*");
503506
clusterAndIndexAsIndexPattern(command, "cluster*:*");
504507
clusterAndIndexAsIndexPattern(command, "*:index*");
505508
clusterAndIndexAsIndexPattern(command, "*:*");
@@ -596,6 +599,7 @@ public void testInvalidCharacterInIndexPattern() {
596599
Map<String, String> commands = new HashMap<>();
597600
commands.put("FROM {}", "line 1:6: ");
598601
if (Build.current().isSnapshot()) {
602+
commands.put("TS {}", "line 1:4: ");
599603
commands.put("ROW x = 1 | LOOKUP_🐔 {} ON j", "line 1:22: ");
600604
}
601605
String lineNumber;
@@ -636,21 +640,25 @@ public void testInvalidCharacterInIndexPattern() {
636640
expectInvalidIndexNameErrorWithLineNumber(command, "index::failure", lineNumber);
637641

638642
// Cluster name cannot be combined with selector yet.
639-
var parseLineNumber = command.contains("FROM") ? 6 : 9;
643+
int parseLineNumber = 6;
644+
if (command.startsWith("TS")) {
645+
parseLineNumber = 4;
646+
}
647+
640648
expectDoubleColonErrorWithLineNumber(command, "cluster:foo::data", parseLineNumber + 11);
641649
expectDoubleColonErrorWithLineNumber(command, "cluster:foo::failures", parseLineNumber + 11);
642650

643651
// Index pattern cannot be quoted if cluster string is present.
644652
expectErrorWithLineNumber(
645653
command,
646654
"cluster:\"foo\"::data",
647-
"line 1:14: ",
655+
command.startsWith("FROM") ? "line 1:14: " : "line 1:12: ",
648656
"mismatched input '\"foo\"' expecting UNQUOTED_SOURCE"
649657
);
650658
expectErrorWithLineNumber(
651659
command,
652660
"cluster:\"foo\"::failures",
653-
"line 1:14: ",
661+
command.startsWith("FROM") ? "line 1:14: " : "line 1:12: ",
654662
"mismatched input '\"foo\"' expecting UNQUOTED_SOURCE"
655663
);
656664

@@ -699,7 +707,7 @@ public void testInvalidCharacterInIndexPattern() {
699707
expectErrorWithLineNumber(
700708
command,
701709
"cluster:\"index,index2\"::failures",
702-
"line 1:14: ",
710+
command.startsWith("FROM") ? "line 1:14: " : "line 1:12: ",
703711
"mismatched input '\"index,index2\"' expecting UNQUOTED_SOURCE"
704712
);
705713
}
@@ -764,6 +772,7 @@ public void testInvalidCharacterInIndexPattern() {
764772
"index#pattern",
765773
"must not contain '#'"
766774
);
775+
expectDateMathErrorWithLineNumber(command, "cluster*:<logstash-{now/D}*>", commands.get(command), dateMathError);
767776
expectDateMathErrorWithLineNumber(command, "*, \"-<-logstash-{now/D}>\"", lineNumber, dateMathError);
768777
expectDateMathErrorWithLineNumber(command, "*, -<-logstash-{now/D}>", lineNumber, dateMathError);
769778
expectDateMathErrorWithLineNumber(command, "\"*, -<-logstash-{now/D}>\"", commands.get(command), dateMathError);
@@ -3183,11 +3192,43 @@ public void testValidJoinPattern() {
31833192
assertThat(joinType.coreJoin().joinName(), equalTo("LEFT OUTER"));
31843193
}
31853194

3195+
public void testInvalidFromPatterns() {
3196+
var sourceCommands = new String[] { "FROM", "TS" };
3197+
var indexIsBlank = "Blank index specified in index pattern";
3198+
var remoteIsEmpty = "remote part is empty";
3199+
var invalidDoubleColonUsage = "invalid usage of :: separator";
3200+
3201+
expectError(randomFrom(sourceCommands) + " \"\"", indexIsBlank);
3202+
expectError(randomFrom(sourceCommands) + " \" \"", indexIsBlank);
3203+
expectError(randomFrom(sourceCommands) + " \",,,\"", indexIsBlank);
3204+
expectError(randomFrom(sourceCommands) + " \",,, \"", indexIsBlank);
3205+
expectError(randomFrom(sourceCommands) + " \", , ,,\"", indexIsBlank);
3206+
expectError(randomFrom(sourceCommands) + " \",,,\",*", indexIsBlank);
3207+
expectError(randomFrom(sourceCommands) + " \"index1,<-+^,index2\",*", "must not contain the following characters");
3208+
expectError(randomFrom(sourceCommands) + " \"\",*", indexIsBlank);
3209+
expectError(randomFrom(sourceCommands) + " \"*: ,*,\"", indexIsBlank);
3210+
expectError(randomFrom(sourceCommands) + " \"*: ,*,\",validIndexName", indexIsBlank);
3211+
expectError(randomFrom(sourceCommands) + " \"\", \" \", \" \",validIndexName", indexIsBlank);
3212+
expectError(randomFrom(sourceCommands) + " \"index1\", \"index2\", \" ,index3,index4\"", indexIsBlank);
3213+
expectError(randomFrom(sourceCommands) + " \"index1,index2,,index3\"", indexIsBlank);
3214+
expectError(randomFrom(sourceCommands) + " \"index1,index2, ,index3\"", indexIsBlank);
3215+
expectError(randomFrom(sourceCommands) + " \"*, \"", indexIsBlank);
3216+
expectError(randomFrom(sourceCommands) + " \"*\", \"\"", indexIsBlank);
3217+
expectError(randomFrom(sourceCommands) + " \"*\", \" \"", indexIsBlank);
3218+
expectError(randomFrom(sourceCommands) + " \"*\", \":index1\"", remoteIsEmpty);
3219+
expectError(randomFrom(sourceCommands) + " \"index1,*,:index2\"", remoteIsEmpty);
3220+
expectError(randomFrom(sourceCommands) + " \"*\", \"::data\"", remoteIsEmpty);
3221+
expectError(randomFrom(sourceCommands) + " \"*\", \"::failures\"", remoteIsEmpty);
3222+
expectError(randomFrom(sourceCommands) + " \"*,index1::\"", invalidDoubleColonUsage);
3223+
expectError(randomFrom(sourceCommands) + " \"*\", index1, index2, \"index3:: \"", invalidDoubleColonUsage);
3224+
expectError(randomFrom(sourceCommands) + " \"*,index1::*\"", invalidDoubleColonUsage);
3225+
}
3226+
31863227
public void testInvalidPatternsWithIntermittentQuotes() {
3187-
// There are 3 ways of crafting an invalid index pattern that conforms to the grammar defined through ANTLR.
3188-
// 1. Quoting the entire pattern.
3189-
// 2. Quoting the cluster alias - "invalid cluster alias":<rest of the pattern>
3190-
// 3. Quoting the index name - <cluster alias>:"invalid index"
3228+
// There are 3 ways of crafting invalid index patterns that conforms to the grammar defined through ANTLR.
3229+
// 1. Not quoting the pattern,
3230+
// 2. Quoting individual patterns ("index1", "index2", ...), and,
3231+
// 3. Clubbing all the patterns into a single quoted string ("index1,index2,...).
31913232
//
31923233
// Note that in these tests, we unquote a pattern and then quote it immediately.
31933234
// This is because when randomly generating an index pattern, it may look like: "foo"::data.
@@ -3198,7 +3239,7 @@ public void testInvalidPatternsWithIntermittentQuotes() {
31983239
var randomIndex = randomIndexPattern();
31993240
// Select an invalid char to sneak in.
32003241
// Note: some chars like '|' and '"' are excluded to generate a proper invalid name.
3201-
char[] invalidChars = { ' ', '/', '<', '>', '?' };
3242+
Character[] invalidChars = { ' ', '/', '<', '>', '?' };
32023243
var randomInvalidChar = randomFrom(invalidChars);
32033244

32043245
// Construct the new invalid index pattern.
@@ -3207,7 +3248,7 @@ public void testInvalidPatternsWithIntermittentQuotes() {
32073248
expectError(query, "must not contain the following characters [' ','\"','*',',','/','<','>','?','\\','|']");
32083249
}
32093250

3210-
// Cluster names with invalid characters, i.e. ':' should result in an error.
3251+
// Colon outside a quoted string should result in an ANTLR error: a comma is expected.
32113252
{
32123253
var randomIndex = randomIndexPattern();
32133254

@@ -3222,8 +3263,7 @@ public void testInvalidPatternsWithIntermittentQuotes() {
32223263
expectError(query, " mismatched input ':'");
32233264
}
32243265

3225-
// If a remote index is quoted and has a remote cluster alias and the pattern is already prefixed with
3226-
// a cluster alias, we should flag the cluster alias in the pattern.
3266+
// If an explicit cluster string is present, then we expect an unquoted string next.
32273267
{
32283268
var randomIndex = randomIndexPattern();
32293269
var remoteClusterAlias = randomBoolean() ? "*" : randomIdentifier();

0 commit comments

Comments
 (0)