refactor: improve error handling, parameter validation, and logging in Spanner DBAPI driver

bhatt4982 · bhatt4982 · commit 1db7aa35d346 · 2026-03-25T12:29:47.000Z
diff --git a/packages/google-cloud-spanner-dbapi-driver/google/cloud/spanner_driver/__init__.py b/packages/google-cloud-spanner-dbapi-driver/google/cloud/spanner_driver/__init__.py
@@ -52,32 +52,32 @@
 logger.addHandler(logging.NullHandler())
 
 __all__: list[str] = [
-    "apilevel",
-    "threadsafety",
-    "paramstyle",
+    "BINARY",
+    "Binary",
     "Connection",
-    "connect",
     "Cursor",
-    "Date",
-    "Time",
-    "Timestamp",
-    "DateFromTicks",
-    "TimeFromTicks",
-    "TimestampFromTicks",
-    "Binary",
-    "STRING",
-    "BINARY",
-    "NUMBER",
     "DATETIME",
-    "ROWID",
-    "InterfaceError",
-    "ProgrammingError",
-    "OperationalError",
-    "DatabaseError",
     "DataError",
-    "NotSupportedError",
+    "DatabaseError",
+    "Date",
+    "DateFromTicks",
+    "Error",
     "IntegrityError",
+    "InterfaceError",
     "InternalError",
+    "NUMBER",
+    "NotSupportedError",
+    "OperationalError",
+    "ProgrammingError",
+    "ROWID",
+    "STRING",
+    "Time",
+    "TimeFromTicks",
+    "Timestamp",
+    "TimestampFromTicks",
     "Warning",
-    "Error",
+    "apilevel",
+    "connect",
+    "paramstyle",
+    "threadsafety",
 ]
diff --git a/packages/google-cloud-spanner-dbapi-driver/google/cloud/spanner_driver/connection.py b/packages/google-cloud-spanner-dbapi-driver/google/cloud/spanner_driver/connection.py
@@ -89,8 +89,8 @@ def commit(self) -> None:
         try:
             self._internal_conn.commit()
         except Exception as e:
-            # raise errors.map_spanner_error(e)
             logger.debug(f"Commit failed {e}")
+            raise errors.map_spanner_error(e)
 
     @check_not_closed
     def rollback(self) -> None:
@@ -102,8 +102,8 @@ def rollback(self) -> None:
         try:
             self._internal_conn.rollback()
         except Exception as e:
-            # raise errors.map_spanner_error(e)
             logger.debug(f"Rollback failed {e}")
+            raise errors.map_spanner_error(e)
 
     def close(self) -> None:
         """Close the connection now.
@@ -127,7 +127,7 @@ def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None:
         self.close()
 
 
-def connect(connection_string: str, **kwargs: Any) -> Connection:
+def connect(connection_string: str) -> Connection:
     logger.debug(f"Connecting to {connection_string}")
     # Create the pool
     pool = Pool.create_pool(connection_string)
diff --git a/packages/google-cloud-spanner-dbapi-driver/google/cloud/spanner_driver/cursor.py b/packages/google-cloud-spanner-dbapi-driver/google/cloud/spanner_driver/cursor.py
@@ -116,7 +116,8 @@ def description(self) -> tuple[tuple[Any, ...], ...] | None:
                     )
                 )
             return tuple(desc)
-        except Exception:
+        except Exception as e:
+            logger.warning("Could not determine cursor description: %s", e)
             return None
 
     @property
@@ -165,8 +166,9 @@ def _prepare_params(
             # GoogleSQL Dialect: Named parameters @name are mapped directly.
             iterator = parameters.items()
         else:
-            # If strictly required, raise an error for unsupported types
-            return {}, {}
+            raise errors.ProgrammingError(
+                f"Parameters must be a dict, list, or tuple, not {type(parameters).__name__}"
+            )
 
         for key, value in iterator:
             if value is None:
@@ -429,7 +431,8 @@ def nextset(self) -> bool | None:
             if next_metadata:
                 return True
             return None
-        except Exception:
+        except Exception as e:
+            logger.warning("Could not determine next set of results: %s", e)
             return None
 
     def __enter__(self) -> "Cursor":
diff --git a/packages/google-cloud-spanner-dbapi-driver/tests/system/test_cursor.py b/packages/google-cloud-spanner-dbapi-driver/tests/system/test_cursor.py
@@ -141,4 +141,3 @@ def test_data_types(self):
                 assert row[2] is True
                 assert row[3] == "hello"
                 assert row[4] == b"bytes"
-                assert row[4] == b"bytes"
diff --git a/packages/google-cloud-spanner-dbapi-driver/tests/unit/test_connection.py b/packages/google-cloud-spanner-dbapi-driver/tests/unit/test_connection.py
@@ -70,23 +70,18 @@ def test_commit(self):
 
     def test_commit_error(self):
         self.mock_internal_conn.commit.side_effect = Exception("Commit Failed")
-        try:
+        with self.assertRaises(errors.DatabaseError):
             self.conn.commit()
-        except Exception:
-            self.fail("commit() raised Exception unexpectedly!")
         self.mock_internal_conn.commit.assert_called_once()
 
     def test_rollback(self):
         self.conn.rollback()
         self.mock_internal_conn.rollback.assert_called_once()
 
     def test_rollback_error(self):
-        # Similar to commit, rollback errors are caught and logged
         self.mock_internal_conn.rollback.side_effect = Exception("Rollback Failed")
-        try:
+        with self.assertRaises(errors.DatabaseError):
             self.conn.rollback()
-        except Exception:
-            self.fail("rollback() raised Exception unexpectedly!")
         self.mock_internal_conn.rollback.assert_called_once()
 
     def test_close(self):
diff --git a/packages/google-cloud-spanner-dbapi-driver/tests/unit/test_cursor.py b/packages/google-cloud-spanner-dbapi-driver/tests/unit/test_cursor.py
@@ -347,3 +347,7 @@ def test_prepare_params(self):
 
         self.assertEqual(converted["P2"], "test")
         self.assertEqual(types["P2"].code, TypeCode.STRING)
+
+    def test_prepare_params_unsupported_type(self):
+        with self.assertRaises(cursor.errors.ProgrammingError):
+            self.cursor._prepare_params(123)  # Int is not supported directly
