Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ def __init__(self, instance, database=None, read_only=False, **kwargs):
self._read_only = read_only
self._staleness = None
self.request_priority = None
self._timeout = None
self._transaction_begin_marked = False
self._transaction_isolation_level = None
# whether transaction started at Spanner. This means that we had
Expand Down Expand Up @@ -348,6 +349,30 @@ def staleness(self, value):

self._staleness = value

@property
def timeout(self):
"""Timeout in seconds for the next SQL operation on this connection.

When set, this value is passed as the ``timeout`` argument to
``execute_sql`` calls on the underlying Spanner client, controlling
the gRPC deadline for those calls.

Returns:
Optional[float]: The timeout in seconds, or None to use the
default gRPC timeout (3600s).
"""
return self._timeout

@timeout.setter
def timeout(self, value):
"""Set the timeout for subsequent SQL operations.

Args:
value (Optional[float]): Timeout in seconds. Set to None to
revert to the default gRPC timeout.
"""
self._timeout = value

def _session_checkout(self):
"""Get a Cloud Spanner session from the pool.

Expand Down Expand Up @@ -560,11 +585,16 @@ def run_statement(
checksum of this statement results.
"""
transaction = self.transaction_checkout()
kwargs = dict(
param_types=statement.param_types,
request_options=request_options or self.request_options,
)
if self._timeout is not None:
kwargs["timeout"] = self._timeout
Comment on lines +592 to +593
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using the public timeout property instead of the private _timeout attribute for consistency, even within the class methods.

Suggested change
if self._timeout is not None:
kwargs["timeout"] = self._timeout
if self.timeout is not None:
kwargs["timeout"] = self.timeout

return transaction.execute_sql(
statement.sql,
statement.params,
param_types=statement.param_types,
request_options=request_options or self.request_options,
**kwargs,
)

@check_not_closed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,17 @@ def _do_execute_update_in_autocommit(self, transaction, sql, params):
"""This function should only be used in autocommit mode."""
self.connection._transaction = transaction
self.connection._snapshot = None
self._result_set = transaction.execute_sql(
sql,
kwargs = dict(
params=params,
param_types=get_param_types(params),
last_statement=True,
)
if self.connection._timeout is not None:
kwargs["timeout"] = self.connection._timeout
Comment on lines +236 to +237
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It is better to use the public timeout property of the connection instead of accessing the private _timeout attribute directly. This ensures better encapsulation and consistency with how other connection properties (like autocommit or read_only) are accessed throughout the codebase.

Suggested change
if self.connection._timeout is not None:
kwargs["timeout"] = self.connection._timeout
if self.connection.timeout is not None:
kwargs["timeout"] = self.connection.timeout

self._result_set = transaction.execute_sql(
sql,
**kwargs,
)
self._itr = PeekIterator(self._result_set)
self._row_count = None

Expand Down Expand Up @@ -542,11 +547,16 @@ def _fetch(self, cursor_statement_type, size=None):
return rows

def _handle_DQL_with_snapshot(self, snapshot, sql, params):
kwargs = dict(
param_types=get_param_types(params),
request_options=self.request_options,
)
if self.connection._timeout is not None:
kwargs["timeout"] = self.connection._timeout
Comment on lines +554 to +555
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It is better to use the public timeout property of the connection instead of accessing the private _timeout attribute directly. This ensures better encapsulation and consistency with how other connection properties are accessed.

Suggested change
if self.connection._timeout is not None:
kwargs["timeout"] = self.connection._timeout
if self.connection.timeout is not None:
kwargs["timeout"] = self.connection.timeout

self._result_set = snapshot.execute_sql(
sql,
params,
get_param_types(params),
request_options=self.request_options,
**kwargs,
)
# Read the first element so that the StreamedResultSet can
# return the metadata after a DQL statement.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,56 @@ def test_request_priority(self):
sql, params, param_types=param_types, request_options=None
)

def test_timeout_default_none(self):
connection = self._make_connection()
self.assertIsNone(connection.timeout)

def test_timeout_property(self):
connection = self._make_connection()
connection.timeout = 60
self.assertEqual(connection.timeout, 60)

connection.timeout = None
self.assertIsNone(connection.timeout)

def test_timeout_passed_to_run_statement(self):
from google.cloud.spanner_dbapi.parsed_statement import Statement

sql = "SELECT 1"
params = []
param_types = {}

connection = self._make_connection()
connection._spanner_transaction_started = True
connection._transaction = mock.Mock()
connection._transaction.execute_sql = mock.Mock()

connection.timeout = 60

connection.run_statement(Statement(sql, params, param_types))

connection._transaction.execute_sql.assert_called_with(
sql, params, param_types=param_types, request_options=None, timeout=60
)

def test_timeout_not_passed_when_none(self):
from google.cloud.spanner_dbapi.parsed_statement import Statement

sql = "SELECT 1"
params = []
param_types = {}

connection = self._make_connection()
connection._spanner_transaction_started = True
connection._transaction = mock.Mock()
connection._transaction.execute_sql = mock.Mock()

connection.run_statement(Statement(sql, params, param_types))

connection._transaction.execute_sql.assert_called_with(
sql, params, param_types=param_types, request_options=None
)

def test_custom_client_connection(self):
from google.cloud.spanner_dbapi import connect

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,64 @@ def test_do_execute_update(self):
self.assertEqual(cursor._result_set, result_set)
self.assertEqual(cursor.rowcount, 1234)

def test_do_execute_update_with_timeout(self):
connection = self._make_connection(self.INSTANCE, self.DATABASE)
connection._timeout = 30
cursor = self._make_one(connection)
transaction = mock.MagicMock()

cursor._do_execute_update_in_autocommit(
transaction=transaction,
sql="UPDATE t SET x=1 WHERE true",
params={},
)

transaction.execute_sql.assert_called_once_with(
"UPDATE t SET x=1 WHERE true",
params={},
param_types={},
last_statement=True,
timeout=30,
)

def test_handle_DQL_with_snapshot_timeout(self):
connection = self._make_connection(self.INSTANCE, self.DATABASE)
connection._timeout = 45
cursor = self._make_one(connection)

snapshot = mock.MagicMock()
result_set = mock.MagicMock()
result_set.metadata.transaction.read_timestamp = None
snapshot.execute_sql.return_value = result_set

cursor._handle_DQL_with_snapshot(snapshot, "SELECT 1", None)

snapshot.execute_sql.assert_called_once_with(
"SELECT 1",
None,
param_types=None,
request_options=None,
timeout=45,
)

def test_handle_DQL_with_snapshot_no_timeout(self):
connection = self._make_connection(self.INSTANCE, self.DATABASE)
cursor = self._make_one(connection)

snapshot = mock.MagicMock()
result_set = mock.MagicMock()
result_set.metadata.transaction.read_timestamp = None
snapshot.execute_sql.return_value = result_set

cursor._handle_DQL_with_snapshot(snapshot, "SELECT 1", None)

snapshot.execute_sql.assert_called_once_with(
"SELECT 1",
None,
param_types=None,
request_options=None,
)

def test_do_batch_update(self):
from google.cloud.spanner_dbapi import connect
from google.cloud.spanner_v1.param_types import INT64
Expand Down Expand Up @@ -953,7 +1011,7 @@ def test_handle_dql_priority(self, MockedPeekIterator):
self.assertEqual(cursor._itr, MockedPeekIterator())
self.assertEqual(cursor._row_count, None)
mock_snapshot.execute_sql.assert_called_with(
sql, None, None, request_options=RequestOptions(priority=1)
sql, None, param_types=None, request_options=RequestOptions(priority=1)
)

def test_handle_dql_database_error(self):
Expand Down
Loading