Escape single quotes with 2 backslashes #56
Conversation
courtneyholcomb
requested review from
susodapop,
arikfr,
moderakh and
yunbodeng-db
as code owners
|
Thanks for this contribution! We're actually bundling a few changes around this behaviour into a larger pull request here shortly. I think per the spec defined in the comments on #51, the escape sequence should be a single back stroke I'm going to keep this PR open while we finish up our internal version. This way we can incorporate your e2e test onto our branch and make sure it still passes (we've been using unit tests defined in #46. This way you'll also get contributor credit on the repo :) |
| # (i.e. only special character is single quote) | ||
| return "'{}'".format(item.replace("'", "''")) | ||
| return "'{}'".format(item.replace("'", "\\'")) |
There was a problem hiding this comment.
i know that per @susodapop this will be fixed elsewhere, but want to point out that the backslash character itself also needs to be escaped
i think the correct replacement would be: item.replace('\\','\\\\' ).replace("'", "\\'")
| @@ -288,6 +288,20 @@ def test_get_columns(self): | |||
| for table in table_names: | |||
| cursor.execute('DROP TABLE IF EXISTS {}'.format(table)) | |||
|
|
|||
| def test_escape_single_quotes(self): | |||
There was a problem hiding this comment.
this is great, probably want the same type of test for a string with a backslash in it, as in
test_str = "Many Linux users are upset that Windows uses \\ to separate path elements"
| with self.cursor({}) as cursor: | ||
| table_name = 'table_{uuid}'.format(uuid=str(uuid4()).replace('-', '_')) | ||
| # Test escape syntax directly | ||
| cursor.execute("CREATE TABLE IF NOT EXISTS {} AS (SELECT 'you\\'re' AS col_1)".format(table_name)) |
There was a problem hiding this comment.
Just a note: this e2e test is really testing how DBSQL behaves rather than our connector. The connector escape behaviour is only called to escape parameters passed to the Connection.execute() method.
This is a good test for demonstrating how to work with DBSQL. But the dynamic is not unique to databricks-sql-connector as we observe the same with NodeJS, Go, and ODBC/JDBC.
Signed-off-by: Jesse Whitehouse <[email protected]>
|
I've incorporated your e2e test from this PR into #46 which includes your change plus a bunch of additional test cases. Closing this PR in favour of that one. Thank you for this contribution! |
* Refactor so we can unit test `inject_parameters` * Add unit tests for inject_parameters * Remove inaccurate comment. Per #51, spark sql does not support escaping a single quote with a second single quote. * Closes #51 and adds unit tests plus the integration test provided in #56 Signed-off-by: Jesse Whitehouse <[email protected]> Co-authored-by: Courtney Holcomb (@courtneyholcomb) Co-authored-by: @mcannamela
Fixes #51 by escaping single quotes with backslashes.