From f4b82cfb441304c4999154fc851d4bfb8d420ad2 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Mon, 15 Mar 2021 12:38:38 +0300 Subject: [PATCH 1/5] feat: implement UNIQUE constraints creation mechanism --- .../sqlalchemy_spanner/sqlalchemy_spanner.py | 12 ++ test/test_suite.py | 151 +++++++++++++++++- 2 files changed, 161 insertions(+), 2 deletions(-) diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 4ecf6e9a..b02bd5b5 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -96,6 +96,18 @@ def visit_primary_key_constraint(self, constraint): """ return None + def visit_unique_constraint(self, constraint): + """Unique contraints in Spanner are defined with indexes: + https://cloud.google.com/spanner/docs/secondary-indexes#unique-indexes + + The method throws an exception to notify user that in + Spanner unique constraints are done with unique indexes. + """ + raise spanner_dbapi.exceptions.ProgrammingError( + "Spanner doesn't support direct UNIQUE constraints creation. " + "Create UNIQUE indexes instead." + ) + def post_create_table(self, table): """Build statements to be executed after CREATE TABLE. diff --git a/test/test_suite.py b/test/test_suite.py index 4f565c65..54621413 100644 --- a/test/test_suite.py +++ b/test/test_suite.py @@ -14,21 +14,33 @@ # See the License for the specific language governing permissions and # limitations under the License. +import operator import pytest +import sqlalchemy +from sqlalchemy import inspect +from sqlalchemy import testing +from sqlalchemy import MetaData +from sqlalchemy.schema import DDL from sqlalchemy.testing import config from sqlalchemy.testing import eq_ from sqlalchemy.testing import provide_metadata +from sqlalchemy.testing.provision import temp_table_keyword_args from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table from sqlalchemy import literal_column +from sqlalchemy import event from sqlalchemy import exists from sqlalchemy import select from sqlalchemy import Boolean from sqlalchemy import String -from sqlalchemy.testing.suite.test_ddl import * # noqa: F401, F403 +from google.cloud import spanner_dbapi + +from sqlalchemy.testing.suite.test_reflection import * # noqa: F401, F403 + from sqlalchemy.testing.suite.test_cte import * # noqa: F401, F403 +from sqlalchemy.testing.suite.test_ddl import * # noqa: F401, F403 from sqlalchemy.testing.suite.test_dialect import * # noqa: F401, F403 from sqlalchemy.testing.suite.test_update_delete import * # noqa: F401, F403 @@ -37,8 +49,10 @@ from sqlalchemy.testing.suite.test_ddl import ( LongNameBlowoutTest as _LongNameBlowoutTest, ) - from sqlalchemy.testing.suite.test_dialect import EscapingTest as _EscapingTest +from sqlalchemy.testing.suite.test_reflection import ( + ComponentReflectionTest as _ComponentReflectionTest, +) from sqlalchemy.testing.suite.test_select import ExistsTest as _ExistsTest from sqlalchemy.testing.suite.test_types import BooleanTest as _BooleanTest @@ -208,3 +222,136 @@ def test_underscore_names(self): @pytest.mark.skip("Max identifier length in Spanner is 128") class LongNameBlowoutTest(_LongNameBlowoutTest): pass + + +class ComponentReflectionTest(_ComponentReflectionTest): + @classmethod + def define_temp_tables(cls, metadata): + """ + SPANNER OVERRIDE: + + In Cloud Spanner unique indexes are used instead of directly + creating unique constraints. Overriding the test to replace + constraints with indexes in testing data. + """ + kw = temp_table_keyword_args(config, config.db) + user_tmp = Table( + "user_tmp", + metadata, + Column("id", sqlalchemy.INT, primary_key=True), + Column("name", sqlalchemy.VARCHAR(50)), + Column("foo", sqlalchemy.INT), + sqlalchemy.Index("user_tmp_uq", "name", unique=True), + sqlalchemy.Index("user_tmp_ix", "foo"), + **kw + ) + if ( + testing.requires.view_reflection.enabled + and testing.requires.temporary_views.enabled + ): + event.listen( + user_tmp, + "after_create", + DDL("create temporary view user_tmp_v as " "select * from user_tmp"), + ) + event.listen(user_tmp, "before_drop", DDL("drop view user_tmp_v")) + + @testing.provide_metadata + def _test_get_unique_constraints(self, schema=None): + """ + SPANNER OVERRIDE: + + In Cloud Spanner unique indexes are used instead of directly + creating unique constraints. Overriding the test to replace + constraints with indexes in testing data. + """ + # SQLite dialect needs to parse the names of the constraints + # separately from what it gets from PRAGMA index_list(), and + # then matches them up. so same set of column_names in two + # constraints will confuse it. Perhaps we should no longer + # bother with index_list() here since we have the whole + # CREATE TABLE? + uniques = sorted( + [ + {"name": "unique_a", "column_names": ["a"]}, + {"name": "unique_a_b_c", "column_names": ["a", "b", "c"]}, + {"name": "unique_c_a_b", "column_names": ["c", "a", "b"]}, + {"name": "unique_asc_key", "column_names": ["asc", "key"]}, + {"name": "i.have.dots", "column_names": ["b"]}, + {"name": "i have spaces", "column_names": ["c"]}, + ], + key=operator.itemgetter("name"), + ) + orig_meta = self.metadata + table = Table( + "testtbl", + orig_meta, + Column("a", sqlalchemy.String(20)), + Column("b", sqlalchemy.String(30)), + Column("c", sqlalchemy.Integer), + # reserved identifiers + Column("asc", sqlalchemy.String(30)), + Column("key", sqlalchemy.String(30)), + schema=schema, + ) + for uc in uniques: + table.append_constraint( + sqlalchemy.Index(uc["name"], *uc["column_names"], unique=True) + ) + orig_meta.create_all() + + inspector = inspect(orig_meta.bind) + reflected = sorted( + inspector.get_unique_constraints("testtbl", schema=schema), + key=operator.itemgetter("name"), + ) + + names_that_duplicate_index = set() + + for orig, refl in zip(uniques, reflected): + # Different dialects handle duplicate index and constraints + # differently, so ignore this flag + dupe = refl.pop("duplicates_index", None) + if dupe: + names_that_duplicate_index.add(dupe) + eq_(orig, refl) + + reflected_metadata = MetaData() + reflected = Table( + "testtbl", reflected_metadata, autoload_with=orig_meta.bind, schema=schema, + ) + + # test "deduplicates for index" logic. MySQL and Oracle + # "unique constraints" are actually unique indexes (with possible + # exception of a unique that is a dupe of another one in the case + # of Oracle). make sure # they aren't duplicated. + idx_names = set([idx.name for idx in reflected.indexes]) + uq_names = set( + [ + uq.name + for uq in reflected.constraints + if isinstance(uq, sqlalchemy.UniqueConstraint) + ] + ).difference(["unique_c_a_b"]) + + assert not idx_names.intersection(uq_names) + if names_that_duplicate_index: + eq_(names_that_duplicate_index, idx_names) + eq_(uq_names, set()) + + @testing.provide_metadata + def test_unique_constraint_raises(self): + """ + Checking that unique constraint creation + fails due to a ProgrammingError. + """ + Table( + "user_tmp_failure", + self.metadata, + Column("id", sqlalchemy.INT, primary_key=True), + Column("name", sqlalchemy.VARCHAR(50)), + sqlalchemy.UniqueConstraint("name", name="user_tmp_uq"), + ) + + with pytest.raises(spanner_dbapi.exceptions.ProgrammingError): + self.metadata.create_all() From d2534e0b7de59e98a2917b9fbcf2f7d227cc761a Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Tue, 16 Mar 2021 10:27:24 +0300 Subject: [PATCH 2/5] don't run all the reflection tests yet --- test/test_suite.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/test_suite.py b/test/test_suite.py index 54621413..2e2c2e5e 100644 --- a/test/test_suite.py +++ b/test/test_suite.py @@ -37,8 +37,6 @@ from google.cloud import spanner_dbapi -from sqlalchemy.testing.suite.test_reflection import * # noqa: F401, F403 - from sqlalchemy.testing.suite.test_cte import * # noqa: F401, F403 from sqlalchemy.testing.suite.test_ddl import * # noqa: F401, F403 from sqlalchemy.testing.suite.test_dialect import * # noqa: F401, F403 From c876f033a68085e2400aba2374691d451d704cb7 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Fri, 19 Mar 2021 12:11:45 +0300 Subject: [PATCH 3/5] EOF --- test/test_suite.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_suite.py b/test/test_suite.py index 2d73c0c8..2bce798f 100644 --- a/test/test_suite.py +++ b/test/test_suite.py @@ -657,4 +657,4 @@ def test_unique_constraint_raises(self): ) with pytest.raises(spanner_dbapi.exceptions.ProgrammingError): - self.metadata.create_all() \ No newline at end of file + self.metadata.create_all() From 456cd11e5d72f17a93d057112455897e88383f2c Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Fri, 19 Mar 2021 12:12:16 +0300 Subject: [PATCH 4/5] override the test data definitions --- test/test_suite.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/test_suite.py b/test/test_suite.py index 2bce798f..17c57bae 100644 --- a/test/test_suite.py +++ b/test/test_suite.py @@ -20,6 +20,7 @@ import sqlalchemy from sqlalchemy import inspect from sqlalchemy import testing +from sqlalchemy import ForeignKey from sqlalchemy import MetaData from sqlalchemy.schema import DDL from sqlalchemy.testing import config @@ -118,6 +119,29 @@ def test_percent_sign_round_trip(self): class CTETest(_CTETest): + @classmethod + def define_tables(cls, metadata): + """ + The original method creates a foreign key without a name, + which causes troubles on test cleanup. Overriding the + method to explicitly set a foreign key name. + """ + Table( + "some_table", + metadata, + Column("id", Integer, primary_key=True), + Column("data", String(50)), + Column("parent_id", ForeignKey("some_table.id", name="fk_some_table")), + ) + + Table( + "some_other_table", + metadata, + Column("id", Integer, primary_key=True), + Column("data", String(50)), + Column("parent_id", Integer), + ) + @pytest.mark.skip("INSERT from WITH subquery is not supported") def test_insert_from_select_round_trip(self): """ From d7e08df1926efb8e1d1ace567ab94dc61e61dc8a Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Fri, 19 Mar 2021 13:29:49 +0300 Subject: [PATCH 5/5] fix: use empty schema name by default --- .../sqlalchemy_spanner/sqlalchemy_spanner.py | 2 +- test/test_suite.py | 45 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py index 5e93d817..b672f46d 100644 --- a/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py +++ b/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py @@ -527,7 +527,7 @@ def get_table_names(self, connection, schema=None, **kw): FROM information_schema.tables WHERE table_schema = '{}' """.format( - schema + schema or "" ) table_names = [] diff --git a/test/test_suite.py b/test/test_suite.py index 17c57bae..36ef6a4d 100644 --- a/test/test_suite.py +++ b/test/test_suite.py @@ -682,3 +682,48 @@ def test_unique_constraint_raises(self): with pytest.raises(spanner_dbapi.exceptions.ProgrammingError): self.metadata.create_all() + + @testing.provide_metadata + def _test_get_table_names(self, schema=None, table_type="table", order_by=None): + """ + SPANNER OVERRIDE: + + Spanner doesn't support temporary tables, so real tables are + used for testing. As the original test expects only real + tables to be read, and in Spanner all the tables are real, + expected results override is required. + """ + _ignore_tables = [ + "comment_test", + "noncol_idx_test_pk", + "noncol_idx_test_nopk", + "local_table", + "remote_table", + "remote_table_2", + ] + meta = self.metadata + + insp = inspect(meta.bind) + + if table_type == "view": + table_names = insp.get_view_names(schema) + table_names.sort() + answer = ["email_addresses_v", "users_v"] + eq_(sorted(table_names), answer) + else: + if order_by: + tables = [ + rec[0] + for rec in insp.get_sorted_table_and_fkc_names(schema) + if rec[0] + ] + else: + tables = insp.get_table_names(schema) + table_names = [t for t in tables if t not in _ignore_tables] + + if order_by == "foreign_key": + answer = ["users", "user_tmp", "email_addresses", "dingalings"] + eq_(table_names, answer) + else: + answer = ["dingalings", "email_addresses", "user_tmp", "users"] + eq_(sorted(table_names), answer)