Skip to content

SQL: Fix NPE for parameterized LIKE/RLIKE #53573

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

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 1 commit
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
10 changes: 9 additions & 1 deletion ...in/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import java.util.Objects;

import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact;

public abstract class RegexMatch<T> extends UnaryScalarFunction {
Expand Down Expand Up @@ -46,7 +47,14 @@ public Nullability nullable() {

@Override
protected TypeResolution resolveType() {
return isStringAndExact(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT);
TypeResolution resolution = isStringAndExact(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT);
if (resolution.unresolved()) {
return resolution;
}
if (pattern == null) {
return new TypeResolution(format(null, "[{}] pattern must not be [null]", sourceText()));
}
return TypeResolution.TYPE_RESOLVED;
}

@Override
Expand Down
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

@costin costin Mar 16, 2020

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

@costin costin Mar 16, 2020

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.

return null;
}
int pos = pattern.indexOf('*');
if (pos >= 0) {
throw new ParsingException(source(ctx.value),
Expand Down
28 changes: 26 additions & 2 deletions ...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@
import org.elasticsearch.xpack.sql.expression.predicate.conditional.Least;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.NullIf;
import org.elasticsearch.xpack.sql.parser.SqlParser;
import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue;
import org.elasticsearch.xpack.sql.stats.Metrics;

import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;

Expand All @@ -44,12 +47,21 @@ public class VerifierErrorMessagesTests extends ESTestCase {
loadMapping("mapping-multi-field-with-nested.json")));

private String error(String sql) {
return error(indexResolution, sql);
return error(indexResolution, sql, Collections.emptyList());
}

private String error(String sql, List<SqlTypedParamValue> params) {
return error(indexResolution, sql, params);
}

private String error(IndexResolution getIndexResult, String sql) {
return error(getIndexResult, sql, Collections.emptyList());
}

private String error(IndexResolution getIndexResult, String sql, List<SqlTypedParamValue> params) {
Analyzer analyzer = new Analyzer(SqlTestUtils.TEST_CFG, new SqlFunctionRegistry(), getIndexResult, new Verifier(new Metrics()));
VerificationException e = expectThrows(VerificationException.class, () -> analyzer.analyze(parser.createStatement(sql), true));
VerificationException e = expectThrows(VerificationException.class,
() -> analyzer.analyze(parser.createStatement(sql, params), true));
String message = e.getMessage();
assertTrue(message.startsWith("Found "));
String pattern = "\nline ";
Expand Down Expand Up @@ -745,12 +757,24 @@ public void testInvalidTypeForLikeMatch() {
error("SELECT * FROM test WHERE text LIKE 'foo'"));
}

public void testInvalidPatternForLikeMatch() {
assertEquals("1:26: [keyword LIKE ?] pattern must not be [null]",
error("SELECT * FROM test WHERE keyword LIKE ?",
Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null))));
}

public void testInvalidTypeForRLikeMatch() {
assertEquals("1:26: [text RLIKE 'foo'] cannot operate on field of data type [text]: " +
"No keyword/multi-field defined exact matches for [text]; define one or use MATCH/QUERY instead",
error("SELECT * FROM test WHERE text RLIKE 'foo'"));
}

public void testInvalidPatternForRLikeMatch() {
assertEquals("1:26: [keyword RLIKE ?] pattern must not be [null]",
error("SELECT * FROM test WHERE keyword RLIKE ?",
Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null))));
}

public void testMatchAndQueryFunctionsNotAllowedInSelect() {
assertEquals("1:8: Cannot use MATCH() or QUERY() full-text search functions in the SELECT clause",
error("SELECT MATCH(text, 'foo') FROM test"));
Expand Down
12 changes: 12 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 @@ -13,6 +13,7 @@
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullEquals;
import org.elasticsearch.xpack.ql.expression.predicate.regex.Like;
import org.elasticsearch.xpack.sql.SqlTestUtils;
import org.elasticsearch.xpack.sql.expression.function.scalar.Cast;
import org.elasticsearch.xpack.sql.expression.literal.interval.Interval;
Expand All @@ -21,16 +22,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 +546,12 @@ public void testCaseWithOperand() {
assertEquals("WHEN 1 THEN 'one'", ifc.sourceText());
assertEquals("many", c.elseResult().toString());
}

public void testLikePatternWithNullParameter() {
Expression expr = parser.createExpression("a LIKE ?",
Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null)));
assertEquals(Like.class, expr.getClass());
Like like = (Like) expr;
assertNull(like.pattern());
}
}