-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
SQL: Fix NPE for parameterized LIKE/RLIKE #53573
Conversation
Fix NPE when `null` is passed as a parameter for a parameterized pattern of LIKE/RLIKE. e.g.: `field LIKE ?` params=[null]` Check for null pattern in LIKE/RLIKE as for RLIKE (RegexpQuery) we get an IllegalArgumentExpression from Lucence but for LIKE (WildcardQuery) we get an NPE. Fixes: elastic#53557
Pinging @elastic/es-search (:Search/SQL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice quick fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -263,6 +263,9 @@ public LikePattern visitPattern(PatternContext ctx) { | |||
} | |||
|
|||
String pattern = string(ctx.value); | |||
if (pattern == null) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astefan isn't an IT a bit too much for this null check? |
Fix NPE when `null` is passed as a parameter for a parameterized pattern of LIKE/RLIKE. e.g.: `field LIKE ?` params=[null]` Check for null pattern in LIKE/RLIKE as for RLIKE (RegexpQuery) we get an IllegalArgumentExpression from Lucence but for LIKE (WildcardQuery) we get an NPE. Fixes: #53557 (cherry picked from commit ec3481e)
Fix NPE when `null` is passed as a parameter for a parameterized pattern of LIKE/RLIKE. e.g.: `field LIKE ?` params=[null]` Check for null pattern in LIKE/RLIKE as for RLIKE (RegexpQuery) we get an IllegalArgumentExpression from Lucence but for LIKE (WildcardQuery) we get an NPE. Fixes: #53557 (cherry picked from commit ec3481e)
Fix NPE when
null
is passed as a parameter for a parameterizedpattern of LIKE/RLIKE. e.g.:
field LIKE ?
params=[null]`Check for null pattern in LIKE/RLIKE as for RLIKE (RegexpQuery) we
get an IllegalArgumentExpression from Lucence but for LIKE
(WildcardQuery) we get an NPE.
Fixes: #53557