Skip to content

[BOTS-2355] escape single quote at the beginning of the string#4

Merged
drarbego merged 2 commits into
wz-developmentfrom
bots-2355-Fix-single-quotes-not-being-properly-escaped-in-ParemetrizedQuery
Mar 26, 2019
Merged

[BOTS-2355] escape single quote at the beginning of the string#4
drarbego merged 2 commits into
wz-developmentfrom
bots-2355-Fix-single-quotes-not-being-properly-escaped-in-ParemetrizedQuery

Conversation

@drarbego

Copy link
Copy Markdown

What does this PR do?

Fixes error when escaping single quotes in queries

What was the issue?

the regular expression used to match unescaped single quotes (([^\\])) was not matching single quotes at the beginning of the string. so 'facebook', 'webchat' resulted in 'facebook\\', \\'webchat\\'

@drarbego drarbego requested review from diacus and juanitodread March 26, 2019 19:39
@drarbego drarbego changed the title [BOTS-2355] replace unescaped single quote at the beginning of the string [BOTS-2355] escape unescaped single quote at the beginning of the string Mar 26, 2019
@drarbego drarbego changed the title [BOTS-2355] escape unescaped single quote at the beginning of the string [BOTS-2355] escape single quote at the beginning of the string Mar 26, 2019

@juanitodread juanitodread left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

This regex is getting hard to maintain. Should we start to think of a different solution?

Comment thread bigquery/tests/test_query_builder.py Outdated
@drarbego drarbego merged commit 8c7fecc into wz-development Mar 26, 2019
@drarbego drarbego deleted the bots-2355-Fix-single-quotes-not-being-properly-escaped-in-ParemetrizedQuery branch March 26, 2019 22:12
@diacus

diacus commented Mar 26, 2019

Copy link
Copy Markdown

@juanitodread 🤔 May be, but I can't think in other scenarios for these kind of strings.

@juanitodread

Copy link
Copy Markdown

What about something simpler using replace?:

'given string'.replace("'", "\\'") # single quotes case
              .replace('"', '\\"') # double quotes case
              .replace... # Any new case

@diacus

diacus commented Mar 26, 2019

Copy link
Copy Markdown

Wouldn't that break already scaped strings?

@juanitodread

Copy link
Copy Markdown

It will if the string was already escaped... in that case json.dumps may be a better approach:

import json

>>> json.dumps("this string already 'contains' \'escaped\' quotes") # single quotes case
'"this string already \'contains\' \'escaped\' quotes"'
>>> json.dumps('this string already "contains" \"escaped\" quotes') # double quotes case
'"this string already \\"contains\\" \\"escaped\\" quotes"'

@diacus

diacus commented Mar 27, 2019

Copy link
Copy Markdown

I like it. Plus it might fix bug BOTS-2357 as well.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants