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

fix: Handle escape and regex characters #930

Merged
merged 4 commits into from
Sep 25, 2024
Merged

fix: Handle escape and regex characters #930

merged 4 commits into from
Sep 25, 2024

Conversation

saig0
Copy link
Member

@saig0 saig0 commented Sep 24, 2024

Description

Correct the handling of escape sequences in string literals.

Don't replace escape sequences in regex expressions, for example, \r or \n. In the parser, these sequences start with \. Same for \s, don't replace it with a whitespace, since this is also a part of a regex.

Handle \ to avoid that the sequence is escaped and returned as \\.

"\s\r\n"
// before: " \r\n"
// after:  "\\s\r\n"

"\\s\\n\\r"
// before: "\\ \\\n\\\r"
// after:  "\\s\\n\\r"

Related issues

closes #883
closes #879

Correct the handling of escape sequences in string literals.

Don't replace escape sequences in regex expressions, for example, \r or \n. In the parser, these sequences start with \\. Same for \s, don't replace it with a whitespace, since this is also a part of a regex.

Handle \\ to avoid that the sequence is escaped and returned as \\\\.
@saig0
Copy link
Member Author

saig0 commented Sep 25, 2024

@mboskamp I fixed the issue with \s character. I did an integration test in C8 and it looks good from my perspective. ✅

However, it is important to note that "\s" is not a valid string literal because \s is not an escape character. Instead, it should be "\\s". Same for \n and \r, if both characters are used for a regex then they should be escaped as \\n\\r. As a result, the string in the example DMN should be "\\s\\r\\n" instead of "\s\r\n".

Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

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

👍 Looks good. I did not test this in C7 and did not fully understand the fix. However, the test cases look good.

@saig0 saig0 added this pull request to the merge queue Sep 25, 2024
Merged via the queue into main with commit 0a11dc1 Sep 25, 2024
5 checks passed
@saig0 saig0 deleted the 879-escapa-chars branch September 25, 2024 11:03
@backport-action
Copy link
Collaborator

Backport failed for 1.16, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 1.16
git worktree add -d .worktree/backport-930-to-1.16 origin/1.16
cd .worktree/backport-930-to-1.16
git switch --create backport-930-to-1.16
git cherry-pick -x c8b01bd09f2cf6fca2bc789a2101001c99778480 58efca98cca80dcdb7d9dc28c2331e9991b63956 544c0388bb0ee8e9a3b078f4567941ce684a8f0a d266e80173126eaf6dbfee4333be6cb0833c39bf

@backport-action
Copy link
Collaborator

Successfully created backport PR for 1.17:

@backport-action
Copy link
Collaborator

Successfully created backport PR for 1.18:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEEL engine replaces regex expressions in output Incorrect escaping in strings
3 participants