Fix painless's regex lexer and error messages #23634
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Without this change, if write a script with multiple regexes *sometimes* the lexer will decide to look at them like one big regex and then some trailing garbage. Like this discuss post: https://discuss.elastic.co/t/error-with-the-split-function-in-painless-script/79021 ``` def val = /\\\\/.split(ctx._source.event_data.param17); if (val[2] =~ /\\./) { def val2 = /\\./.split(val[2]); ctx._source['user_crash'] = val2[0] } else { ctx._source['user_crash'] = val[2] } ``` The error message you get from the lexer is `lexer_no_viable_alt_exception` right after the *second* regex. With this change each regex is just a single regex like it ought to be. As a bonus, while looking into this issue I found that the error reporting for regexes wasn't very nice. If you specify an invalid pattern then you get an error marker on the start of the pattern with the JVM's regex error message which attempts to point you to the location in the regex but is totally unreadable in the JSON response. This change fixes the location to point to the appropriate spot inside the pattern and removes the portion of the JVM's error message that doesn't render well. It is no longer needed now that we point users to the appropriate spot in the pattern.
nik9000
commented
Mar 17, 2017
|
|
||
| STRING: ( '"' ( '\\"' | '\\\\' | ~[\\"] )*? '"' ) | ( '\'' ( '\\\'' | '\\\\' | ~[\\'] )*? '\'' ); | ||
| REGEX: '/' ( ~('/' | '\n') | '\\' ~'\n' )+ '/' [cilmsUux]* { slashIsRegex() }?; | ||
| REGEX: '/' ( '\\' ~'\n' | ~('/' | '\n') )+? '/' [cilmsUux]* { slashIsRegex() }?; |
Member
Author
There was a problem hiding this comment.
This non-greedy pattern works well for strings so I figured I should use it for regexes as well. I had to reorder the escape characters to the front so they wouldn't be ignored because order is important in non-greedy rules.
| } | ||
|
|
||
| public void testRegexIsNonGreedy() { | ||
| assertEquals(true, exec("def s = /\\\\/.split('.\\\\.'); return s[1] ==~ /\\./")); |
Member
Author
There was a problem hiding this comment.
This blew up mightily without the lexer change.
| public void testSlashesEscapePattern() { | ||
| assertEquals(true, exec("return '//' ==~ /\\/\\//")); | ||
| public void testBackslashEscapesForwardSlash() { | ||
| assertEquals(true, exec("'//' ==~ /\\/\\//")); |
Member
Author
There was a problem hiding this comment.
This one blows up if you don't reorder the lexer rules after you switch it to non-greedy.
| } catch (PatternSyntaxException exception) { | ||
| throw createError(exception); | ||
| } catch (PatternSyntaxException e) { | ||
| throw new Location(location.getSourceName(), location.getOffset() + 1 + e.getIndex()).createError( |
Member
Author
There was a problem hiding this comment.
I found this while debugging this issue and it bothered me more than it should have. I'm quite happy PatternSyntaxException seems designed for you to extract this information.
The 1 is for the leading slash.
Contributor
There was a problem hiding this comment.
Is it not possible to fix this value where the Location object is originally generated?
Member
Author
There was a problem hiding this comment.
I need the offset from the exception to get the location right. The +1 bit is to get the leading /. What kind of fix were you thinking about? An override of createError maybe?
jdconrad
approved these changes
Mar 17, 2017
Contributor
jdconrad
left a comment
jdconrad
left a comment
There was a problem hiding this comment.
LGTM! Thanks for fixing this. Left one minor comment.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Mar 22, 2017
* master: Docs: Fix language on a few snippets Painless: Fix regex lexer and error messages (elastic#23634) Skip 5.4 bwc test for new name for now Count through the primary in list of strings test Skip testing new name if it isn't known Wait for all shards in list of strings test Deprecate request_cache for clear-cache (elastic#23638)
nik9000
added a commit
that referenced
this pull request
Mar 22, 2017
Without this change, if write a script with multiple regexes *sometimes* the lexer will decide to look at them like one big regex and then some trailing garbage. Like this discuss post: https://discuss.elastic.co/t/error-with-the-split-function-in-painless-script/79021 ``` def val = /\\\\/.split(ctx._source.event_data.param17); if (val[2] =~ /\\./) { def val2 = /\\./.split(val[2]); ctx._source['user_crash'] = val2[0] } else { ctx._source['user_crash'] = val[2] } ``` The error message you get from the lexer is `lexer_no_viable_alt_exception` right after the *second* regex. With this change each regex is just a single regex like it ought to be. As a bonus, while looking into this issue I found that the error reporting for regexes wasn't very nice. If you specify an invalid pattern then you get an error marker on the start of the pattern with the JVM's regex error message which attempts to point you to the location in the regex but is totally unreadable in the JSON response. This change fixes the location to point to the appropriate spot inside the pattern and removes the portion of the JVM's error message that doesn't render well. It is no longer needed now that we point users to the appropriate spot in the pattern.
clintongormley
added
:Core/Infra/Scripting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without this change, if write a script with multiple regexes
sometimes the lexer will decide to look at them like one
big regex and then some trailing garbage. Like this discuss post:
https://discuss.elastic.co/t/error-with-the-split-function-in-painless-script/79021
The error message you get from the lexer is
lexer_no_viable_alt_exceptionright after the second regex.
With this change each regex is just a single regex like it ought to be.
As a bonus, while looking into this issue I found that the error
reporting for regexes wasn't very nice. If you specify an invalid
pattern then you get an error marker on the start of the pattern
with the JVM's regex error message which attempts to point you to the
location in the regex but is totally unreadable in the JSON response.
This change fixes the location to point to the appropriate spot
inside the pattern and removes the portion of the JVM's error message
that doesn't render well. It is no longer needed now that we point
users to the appropriate spot in the pattern.