EQL: Replace ?"..." with """...""" for unescaped strings #62539

matriv · 2020-09-17T12:44:38Z

Use triple double quotes enclosing a string literal to interpret it
as unescaped, in order to use ? for marking query params and avoid
user confusion.

Relates to #61659

Use triple doulbe quotes enclosing a string literal to interpret it as unescaped, in order to use `?` for marking query params and avoid user confusion. Relates to elastic#61659

elasticmachine · 2020-09-17T12:44:40Z

Pinging @elastic/es-ql (:Query Languages/EQL)

matriv · 2020-09-17T12:45:15Z

/cc @rw-access
/cc @jrodewig

matriv · 2020-09-17T12:48:02Z

Currently the usage of ?'.......' is handled with the error message regarding the single quotes.
Personally I prefer this to handling it as a specific case alongside ?"......", but let me know if you have a different opinion.

costin

Please add some parser tests to validate the error being thrown with the old and new format.

costin · 2020-09-17T13:03:17Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/AbstractBuilder.java

@@ -122,10 +122,18 @@ public static String unquoteString(Source source) {
            return null;
        }

-        // unescaped strings can be interpreted directly
+        // catch old method of ?" and ?' to define unescaped strings
        if (text.startsWith("?")) {
            checkForSingleQuotedString(source, text, 1);


This is incorrect - in case of ?' the error message is invalid - it tells that double quotes should be used which is not the case.
In fact I would argue that since the string is always quoted, if it starts with ? an exception should be thrown regardless of what follows it.

That's true, It's a 2-step error for the user: ?' -> ?" -> """. I just thought that explicitly handling the single quotes (no matter of the preceding ?) was a "better" approach. I'll gladly change it.

matriv · 2020-09-17T13:19:27Z

@costin other that this: https://github.com/elastic/elasticsearch/pull/62539/files#diff-8b3f6645d4cf54ff461c3e337b4bbe84R108 ?

rw-access

I don't think this has been decided as a new syntax so I would like to hold off on merging and run this by the EQL rules team.

rw-access · 2020-09-17T17:22:49Z

x-pack/plugin/eql/src/main/antlr/EqlBase.g4

@@ -195,6 +195,7 @@ STRING
    | '"'   ('\\' [btnfr"'\\] | ~[\r\n"\\])* '"'
    | '?"'  ('\\"' |~["\r\n])* '"'
    | '?\'' ('\\\'' |~['\r\n])* '\''
+    | '"""' ('\\\'' |~['\r\n])* '"""'


Looks like ' needs to be escaped under this format.
I also have some questions about the desired behavior of triple double quotes:

How would each of these be interpreted? I think we should know our edge cases up front. I'm wondering what the implication is for " at the beginning or at the end of a string. And how many of them are supported

""""""

"""""x"""

"""x"""""

""""""""

Looks like ' needs to be escaped under this format.

Correct, I rushed and overlooked that.

I also have some questions about the desired behavior of triple double quotes:

How would each of these be interpreted? I think we should know our edge cases up front. I'm wondering what the implication is for " at the beginning or at the end of a string. And how many of them are supported

""""""

"""""x"""

"""x"""""

""""""""

The extra " in the beginning or end of the string would be part of the final string:

`` -> empty string

""x

x""

""

rw-access · 2020-09-17T17:25:08Z

in order to use ? for marking query params and avoid
user confusion.

there is no grammatical ambiguity for a lone ?. it was passing tests here: https://github.com/elastic/elasticsearch/pull/52301/files#diff-75fcba04994edcd265cc994992f6124bR113

sure there may be an ambiguity from the user's expectation of what ? means, but since that's deprecated that should no longer be a problem. you should be safe to pursue ? as a templated param without needing to make any other grammatical changes

matriv · 2020-09-18T08:42:35Z

there is no grammatical ambiguity for a lone ?. it was passing tests here: https://github.com/elastic/elasticsearch/pull/52301/files#diff-75fcba04994edcd265cc994992f6124bR113

sure there may be an ambiguity from the user's expectation of what ? means, but since that's deprecated that should no longer be a problem. you should be safe to pursue ? as a templated param without needing to make any other grammatical changes

That's true, It's only to have a clear user of ? only for the params, there's no problem with the grammar, that's why I mentioned "user confusion", not implementation wise.

…-triple-doublequotes

costin

LGTM

rw-access · 2020-09-18T14:23:14Z

x-pack/plugin/eql/src/main/antlr/EqlBase.g4

@@ -199,6 +199,7 @@ STRING
    | '"'   ('\\' [btnfr"'\\] | ~[\r\n"\\])* '"'
    | '?"'  ('\\"' |~["\r\n])* '"'
    | '?\'' ('\\\'' |~['\r\n])* '\''
+    | '"""' ('\\"' |~['\r\n])* '"""'


This sttill doesn't seem right and needs more tests.
Under this regex, you start and end with """, but you can't have any ' characters in between. In addition, you can have an infinite number of " characters. This would mean """"""""""""""" is valid syntax. But I'm not sure that's good behavior.

rw-access

I think this particular implementation needs some more tests and an updated regex.

Regardless of the implementation though, I think we still need to hold off on merging this PR. I think the turnaround was impressive, but that we skipped a few crucial steps. Using """ for quoting literal strings went from proposal straight to implementation, but I think we still need the opportunity for a discussion, ironing out the behavior for edge cases, and ultimately agreement.

The specification going forward

I'm thinking that any changes to the EQL specification, like this example, should be collectively deliberated and agreed upon. I'm not completely sure if this syntax is desirable, and would like some more time to explore options. And when things merge, it's harder to back them out. Especially for 7.10+ changes or whenever we land as beta.

I think that all of the interested bodies should have weight, and I'm wondering if our existing syncs are sufficient to discuss changes to the specification. An idea was pitched on this PR elastic/beats#20994 (comment) to separate out the specification into a separate repo. I think this could help draw clearer lines between implementation and specification.

Thoughts @costin @philkra @paulewing @tsg?

matriv · 2020-09-21T09:45:24Z

Please see my comment here

…-triple-doublequotes

rw-access · 2020-09-28T15:50:38Z

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/parser/ExpressionTests.java

+                e.getMessage());
+    }
+
+    public void testTripleDoubleQuotedUnescapedString() {


how is """hello""" == """world""" interpreted?

it should be equivalent to this expression Equals("hello", "world):
"hello" == "world"

I can add a test for that

rw-access · 2020-09-28T15:52:00Z

x-pack/plugin/eql/src/main/antlr/EqlBase.g4

@@ -200,6 +200,7 @@ STRING
    | '"'   ('\\' [btnfr"'\\] | ~[\r\n"\\])* '"'
    | '?"'  ('\\"' |~["\r\n])* '"'
    | '?\'' ('\\\'' |~['\r\n])* '\''
+    | '"""' (~['\r\n])* '"""'


This shouldn't need to forbid ' inside the string.

Suggested change

| '"""' (~['\r\n])* '"""'

| '"""' (~[\r\n])* '"""'

…-triple-doublequotes

matriv · 2020-10-01T16:44:58Z

@rw-access Was right about the greediness, so in my previous implementation an expression like:

"""foo""" == """bar"""

would be parsed as a literal:

foo""" == """bar

So instead I've pushed the python approach that within the """....""" you can have anything except for 3 double quotes in a raw, one of them at least must be escaped with \. That means that at the end of string you always need to escape the last " that is adjacent to the closing """:
Wrong: """foo"""", correct: """foo\""""

Please advice for any more tests than the current ones.

/cc @jrodewig

rw-access · 2020-10-01T16:51:13Z

x-pack/plugin/eql/src/main/antlr/EqlBase.g4

@@ -200,6 +200,7 @@ STRING
    | '"'   ('\\' [btnfr"'\\] | ~[\r\n"\\])* '"'
    | '?"'  ('\\"' |~["\r\n])* '"'
    | '?\'' ('\\\'' |~['\r\n])* '\''
+    | '"""' ('\\"' |~[\r\n])*? '"""'


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

This grammar has other issues:

"""""hello\"""world!\"""" == """"foo"\""bar""\""""

=>

org.elasticsearch.xpack.eql.parser.ParsingException: line 1:21: token recognition error at: '!\'

and with the proposed solution you can have a \" literal if you double escape it \\"

We could replace \" with " only if part of an escaped triple double quote sequence, but I think that produces more confusion for a user.

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.

"""""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 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...

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 D since 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.

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.

"""[^\r\n]*?""""?"?

and it doesn't suffer from these contextual issues with \"

confirmed that """ isn't allowed anywhere in the string but "" is;

@rw-access I'm also happy with that, pushed the change and fixed all relevant tests.

…-triple-doublequotes

matriv · 2020-10-02T09:47:33Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

rw-access

LGTM!
thanks for your endurance!

matriv · 2020-10-02T11:03:45Z

thanks for your endurance!

Thank you too from my side!

matriv · 2020-10-02T11:29:06Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

matriv · 2020-10-02T11:44:39Z

@jrodewig So finally the change is that we replace ?"......." with """.....""".
Everything inside """....""" is allowed as is (raw) and the only thing that is not allowed is triple double quotes as those are considered to be marking the end of the raw string. So if one has """ in their string they need to use the usual string literal syntax: "....." and properly escape it, e.g.: "foo\"\"\"bar" or "foobar\"\"\""

…3174) Use triple double quotes enclosing a string literal to interpret it as unescaped, in order to use `?` for marking query params and avoid user confusion. `?` also usually implies regex expressions. Any character inside the `"""` beginning-closing markings is considered raw and the only thing that is not permitted is the `"""` sequence itself. If a user wants to use that, needs to resort to the normal `"` string literal and use proper escaping. Relates to #61659 (cherry picked from commit d87c2ca)

costin · 2020-10-02T13:52:19Z

Woohoo

EQL: Replace ?"..." with """...""" for unescaped strings

32e85fd

Use triple doulbe quotes enclosing a string literal to interpret it as unescaped, in order to use `?` for marking query params and avoid user confusion. Relates to elastic#61659

matriv added >breaking v8.0.0 :Analytics/EQL EQL querying v7.10.0 labels Sep 17, 2020

matriv requested review from costin and astefan September 17, 2020 12:44

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Sep 17, 2020

costin requested changes Sep 17, 2020

View reviewed changes

rw-access suggested changes Sep 17, 2020

View reviewed changes

change error message

480aac5

rw-access reviewed Sep 17, 2020

View reviewed changes

matriv and others added 2 commits September 18, 2020 12:44

Merge remote-tracking branch 'upstream/master' into replace-unescaped…

356cabf

…-triple-doublequotes

fix escaped '

374bc92

matriv requested review from costin and rw-access September 18, 2020 10:25

costin approved these changes Sep 18, 2020

View reviewed changes

rw-access reviewed Sep 18, 2020

View reviewed changes

rw-access suggested changes Sep 18, 2020

View reviewed changes

rw-access mentioned this pull request Sep 18, 2020

EQL: Replace [`] with ['] for identifiers #62638

Closed

costin mentioned this pull request Sep 18, 2020

Significant EQL changes in 7.10 #62652

Closed

7 tasks

costin mentioned this pull request Sep 22, 2020

EQL: Replace ?" and ?' with """ #62645

Closed

add more tests

ae7d4ed

matriv added 2 commits September 28, 2020 18:23

Merge remote-tracking branch 'upstream/master' into replace-unescaped…

b32a383

…-triple-doublequotes

fix tests

fec23ce

rw-access reviewed Sep 28, 2020

View reviewed changes

matriv and others added 4 commits September 29, 2020 19:47

Try to allow """ with one escapedwq

af8ca3d

Merge remote-tracking branch 'upstream/master' into replace-unescaped…

3ada210

…-triple-doublequotes

Use Python approach

a8f4cbb

Merge remote-tracking branch 'upstream/master' into replace-unescaped…

60e1b4e

…-triple-doublequotes

matriv requested review from costin and rw-access October 1, 2020 16:45

rw-access reviewed Oct 1, 2020

View reviewed changes

matriv added 2 commits October 1, 2020 19:53

Merge remote-tracking branch 'upstream/master' into replace-unescaped…

822d494

…-triple-doublequotes

Merge remote-tracking branch 'upstream/master' into replace-unescaped…

23ffe1f

…-triple-doublequotes

Disallow any triple double quote sequence and everything is raw

f838bc2

matriv requested a review from rw-access October 2, 2020 11:00

rw-access approved these changes Oct 2, 2020

View reviewed changes

matriv merged commit d87c2ca into elastic:master Oct 2, 2020

matriv deleted the replace-unescaped-triple-doublequotes branch October 2, 2020 12:00

jrodewig mentioned this pull request Oct 2, 2020

[DOCS] EQL: Replace ?"..." with """...""" for raw strings #63191

Merged

pgomulka mentioned this pull request Feb 11, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

EQL: Replace ?"..." with """...""" for unescaped strings #62539

EQL: Replace ?"..." with """...""" for unescaped strings #62539

Conversation

matriv commented Sep 17, 2020 • edited Loading

elasticmachine commented Sep 17, 2020

matriv commented Sep 17, 2020

matriv commented Sep 17, 2020

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Sep 17, 2020

rw-access left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw-access commented Sep 17, 2020

matriv commented Sep 18, 2020

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw-access left a comment • edited Loading

Choose a reason for hiding this comment

The specification going forward

matriv commented Sep 21, 2020 • edited Loading

rw-access Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw-access Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

matriv commented Oct 1, 2020

Choose a reason for hiding this comment

matriv Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw-access Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw-access Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Oct 2, 2020

rw-access left a comment

Choose a reason for hiding this comment

matriv commented Oct 2, 2020

matriv commented Oct 2, 2020

matriv commented Oct 2, 2020 • edited Loading

costin commented Oct 2, 2020

matriv commented Sep 17, 2020 •

edited

Loading

rw-access left a comment •

edited

Loading

matriv commented Sep 21, 2020 •

edited

Loading

rw-access Sep 28, 2020 •

edited

Loading

rw-access Sep 28, 2020 •

edited

Loading

matriv Oct 1, 2020 •

edited

Loading

rw-access Oct 2, 2020 •

edited

Loading

rw-access Oct 2, 2020 •

edited

Loading

matriv commented Oct 2, 2020 •

edited

Loading