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.

Sign up for GitHub

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

Jump to bottom

SQL: Fix NPE for parameterized LIKE/RLIKE #53573

Merged
merged 3 commits into from
Mar 16, 2020
Merged

SQL: Fix NPE for parameterized LIKE/RLIKE #53573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09d24c7
SQL: Fix NPE for parameterized LIKE/RLIKE
matriv Mar 14, 2020
676a3a5
move null check to Parser
matriv Mar 16, 2020
a946dc0
remove unused import
matriv Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ public LikePattern visitPattern(PatternContext ctx) {
}

String pattern = string(ctx.value);
if (pattern == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Since the pattern always has to be non-null, I think it would be better to report the error at the parser level instead of letting it propagate all the way to the tree which would be invalid anyway...
Just like w do with pos below by throwing a ParsingException

Copy link
Contributor Author

@matriv matriv Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about it and I kind of liked more the separation of Parsing to an illegalarg case.
In my mind null is acceptable in parsing but not for the evaluation of the expression.
But if you prefer the ParsingException I can also do.

@astefan what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Where possible, the arguments should be checked as soon as possible - the whole method checks the validity of the pattern right after parsing - checking its existence is one that was missed.
I see no advantages of postponing the check.

throw new ParsingException(source(ctx.value), "Pattern must not be [null]");
}
int pos = pattern.indexOf('*');
if (pos >= 0) {
throw new ParsingException(source(ctx.value),
Expand Down
10 changes: 10 additions & 0 deletions x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,19 @@
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Add;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Mul;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Sub;
import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue;

import java.time.Duration;
import java.time.Period;
import java.time.temporal.TemporalAmount;
import java.util.Collections;
import java.util.Locale;

import static java.lang.String.format;
import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN;
import static org.elasticsearch.xpack.ql.type.DataTypes.DOUBLE;
import static org.elasticsearch.xpack.ql.type.DataTypes.INTEGER;
import static org.elasticsearch.xpack.ql.type.DataTypes.KEYWORD;
import static org.elasticsearch.xpack.ql.type.DataTypes.LONG;
import static org.hamcrest.Matchers.startsWith;

Expand Down Expand Up @@ -542,4 +545,11 @@ public void testCaseWithOperand() {
assertEquals("WHEN 1 THEN 'one'", ifc.sourceText());
assertEquals("many", c.elseResult().toString());
}

public void testLikePatternWithNullParameterNotAllowed() {
ParsingException e = expectThrows(ParsingException.class,
() -> parser.createExpression("a LIKE ?",
Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null))));
assertEquals("line 1:9: Pattern must not be [null]", e.getMessage());
}
}