Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add index name validation rule for empty index names #119960

Merged
merged 9 commits into from
Jan 13, 2025

Conversation

craigtaverner
Copy link
Contributor

@craigtaverner craigtaverner commented Jan 10, 2025

In serverless there was a suppressed REST error with the following stacktrace:

java.lang.StringIndexOutOfBoundsException: Index 0 out of bounds for length 0
at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:55)
at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:52)
at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106)
at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302)
at java.base/java.lang.String.checkIndex(String.java:4930) at java.base/java.lang.StringLatin1.charAt(StringLatin1.java:46)
at java.base/java.lang.String.charAt(String.java:1629)
at [email protected]/org.elasticsearch.cluster.metadata.MetadataCreateIndexService.validateIndexOrAliasName(MetadataCreateIndexService.java:250)
at org.elasticsearch.xpack.esql.parser.IdentifierBuilder.validateIndexPattern(IdentifierBuilder.java:104)
at org.elasticsearch.xpack.esql.parser.IdentifierBuilder.lambda$visitIndexPattern$0(IdentifierBuilder.java:70)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
at org.elasticsearch.xpack.esql.parser.IdentifierBuilder.visitIndexPattern(IdentifierBuilder.java:63)
at org.elasticsearch.xpack.esql.parser.ExpressionBuilder.visitIndexPattern(ExpressionBuilder.java:92)
at org.elasticsearch.xpack.esql.parser.LogicalPlanBuilder.visitFromCommand(LogicalPlanBuilder.java:260)
at org.elasticsearch.xpack.esql.parser.LogicalPlanBuilder.visitFromCommand(LogicalPlanBuilder.java:88)
at org.elasticsearch.xpack.esql.parser.EsqlBaseParser$FromCommandContext.accept(EsqlBaseParser.java:2210)
at org.antlr.v4.runtime.tree.AbstractParseTreeVisitor.visitChildren(AbstractParseTreeVisitor.java:46)
at org.elasticsearch.xpack.esql.parser.EsqlBaseParserBaseVisitor.visitSourceCommand(EsqlBaseParserBaseVisitor.java:50)
...

Without knowing the original query, it is hard to know how this happened, but the underlying exception is being thrown because the index name being identified in the ES|QL parser is empty. The current index name validation rules do not consider this case, so we get a StringIndexOutOfBoundsException, but we can enhance the validation rules to at least throw the correct exception, in this case InvalidIndexNameException.

@craigtaverner craigtaverner added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Jan 10, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@craigtaverner craigtaverner added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Jan 10, 2025
Copy link
Member

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @craigtaverner, LGTM!

There are two types of index patterns, *- and -, that I can think of which can cause this error, and both of them are addressed in this PR. If we can have someone who is familiar with MetadataCreateIndexService.validateIndexOrAliasName to double check that will be great. I couldn't think of a reason that we should not make this change.

An alternative is to skip the call to MetadataCreateIndexService.validateIndexOrAliasName in IdentifierBuilder.validateIndexPattern if it is an empty index pattern, just in case it is not preferable to change MetadataCreateIndexService.validateIndexOrAliasName.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - LGTM

@craigtaverner craigtaverner merged commit d613323 into elastic:main Jan 13, 2025
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

craigtaverner added a commit to craigtaverner/elasticsearch that referenced this pull request Jan 13, 2025
* Add index name validation rule for empty index names

* Created negative tests for ES|QL index names with only exclusion character

* Added test for `*-`
elasticsearchmachine pushed a commit that referenced this pull request Jan 13, 2025
* Add index name validation rule for empty index names

* Created negative tests for ES|QL index names with only exclusion character

* Added test for `*-`
martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Jan 14, 2025
* Add index name validation rule for empty index names

* Created negative tests for ES|QL index names with only exclusion character

* Added test for `*-`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants