GeezerVison is showing github.com — single quote not properly escaped · Issue #51 · databricks/databricks-sql-python
single quote not properly escaped #51DescriptionWhat
ParamEscaper's escape_string() gives incorrect behavior on Databricks SQL and in Databricks notebooks.
It replaces a single quote ' with '', but the correct way to escape ' is with a backslash, like \'.
You can verify in PySpark with:
assert spark.sql("select 'cat''s meow' as my_col").head(1)[0]['my_col'] == "cats meow"
assert spark.sql("select 'cat\\'s meow' as my_col").head(1)[0]['my_col'] == "cat's meow"
Note that because it starts as a Python literal, we need two backslashes \\ to get Python to first escape \\' to \' and then Spark escapes to '.
I don't know what the motivation for this implementation was, but the result seems to be concatenation instead of escaping the quote character.
Reproduction in databricks-sql-python
The following demonstrates the issue in version 1.2.2 2.0.5 of databricks-sql-python against a serverless SQL warehouse in Azure, v2022.30, plus an implementation without parameter substitution showing an escape treatment that does work :
from typing import List
from databricks import sql
import os
from databricks.sql import ServerOperationError
server_hostname= os.environ.get('DBT_DATABRICKS_HOST')
http_path=f'/sql/1.0/endpoints/{os.environ.get("DBT_DATABRICKS_ENDPOINT")}'
access_token=os.environ.get('DBT_DATABRICKS_TOKEN')
user_agent_entry = "dbt-databricks/1.2.2"
connection = sql.connect(
server_hostname=server_hostname,
http_path=http_path,
access_token=access_token,
_user_agent_entry=user_agent_entry
)
cursor = connection.cursor()
def get_result_using_parameter_bindings(p:List[str]):
try:
cursor.execute('select %s as my_col', p)
result = list(cursor.fetchall())[0]['my_col']
except ServerOperationError as exc:
result = exc.message.strip()[:20] + '...'
return result
def get_result_using_fstring(p:List[str]):
try:
escaped = p[0].replace('\\','\\\\' ).replace("'", "\\'")
cursor.execute(f"select '{escaped}' as my_col")
result = list(cursor.fetchall())[0]['my_col']
except ServerOperationError as exc:
result = exc.message.strip()[:20] + '...'
return result
params = [
["cat's meow"],
["cat\'s meow"],
["cat\\'s meow"],
["cat''s meow"],
]
for p in params:
# using dbt-databricks-sql's parameter substitution
param_binding_result = get_result_using_parameter_bindings(p)
# using manually built and escaped query
f_string_result = get_result_using_fstring(p)
print('\nparameter value:', p[0], 'parameter-binding result:', param_binding_result, 'round-trip ok?', p[0]==param_binding_result)
print('parameter value:', p[0], 'f-string result:', f_string_result, 'round-trip ok?',
p[0] == f_string_result
)
assert p[0] == f_string_result
cursor.close()
connection.close()The output is:
bash_1 | parameter value: cat's meow parameter-binding result: cats meow round-trip ok? False
bash_1 | parameter value: cat's meow f-string result: cat's meow round-trip ok? True
bash_1 |
bash_1 | parameter value: cat's meow parameter-binding result: cats meow round-trip ok? False
bash_1 | parameter value: cat's meow f-string result: cat's meow round-trip ok? True
bash_1 |
bash_1 | parameter value: cat\'s meow parameter-binding result: [PARSE_SYNTAX_ERROR]... round-trip ok? False
bash_1 | parameter value: cat\'s meow f-string result: cat\'s meow round-trip ok? True
bash_1 |
bash_1 | parameter value: cat''s meow parameter-binding result: cats meow round-trip ok? False
bash_1 | parameter value: cat''s meow f-string result: cat''s meow round-trip ok? True
Expected results
String parameters with single quotes and backslashes should be properly reproduced:
"cat's meow" would be escaped as "cat\\'s meow" and the resulting SQL would return cat's meow
"cat\\'s meow" would escape to "cat\\\\\\'s meow" and the SQL would return cat\'s meow
Suggested fix
I'm not sure how this is usually implemented, but in my example just doing param.replace('\\','\\\\' ).replace("'", "\\'") at least preserves single quotes and backslashes, which are probably the most common cases. It would also leave alone escaped unicode literals like \U0001F44D.
How I encountered it
I'm using dbt with Databricks and noticed on upgrading from dbt-databricks 1.0 to 1.2.2 that single quotes started disappearing from our "seeds" (csv files loaded as Delta tables). Code had changed in dbt-databricks to use the new parameter binding functionality in this library, whereas (I assume) before it must have been injecting the values as literals into the SQL. Activitysusodapop commented on Sep 26, 2022 Thanks for opening a detailed issue report! I've opened #46 which introduces unit tests for the expected escaping behaviour. Will need to compare your inputs against the expected behaviour to see if I can reproduce.
Just for clarity, you wrote:
The following demonstrates the issue in version 1.2.2 of databricks-sql-python
Do you mean version 1.2.2 of the dbt package? I don't believe we've released a version 1.2.2 of databricks-sql-python. mcannamela commented on Sep 26, 2022
Do you mean version 1.2.2 of the dbt package? I don't believe we've released a version 1.2.2 of databricks-sql-python.
@susodapop yes, my bad, it's version 2.0.5 of the sql connector. Just so there can be no mistakes:
root@4571a4538ab2:/dbt# pip freeze | grep databricks
databricks-sql-connector==2.0.5
dbt-databricks==1.2.2
susodapop commented on Oct 4, 2022 I agree there's a bug here. ParamEscaper should not try to escape a single quote ' with a second single quote ''. Per Chesterton's Fence, however, I wanted to suss out why the ParameterEscaper behaves the way it does. Here's what I found:
Background
The ANSI SQL spec says that a single quote ' within a string literal wrapped in single quotes ' may be escaped either by a back stroke \' or by an extra single quote ''. Databricks supports the first style of escape character but not the second. Instead of treating two single quotes '' as one, escaped single quote ', Databricks parses them as separate string literals and then concatenates them. In effect it appears to swallow both single-quotes '. Therefore we see the following behaviour:
| Input query |
ANSI SQL |
Databricks |
Same? |
SELECT 'foo''bar' |
foo'bar |
foobar |
🔴 |
SELECT 'cat''s meow' |
cat's meow |
cats meow |
🔴 |
SELECT 'cat\'s meow' |
cat's meow |
cat's meow |
🟢 |
SELECT "cat's meow" |
cat's meow |
cat's meow |
🟢 |
The author of ParamEscaper() assumed that because Databricks advertises ANSI compliance the customary two single quote '' escape pattern would work. But it does not.
To behave properly, ParameterEscaper() should escape single quotes ' with a back stroke \'.
Impact
When Cursor.execute(statement:str, params:List) is called today, databricks-sql-connector renders the statement with the provided params then sends it to Databricks. Before rendering, ParamEscaper() is called to sanitise params and thereby avoid SQL injection. This is necessary only because Databricks does not yet support parameterised SQL. The ParamEscaper() will be removed once Databricks adds support for parameterised SQL.
Solution
ParamEscaper() should be modified to escape one single quote ' with a back stroke \'.
mcannamela commented on Oct 6, 2022 Ahhh, well played!
ANSI compliance is "opt in" I believe. Some months ago, I thought it would become "opt out", but that appears not to have been the case and I can no longer find evidence of this.
However, it still doesn't appear that ANSI mode gives the escaping behavior you found documented, even though it does change other behaviors (note in the following examples, be sure to run the set statements in the same session):
--convince yourself you know how to set ansi mode
set ansi_mode = false;
select 1/0; -- null
set ansi_mode = true;
select 1/0; -- throws
--now check escaping behavior, and be surprised and dismayed that it doesn't change
set ansi_mode = false;
select 'cat''s meow'; -- cats meow
set ansi_mode = true;
select 'cat''s meow'; -- cats meow
So having done that exercise I doubly agree with your conclusion courtneyholcomb commented on Oct 6, 2022 Not sure if anyone was actively working on this, but I just put up a PR to fix it if it's helpful! #56 Reacted by mcannamela, Thomas Lento, Jesse and Nick Handel susodapop commented on Oct 6, 2022 Thanks @courtneyholcomb! We are working on a similar implementation internally but we hadn't incorporated an e2e test like you wrote. I'm going to keep your PR open so we can verify that our change will still pass your e2e test. If it doesn't, we can make sure that your use-case is still covered. I'll have more details within the next week!
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|