-
Notifications
You must be signed in to change notification settings - Fork 25.6k
EQL: Replace ?"..." with """...""" for unescaped strings #62539
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
Merged
matriv
merged 14 commits into
elastic:master
from
matriv:replace-unescaped-triple-doublequotes
Oct 2, 2020
Merged
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
32e85fd
EQL: Replace ?"..." with """...""" for unescaped strings
matriv 480aac5
change error message
matriv 356cabf
Merge remote-tracking branch 'upstream/master' into replace-unescaped…
matriv 374bc92
fix escaped '
matriv ae7d4ed
add more tests
matriv b32a383
Merge remote-tracking branch 'upstream/master' into replace-unescaped…
matriv fec23ce
fix tests
matriv af8ca3d
Try to allow """ with one escapedwq
matriv 3ada210
Merge remote-tracking branch 'upstream/master' into replace-unescaped…
matriv a8f4cbb
Use Python approach
matriv 60e1b4e
Merge remote-tracking branch 'upstream/master' into replace-unescaped…
matriv 822d494
Merge remote-tracking branch 'upstream/master' into replace-unescaped…
matriv 23ffe1f
Merge remote-tracking branch 'upstream/master' into replace-unescaped…
matriv f838bc2
Disallow any triple double quote sequence and everything is raw
matriv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Oops, something went wrong.
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.
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 think this could really come to bite us. Mostly because it means that a raw string is not a raw string. With this, there's no way to express a literal
\"anywhere in the string.This branch allows a literal
\":matriv/elasticsearch@replace-unescaped-triple-doublequotes...rw-access:raw-string-changes
Uh oh!
There was an error while loading. Please reload this page.
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.
This grammar has other issues:
=>
and with the proposed solution you can have a
\"literal if you double escape it\\"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.
We could replace
\"with"only if part of an escaped triple double quote sequence, but I think that produces more confusion for a user.Uh oh!
There was an error while loading. Please reload this page.
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.
but it should give that syntax error because it's invalid syntax. the string was completed after seeing
""".that sounds like the correct behavior. If we introduce any escapes at all we defeat the purpose of this being a raw string, and require some potentially complex parsing logic and strange edge cases. although I don't think we should stop immediately when seeing
"""because then a string can't end with""". we could process a few more quotes after without issue. then a string will be able to end with quotes, too.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.
"""""hello\"""world!\""""it's a valid syntax, it has a closing""".The previous
?"couldn't accept any"and every"should be escaped.With this approach we only need to escape a
"""which is very rare to occur.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.
There are a number of issues at hand:
"is a common character. To avoid escaping it too often, the raw string relies on 3 triple quotes""". This however can become a problem at the end of the raw string where a trailing"can mess up quoting"are allowed inside and can be escaped to avoid closing a string"""through\. This again is it occurs at the end of the string.To wit:
""" dir "c:\dir""""<-- 4 ending quotes""" dir c:\dir\"""<-- \ escapes the"""Due to file paths on windows 2, aka
\"is likely a common occurrence.I see several options to solve this:
A. Require raw strings to not end with
"and advice folks to add a space for example:""" dir "c:\dir" """B. Use a different escape character from
\, say|. It solves 2 but not 1.C. Do not allow escaping inside raw strings. This solves 2 but not 1.
D. Use a different set for quoting characters. Say "?""
and""?`. However this does not solve the problem, rather just minimizes the occurrences in which they appear. Which might be a win in itself.Considering
\is traditionally used for escaping and in Window paths, I have a preference for C over B. That's because introducing a new character for escaping is surprising and might backfire again depending on the raw string .B however mean that one cannot escape
"""inside a raw string but then for that there's the regular string definition. Unfortunately this doesn't solve 1 either...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.
A. I thing introducing a mandatory whitespace can lead to other issues, and is it only one space? what happens with more spaces, we keep all but one?, or with tabs?
B. I also don't like a new escape char, doesn't address the main issue no1.
D. I definitely don't like
Dsince it still contains the?char which usually refers to regex or query param.C. I'm ok with C so if a user needs to escape chars or needs a
""", he/she has to resort to the normal"....."syntax and properly escape what's needed.Uh oh!
There was an error while loading. Please reload this page.
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.
option E:
Allow for two optional additional trailing quotes with no escape sequences. then you can solve the trailing quotes issue without escapes.
The string can't contain
"""anywhere inside. If you ever need to write a string with that sequence, a raw string is insufficient, so users will need to switch to a traditional escaped"string.and it doesn't suffer from these contextual issues with

\"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.
confirmed that
"""isn't allowed anywhere in the string but""is;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.
@rw-access I'm also happy with that, pushed the change and fixed all relevant tests.