test: fix string compliance tests #23

HemangChothani · 2021-03-11T13:33:37Z

No description provided.

larkee

The literal string overrides LGTM, just some nits on the docstrings.

The cleanup override I'm not sure about as mentioned in #20. Let's resolve the discussion there and then apply any decision we make to this PR 👍

test/test_suite.py

…n-spanner-sqlalchemy into fix_string_compliance_tests

larkee · 2021-03-19T03:53:20Z

The test_literal_backslashes test is failing with:

sqlalchemy.exc.IntegrityError: (google.cloud.spanner_dbapi.exceptions.IntegrityError) 400 Duplicate name in schema: t_string.

test/test_suite.py

HemangChothani · 2021-03-22T14:12:07Z

The test_literal_backslashes test will pass after merging #34 PR

larkee · 2021-03-22T14:15:24Z

The test_literal_backslashes test will pass after merging #34 PR

The Github Action says it's already passing against the emulator 👍

larkee · 2021-03-23T07:05:41Z

test/test_suite.py

+
+        The original test string is : \"""some 'text' hey "hi there" that's text\"""
+
+        Spanner doesn't support string which contains `'s` in


This is incorrect. Spanner does support strings containing `'s': https://cloud.google.com/spanner/docs/lexical#literals

Using the original test string with the Python client:
Input: """some 'text' hey "hi there" that's text"""
Output: 'some \'text\' hey "hi there" that\'s text'

Yes, python successfully convert that string but spanner throws an error like 400 Syntax error: Expected \")\" or \",\" but got string literal \'text\' [at 1:41]\nINSERT INTO t_string (x) VALUES (\'some \'\'text\'\' hey \"hi there\" that\'\'s text\')\n.
I have tried with DBAPI with the direct string 'some \'text\' hey "hi there" that\'s text' and it throws the same error.

Spanner prod is working with these 3 strings and adding escape characters to them.

$ Report(name=r"1. backslash one \ backslash \\ two end").save()
$ Report.objects.all()[0].name
'1. backslash one \\ backslash \\\\ two end'

$ Report(name=r"2. backslash one \ backslash \ two end").save()
$ Report.objects.all()[1].name
'2. backslash one \ backslash \\ two end'

$ Report(name="""3. some 'text' hey "hi there" that's text""").save()
$ Report.objects.all()[2].name
'3. some 'text' hey "hi there" that's text'

I mean when it comes through the DB API it throws an error:

from google.cloud.spanner_dbapi import connect connection = connect('instance-id', 'databse-id') cursor = connection.cursor() cursor.execute("INSERT INTO t (x) VALUES ('backslash one \ backslash \\ two end')") #it will through an error `400 Syntax error: Illegal escape sequence` cursor.execute("""INSERT INTO t (x) VALUES (some 'text' hey "hi there" that's text)""") #it will through an error `400 Syntax error: Expected \")\" or \",\" but got string literal \'text\' `

@vi3k6i5 if you are using spanner client directly for the insert, then for help can you share the actual sql query syntax passing at that time.

Okay, it looks like the issue is the SQL query rather than the data string. Spanner supports both 'backslash one \ backslash \\ two end'' and "some 'text' hey "hi there" that's text' however, when specifying these types of strings by SQL, the additional escapes are necessary.

Rather than skipping these tests, we need to add additional logic to add these additional escapes before they are used to generate SQL statements.

Here is the DBAPI code showing the strings are supported:

connection = connect('test-instance', 'test-db') cursor = connection.cursor() cursor.execute( "INSERT INTO t (pkey, val) VALUES (0, \"{}\")".format( r'backslash one \\ backslash two \\\\ end')) cursor.execute( "INSERT INTO t (pkey, val) VALUES (1, \"{}\")".format( r"""some 'text' hey \"hi there\" that's text""")) cursor.execute("SELECT * FROM T") for row in cursor.fetchall(): print(str(row[1]))

Output:

some 'text' hey "hi there" that's text backslash one \ backslash two \\ end

google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py

…n-spanner-sqlalchemy into fix_string_compliance_tests

google-cla · 2021-04-08T05:20:00Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-04-14T15:58:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-04-15T06:08:26Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-04-23T09:50:11Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-04-23T09:57:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

…n-spanner-sqlalchemy into fix_string_compliance_tests

google-cla · 2021-04-28T15:55:55Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-04T08:10:23Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

…n-spanner-sqlalchemy into fix_string_compliance_tests

google-cla · 2021-05-04T08:17:22Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-04T12:44:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

HemangChothani · 2021-05-10T06:02:35Z

@larkee Can you change the cla for this PR? Don't know how to changed from yes to no.

…n-spanner-sqlalchemy into fix_string_compliance_tests

google-cla · 2021-05-11T04:53:35Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

larkee · 2021-05-11T05:19:05Z

test/test_suite.py

+
+
+class StringTest(_StringTest):
+    def test_literal_backslashes(self):


With the added render_literal_value method, test_literal_backslashes and test_literal_quoting should no longer need to be overridden. Please remove.

larkee · 2021-05-11T05:20:34Z

google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py

+        """
+        raw = ["\\", "'", '"', "\n", "\t", "\r"]
+        if type(value) == str and any(single in value for single in raw):
+            value = '"{}"'.format(value)


Locally, after removed the test_literal_backslashes and test_literal_quoting overrides, I was still getting failures.
The following change fixes the errors:

Suggested change

value = '"{}"'.format(value)

value = 'r"""{}"""'.format(value)

google-cla · 2021-05-11T05:40:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

test: fix string compliance tests

5b1d1b1

HemangChothani requested a review from larkee March 11, 2021 13:33

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2021

fix: resolve conflict

0730997

larkee reviewed Mar 16, 2021

View reviewed changes

test/test_suite.py Outdated Show resolved Hide resolved

test/test_suite.py Outdated Show resolved Hide resolved

test/test_suite.py Outdated Show resolved Hide resolved

test/test_suite.py Outdated Show resolved Hide resolved

larkee reviewed Mar 17, 2021

View reviewed changes

test/test_suite.py Outdated Show resolved Hide resolved

HemangChothani and others added 4 commits March 17, 2021 19:17

test: add todo note to remove override method and resolve conflict

1dba2c0

Merge branch 'main' of https://github.com/cloudspannerecosystem/pytho…

55eebbf

…n-spanner-sqlalchemy into fix_string_compliance_tests

test: nit

de0732d

Merge branch 'main' into fix_string_compliance_tests

89ab8d5

larkee reviewed Mar 19, 2021

View reviewed changes

test/test_suite.py Outdated Show resolved Hide resolved

HemangChothani added 2 commits March 22, 2021 18:49

test: remove select statements and resolve conflict

210f938

test: nit

9b52c20

larkee approved these changes Mar 22, 2021

View reviewed changes

larkee requested changes Mar 23, 2021

View reviewed changes

HemangChothani added 2 commits March 23, 2021 17:08

fix: nit

a08a154

fix: remove override methods and add default value

694ec7b

HemangChothani commented Mar 24, 2021

View reviewed changes

google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py Outdated Show resolved Hide resolved

fix: add default value MAX

703f58c

HemangChothani mentioned this pull request Mar 25, 2021

test: add text compliance tests #36

Merged

HemangChothani and others added 5 commits April 1, 2021 11:25

Merge branch 'main' of https://github.com/cloudspannerecosystem/pytho…

84782a0

…n-spanner-sqlalchemy into fix_string_compliance_tests

test: resolve conflict

0c53c30

Merge branch 'main' of https://github.com/cloudspannerecosystem/pytho…

175535c

…n-spanner-sqlalchemy into fix_string_compliance_tests

Merge branch 'main' into fix_string_compliance_tests

611605c

Merge branch 'main' into fix_string_compliance_tests

6025d3a

google-cla bot removed the cla: yes This human has signed the Contributor License Agreement. label Apr 8, 2021

test: resolve conflict

c4b9420

test: resolve conflict

81bb2eb

test: fix nit

01a85fe

Merge branch 'main' of https://github.com/cloudspannerecosystem/pytho…

6b0a402

…n-spanner-sqlalchemy into fix_string_compliance_tests

test: add logic for additional escape ti fox the test

69cfd6b

Merge branch 'main' of https://github.com/cloudspannerecosystem/pytho…

b58b718

…n-spanner-sqlalchemy into fix_string_compliance_tests

test: add condition check for other data type

bcf939c

larkee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 10, 2021

Merge branch 'main' of https://github.com/cloudspannerecosystem/pytho…

3229b2c

…n-spanner-sqlalchemy into fix_string_compliance_tests

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 11, 2021

larkee reviewed May 11, 2021

View reviewed changes

test: remove override tests

913710c

larkee approved these changes May 11, 2021

View reviewed changes

larkee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 11, 2021

larkee merged commit bcb442e into main May 11, 2021

IlyaFaer deleted the fix_string_compliance_tests branch May 11, 2021 08:16


		The original test string is : \"""some 'text' hey "hi there" that's text\"""

		Spanner doesn't support string which contains `'s` in



		class StringTest(_StringTest):
		def test_literal_backslashes(self):

	value = '"{}"'.format(value)
	value = 'r"""{}"""'.format(value)

test: fix string compliance tests #23

test: fix string compliance tests #23

Uh oh!

Conversation

HemangChothani commented Mar 11, 2021

Uh oh!

larkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larkee commented Mar 19, 2021

Uh oh!

Uh oh!

HemangChothani commented Mar 22, 2021

Uh oh!

larkee commented Mar 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vi3k6i5 Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

google-cla bot commented Apr 8, 2021

Uh oh!

google-cla bot commented Apr 14, 2021

Uh oh!

google-cla bot commented Apr 15, 2021

Uh oh!

google-cla bot commented Apr 23, 2021

Uh oh!

google-cla bot commented Apr 23, 2021

Uh oh!

google-cla bot commented Apr 28, 2021

Uh oh!

google-cla bot commented May 4, 2021

Uh oh!

google-cla bot commented May 4, 2021

Uh oh!

google-cla bot commented May 4, 2021

Uh oh!

HemangChothani commented May 10, 2021

Uh oh!

google-cla bot commented May 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented May 11, 2021

Uh oh!

Uh oh!

vi3k6i5 Apr 23, 2021 •

edited

Loading