Skip to content
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

EQL: Forbid usage of ['] for string literals #62458

Merged
merged 5 commits into from
Sep 16, 2020

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Sep 16, 2020

The usage of single quotes to wrap a string literal is forbidden
and an error encouraging the user to user double quotes is returned.

Tests are properly adjusted.

Relates to #61659

The usage of single quotes to wrap a string literal is forbidden
and an error encouraging the user to user double quotes is returned.

Tests are properly adjusted.

Related to elastic#61659
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Sep 16, 2020
@matriv
Copy link
Contributor Author

matriv commented Sep 16, 2020

Currently, the grammar is not touched so we have a nice error message to the user instead of an automated Parser generated exception. Also I choose to have the same message for ' and ?', but we can easily differentiate if needed.

Moreover, we chose to send an error instead of a deprecation log message so that it's easily visible to the user, who in turn should change existing queries to accommodate the change and therefore free the ' for possible usage around identifiers (instead of the backquote).

/cc @rw-access

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -178,7 +178,7 @@ public void testEqlRestUsage() throws IOException {
" [process where serial_event_id < 4] by exit_code" +
" [process where opcode == 1] by user" +
" [process where opcode == 2] by user" +
" [file where parent_process_name == 'file_delete_event'] by exit_code" +
" [file where parent_process_name == \\\"file_delete_event\\\"] by exit_code" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why \\\" and not just \"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of the runEql() which constructs a json body, so with \" it's escaped in the java string but then it's interpreted as a closing " in json.

private static void checkForSingleQuotedString(Source source, String text, int i) {
if (text.charAt(i) == '\'') {
throw new ParsingException(source,
"Single quotes ['] are not supported around literal strings, please use double quotes [\"]");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use double quotes ["] for defining string literals, not single quotes [']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make it: Use double quotes ["] to define string literals, not single quotes [']

@matriv matriv merged commit 8be400b into elastic:master Sep 16, 2020
@matriv matriv deleted the forbid-single-quotes branch September 16, 2020 19:22
matriv added a commit that referenced this pull request Sep 17, 2020
The usage of single quotes to wrap a string literal is forbidden
and an error encouraging the user to user double quotes is returned.

Tests are properly adjusted.

Relates to #61659

(cherry picked from commit 8be400b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants