single quote not properly escaped about databricks-sql-python

[courtneyholcomb]

Not sure if anyone was actively working on this, but I just put up a PR to fix it if it's helpful! #56

[susodapop]

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]

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]

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]

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

[susodapop]

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!