From 86ba731662acb14751ec0399cf7fa5e0a1a9f16c Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Fri, 27 Aug 2021 12:22:01 +0300 Subject: [PATCH 1/4] fix(db_api): move connection validation into a separate method --- google/cloud/spanner_dbapi/connection.py | 32 ++++++++------ tests/unit/spanner_dbapi/test_connect.py | 25 ----------- tests/unit/spanner_dbapi/test_connection.py | 43 ++++++++++++++++++ tests/unit/spanner_dbapi/test_cursor.py | 48 +++------------------ 4 files changed, 69 insertions(+), 79 deletions(-) diff --git a/google/cloud/spanner_dbapi/connection.py b/google/cloud/spanner_dbapi/connection.py index 110e0f9b9b..2335db5d9f 100644 --- a/google/cloud/spanner_dbapi/connection.py +++ b/google/cloud/spanner_dbapi/connection.py @@ -28,7 +28,7 @@ from google.cloud.spanner_dbapi.checksum import _compare_checksums from google.cloud.spanner_dbapi.checksum import ResultsChecksum from google.cloud.spanner_dbapi.cursor import Cursor -from google.cloud.spanner_dbapi.exceptions import InterfaceError +from google.cloud.spanner_dbapi.exceptions import InterfaceError, OperationalError from google.cloud.spanner_dbapi.version import DEFAULT_USER_AGENT from google.cloud.spanner_dbapi.version import PY_VERSION @@ -349,6 +349,24 @@ def run_statement(self, statement, retried=False): ResultsChecksum() if retried else statement.checksum, ) + def validate(self): + """ + Execute a minimal request to check if the connection + is valid and the related database is reachable. + + Raise an exception in case if the connection is closed, + invalid, target database is not found, or the request result + is incorrect. + + :raises: :class:`InterfaceError`: if this connection is closed. + :raises: :class:`OperationalError`: if the request result is incorrect. + """ + self._raise_if_closed() + + with self.database.snapshot() as snapshot: + if [[1]] != list(snapshot.execute_sql("SELECT 1")): + raise OperationalError("The connection is invalid") + def __enter__(self): return self @@ -399,9 +417,6 @@ def connect( :rtype: :class:`google.cloud.spanner_dbapi.connection.Connection` :returns: Connection object associated with the given Google Cloud Spanner resource. - - :raises: :class:`ValueError` in case of given instance/database - doesn't exist. """ client_info = ClientInfo( @@ -418,14 +433,7 @@ def connect( ) instance = client.instance(instance_id) - if not instance.exists(): - raise ValueError("instance '%s' does not exist." % instance_id) - - database = instance.database(database_id, pool=pool) - if not database.exists(): - raise ValueError("database '%s' does not exist." % database_id) - - conn = Connection(instance, database) + conn = Connection(instance, instance.database(database_id, pool=pool)) if pool is not None: conn._own_pool = False diff --git a/tests/unit/spanner_dbapi/test_connect.py b/tests/unit/spanner_dbapi/test_connect.py index 96dcb20e01..f4dfe28a96 100644 --- a/tests/unit/spanner_dbapi/test_connect.py +++ b/tests/unit/spanner_dbapi/test_connect.py @@ -88,31 +88,6 @@ def test_w_explicit(self, mock_client): self.assertIs(connection.database, database) instance.database.assert_called_once_with(DATABASE, pool=pool) - def test_w_instance_not_found(self, mock_client): - from google.cloud.spanner_dbapi import connect - - client = mock_client.return_value - instance = client.instance.return_value - instance.exists.return_value = False - - with self.assertRaises(ValueError): - connect(INSTANCE, DATABASE) - - instance.exists.assert_called_once_with() - - def test_w_database_not_found(self, mock_client): - from google.cloud.spanner_dbapi import connect - - client = mock_client.return_value - instance = client.instance.return_value - database = instance.database.return_value - database.exists.return_value = False - - with self.assertRaises(ValueError): - connect(INSTANCE, DATABASE) - - database.exists.assert_called_once_with() - def test_w_credential_file_path(self, mock_client): from google.cloud.spanner_dbapi import connect from google.cloud.spanner_dbapi import Connection diff --git a/tests/unit/spanner_dbapi/test_connection.py b/tests/unit/spanner_dbapi/test_connection.py index 48129dcc2f..a9f95bf3ec 100644 --- a/tests/unit/spanner_dbapi/test_connection.py +++ b/tests/unit/spanner_dbapi/test_connection.py @@ -624,3 +624,46 @@ def test_retry_transaction_w_empty_response(self): compare_mock.assert_called_with(checksum, retried_checkum) run_mock.assert_called_with(statement, retried=True) + + def test_validate_ok(self): + def exit_func(self, exc_type, exc_value, traceback): + if exc_value: + raise exc_value + + connection = self._make_connection() + + # mock snapshot context manager + snapshot_obj = mock.Mock() + snapshot_obj.execute_sql = mock.Mock(return_value=[[1]]) + + snapshot_ctx = mock.Mock() + snapshot_ctx.__enter__ = mock.Mock(return_value=snapshot_obj) + snapshot_ctx.__exit__ = exit_func + snapshot_method = mock.Mock(return_value=snapshot_ctx) + + connection.database.snapshot = snapshot_method + + connection.validate() + + def test_validate_fail(self): + from google.cloud.spanner_dbapi.exceptions import OperationalError + + def exit_func(self, exc_type, exc_value, traceback): + if exc_value: + raise exc_value + + connection = self._make_connection() + + # mock snapshot context manager + snapshot_obj = mock.Mock() + snapshot_obj.execute_sql = mock.Mock(return_value=[[3]]) + + snapshot_ctx = mock.Mock() + snapshot_ctx.__enter__ = mock.Mock(return_value=snapshot_obj) + snapshot_ctx.__exit__ = exit_func + snapshot_method = mock.Mock(return_value=snapshot_ctx) + + connection.database.snapshot = snapshot_method + + with self.assertRaises(OperationalError): + connection.validate() diff --git a/tests/unit/spanner_dbapi/test_cursor.py b/tests/unit/spanner_dbapi/test_cursor.py index d7c181ff0b..07deffd707 100644 --- a/tests/unit/spanner_dbapi/test_cursor.py +++ b/tests/unit/spanner_dbapi/test_cursor.py @@ -332,13 +332,7 @@ def test_executemany_delete_batch_autocommit(self): sql = "DELETE FROM table WHERE col1 = %s" - with mock.patch( - "google.cloud.spanner_v1.instance.Instance.exists", return_value=True - ): - with mock.patch( - "google.cloud.spanner_v1.database.Database.exists", return_value=True, - ): - connection = connect("test-instance", "test-database") + connection = connect("test-instance", "test-database") connection.autocommit = True transaction = self._transaction_mock() @@ -369,13 +363,7 @@ def test_executemany_update_batch_autocommit(self): sql = "UPDATE table SET col1 = %s WHERE col2 = %s" - with mock.patch( - "google.cloud.spanner_v1.instance.Instance.exists", return_value=True - ): - with mock.patch( - "google.cloud.spanner_v1.database.Database.exists", return_value=True, - ): - connection = connect("test-instance", "test-database") + connection = connect("test-instance", "test-database") connection.autocommit = True transaction = self._transaction_mock() @@ -418,13 +406,7 @@ def test_executemany_insert_batch_non_autocommit(self): sql = """INSERT INTO table (col1, "col2", `col3`, `"col4"`) VALUES (%s, %s, %s, %s)""" - with mock.patch( - "google.cloud.spanner_v1.instance.Instance.exists", return_value=True - ): - with mock.patch( - "google.cloud.spanner_v1.database.Database.exists", return_value=True, - ): - connection = connect("test-instance", "test-database") + connection = connect("test-instance", "test-database") transaction = self._transaction_mock() @@ -461,13 +443,7 @@ def test_executemany_insert_batch_autocommit(self): sql = """INSERT INTO table (col1, "col2", `col3`, `"col4"`) VALUES (%s, %s, %s, %s)""" - with mock.patch( - "google.cloud.spanner_v1.instance.Instance.exists", return_value=True - ): - with mock.patch( - "google.cloud.spanner_v1.database.Database.exists", return_value=True, - ): - connection = connect("test-instance", "test-database") + connection = connect("test-instance", "test-database") connection.autocommit = True @@ -510,13 +486,7 @@ def test_executemany_insert_batch_failed(self): sql = """INSERT INTO table (col1, "col2", `col3`, `"col4"`) VALUES (%s, %s, %s, %s)""" err_details = "Details here" - with mock.patch( - "google.cloud.spanner_v1.instance.Instance.exists", return_value=True - ): - with mock.patch( - "google.cloud.spanner_v1.database.Database.exists", return_value=True, - ): - connection = connect("test-instance", "test-database") + connection = connect("test-instance", "test-database") connection.autocommit = True cursor = connection.cursor() @@ -546,13 +516,7 @@ def test_executemany_insert_batch_aborted(self): sql = """INSERT INTO table (col1, "col2", `col3`, `"col4"`) VALUES (%s, %s, %s, %s)""" err_details = "Aborted details here" - with mock.patch( - "google.cloud.spanner_v1.instance.Instance.exists", return_value=True - ): - with mock.patch( - "google.cloud.spanner_v1.database.Database.exists", return_value=True, - ): - connection = connect("test-instance", "test-database") + connection = connect("test-instance", "test-database") transaction1 = mock.Mock(committed=False, rolled_back=False) transaction1.batch_update = mock.Mock( From c1ee8ebb1b137116fa039892aa2f6ace6d0c494b Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Mon, 30 Aug 2021 11:21:35 +0300 Subject: [PATCH 2/4] add more tests --- google/cloud/spanner_dbapi/connection.py | 8 ++++++-- tests/system/test_dbapi.py | 7 +++++++ tests/unit/spanner_dbapi/test_connection.py | 12 ++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/google/cloud/spanner_dbapi/connection.py b/google/cloud/spanner_dbapi/connection.py index 2335db5d9f..808e80a73c 100644 --- a/google/cloud/spanner_dbapi/connection.py +++ b/google/cloud/spanner_dbapi/connection.py @@ -364,8 +364,12 @@ def validate(self): self._raise_if_closed() with self.database.snapshot() as snapshot: - if [[1]] != list(snapshot.execute_sql("SELECT 1")): - raise OperationalError("The connection is invalid") + result = list(snapshot.execute_sql("SELECT 1")) + if result != [[1]]: + raise OperationalError( + "The checking query (SELECT 1) returned an unexpected result: %s. " + "Expected: [[1]]" % result + ) def __enter__(self): return self diff --git a/tests/system/test_dbapi.py b/tests/system/test_dbapi.py index 5cc7df677a..210a4f5e90 100644 --- a/tests/system/test_dbapi.py +++ b/tests/system/test_dbapi.py @@ -350,3 +350,10 @@ def test_DDL_commit(shared_instance, dbapi_database): cur.execute("DROP TABLE Singers") conn.commit() + + +def test_ping(shared_instance, dbapi_database): + """Check connection validation method.""" + conn = Connection(shared_instance, dbapi_database) + conn.validate() + conn.close() diff --git a/tests/unit/spanner_dbapi/test_connection.py b/tests/unit/spanner_dbapi/test_connection.py index a9f95bf3ec..a20b2ca8d8 100644 --- a/tests/unit/spanner_dbapi/test_connection.py +++ b/tests/unit/spanner_dbapi/test_connection.py @@ -644,6 +644,7 @@ def exit_func(self, exc_type, exc_value, traceback): connection.database.snapshot = snapshot_method connection.validate() + snapshot_obj.execute_sql.assert_called_once_with("SELECT 1") def test_validate_fail(self): from google.cloud.spanner_dbapi.exceptions import OperationalError @@ -667,3 +668,14 @@ def exit_func(self, exc_type, exc_value, traceback): with self.assertRaises(OperationalError): connection.validate() + + snapshot_obj.execute_sql.assert_called_once_with("SELECT 1") + + def test_validate_closed(self): + from google.cloud.spanner_dbapi.exceptions import InterfaceError + + connection = self._make_connection() + connection.close() + + with self.assertRaises(InterfaceError): + connection.validate() From 541c30b93cb9f70a0f89122f68d8c9a726e66b9d Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Tue, 31 Aug 2021 12:27:46 +0300 Subject: [PATCH 3/4] add test for an exception --- tests/unit/spanner_dbapi/test_connection.py | 30 ++++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/unit/spanner_dbapi/test_connection.py b/tests/unit/spanner_dbapi/test_connection.py index a20b2ca8d8..abdd3357dd 100644 --- a/tests/unit/spanner_dbapi/test_connection.py +++ b/tests/unit/spanner_dbapi/test_connection.py @@ -627,8 +627,7 @@ def test_retry_transaction_w_empty_response(self): def test_validate_ok(self): def exit_func(self, exc_type, exc_value, traceback): - if exc_value: - raise exc_value + pass connection = self._make_connection() @@ -650,8 +649,7 @@ def test_validate_fail(self): from google.cloud.spanner_dbapi.exceptions import OperationalError def exit_func(self, exc_type, exc_value, traceback): - if exc_value: - raise exc_value + pass connection = self._make_connection() @@ -671,6 +669,30 @@ def exit_func(self, exc_type, exc_value, traceback): snapshot_obj.execute_sql.assert_called_once_with("SELECT 1") + def test_validate_error(self): + from google.cloud.exceptions import NotFound + + def exit_func(self, exc_type, exc_value, traceback): + pass + + connection = self._make_connection() + + # mock snapshot context manager + snapshot_obj = mock.Mock() + snapshot_obj.execute_sql = mock.Mock(side_effect=NotFound("Not found")) + + snapshot_ctx = mock.Mock() + snapshot_ctx.__enter__ = mock.Mock(return_value=snapshot_obj) + snapshot_ctx.__exit__ = exit_func + snapshot_method = mock.Mock(return_value=snapshot_ctx) + + connection.database.snapshot = snapshot_method + + with self.assertRaises(NotFound): + connection.validate() + + snapshot_obj.execute_sql.assert_called_once_with("SELECT 1") + def test_validate_closed(self): from google.cloud.spanner_dbapi.exceptions import InterfaceError From 69057795585889fc2c2865ba5d6c55fae312594d Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Thu, 2 Sep 2021 11:25:48 +0300 Subject: [PATCH 4/4] add exception docstring --- google/cloud/spanner_dbapi/connection.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/google/cloud/spanner_dbapi/connection.py b/google/cloud/spanner_dbapi/connection.py index 808e80a73c..8d46b84cef 100644 --- a/google/cloud/spanner_dbapi/connection.py +++ b/google/cloud/spanner_dbapi/connection.py @@ -360,6 +360,8 @@ def validate(self): :raises: :class:`InterfaceError`: if this connection is closed. :raises: :class:`OperationalError`: if the request result is incorrect. + :raises: :class:`google.cloud.exceptions.NotFound`: if the linked instance + or database doesn't exist. """ self._raise_if_closed()