From 7c66f8a85af0c91ae1599dafd91b7ca96e2c777d Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:55:27 +0530 Subject: [PATCH 1/3] FIX: accept timeout=0 in bulkcopy as no timeout the BCP API spec documents timeout 0 as no timeout, and mssql-py-core implements it that way (BulkCopyTimeoutState::from_seconds maps 0 to an infinite deadline). the python layer rejected it with a 'timeout must be positive' guard that has been in place since the feature shipped, so the documented value never worked. relaxes the bound to '< 0', matching what batch_size already does. bool is excluded explicitly since it is an int subclass and False would otherwise slip through as 0 and silently disable the timeout. negatives stay rejected, py-core takes an unsigned value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/cursor.py | 11 +++--- tests/test_019_bulkcopy.py | 72 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index dc661a73..b4598e5d 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -2893,7 +2893,8 @@ def bulkcopy( batch_size: Number of rows to send per batch. Default 0 uses server optimal. - timeout: Operation timeout in seconds. Default is 30. + timeout: Operation timeout in seconds. Default is 30. 0 disables the + bulk copy operation timeout. column_mappings: Maps source data columns to target table column names. Two formats supported: @@ -2975,10 +2976,10 @@ def bulkcopy( raise ValueError(f"batch_size must be non-negative, got {batch_size}") # Validate timeout type and value - if not isinstance(timeout, int): - raise TypeError(f"timeout must be a positive integer, got {type(timeout).__name__}") - if timeout <= 0: - raise ValueError(f"timeout must be positive, got {timeout}") + if not isinstance(timeout, int) or isinstance(timeout, bool): + raise TypeError(f"timeout must be a non-negative integer, got {type(timeout).__name__}") + if timeout < 0: + raise ValueError(f"timeout must be non-negative, got {timeout}") # Get and parse connection string if not hasattr(self.connection, "connection_str"): diff --git a/tests/test_019_bulkcopy.py b/tests/test_019_bulkcopy.py index 696e4901..7a3951d1 100644 --- a/tests/test_019_bulkcopy.py +++ b/tests/test_019_bulkcopy.py @@ -440,6 +440,78 @@ def test_bulkcopy_tuple_passthrough(cursor): cursor.connection.commit() +def test_bulkcopy_accepts_timeout_zero(cursor): + """GH-697: timeout=0 means no timeout per the BCP spec, so it must not be rejected.""" + table_name = "mssql_python_bcp_timeout_zero" + try: + cursor.execute(f"IF OBJECT_ID('{table_name}', 'U') IS NOT NULL DROP TABLE {table_name}") + cursor.execute(f"CREATE TABLE {table_name} (id INT, name VARCHAR(20))") + cursor.connection.commit() + + result = cursor.bulkcopy(table_name, [(1, "Alice"), (2, "Bob")], timeout=0) + assert result["rows_copied"] == 2 + + cursor.execute(f"SELECT COUNT(*) FROM {table_name}") + assert cursor.fetchone()[0] == 2 + + finally: + cursor.execute(f"IF OBJECT_ID('{table_name}', 'U') IS NOT NULL DROP TABLE {table_name}") + cursor.connection.commit() + + +def test_bulkcopy_forwards_timeout_zero_unchanged(): + """GH-697: 0 must reach py-core as 0, not be swapped for the 30s default. + + mssql-py-core maps a 0 timeout to an infinite deadline, so a copy that + merely succeeds does not prove the value survived the python layer. + """ + from unittest.mock import MagicMock, patch + + from mssql_python.cursor import Cursor + + mock_conn = MagicMock() + mock_conn.connection_str = "Server=localhost;Database=testdb;UID=sa;PWD=mypwd" + mock_conn._auth_type = None + mock_conn._is_connected = True + + cursor = Cursor.__new__(Cursor) + cursor._connection = mock_conn + cursor._timeout = 0 + cursor.closed = False + cursor.hstmt = None + + pycore_cursor = MagicMock() + pycore_cursor.bulkcopy.return_value = { + "rows_copied": 1, + "batch_count": 1, + "elapsed_time": 0.1, + } + pycore_conn = MagicMock() + pycore_conn.cursor.return_value = pycore_cursor + pycore_module = MagicMock() + pycore_module.PyCoreConnection = MagicMock(return_value=pycore_conn) + + with patch.dict("sys.modules", {"mssql_py_core": pycore_module}): + cursor.bulkcopy("dbo.some_table", [(1,)], timeout=0) + + assert pycore_cursor.bulkcopy.call_args.kwargs["timeout"] == 0 + + +def test_bulkcopy_rejects_invalid_timeouts(cursor): + """GH-697: negatives and bools stay rejected. Validation fires before any DB work.""" + with pytest.raises(ValueError, match="timeout must be non-negative"): + cursor.bulkcopy("mssql_python_bcp_unused", [(1,)], timeout=-1) + + with pytest.raises(TypeError, match="timeout must be a non-negative integer"): + cursor.bulkcopy("mssql_python_bcp_unused", [(1,)], timeout=1.5) + + # bool is an int subclass, so False would otherwise slip through as 0 + # and silently disable the timeout. + for flag in (False, True): + with pytest.raises(TypeError, match="timeout must be a non-negative integer"): + cursor.bulkcopy("mssql_python_bcp_unused", [(1,)], timeout=flag) + + def test_bulkcopy_empty_iterator(cursor): """Regression test: empty iterator doesn't crash (GH-482).""" table_name = "mssql_python_bcp_empty" From 49b3d60de968daa0f079d8232618af1f6c7ac2c8 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 10:58:42 +0530 Subject: [PATCH 2/3] CHORE: replace timeout=0 mock test with a real end-to-end one the mock only asserted that 0 reached py-core, which is plumbing rather than behaviour. a slow generator makes the real thing testable: the same source raises a timeout error at timeout=1 and copies all 30 rows at timeout=0, so the only variable is the timeout value. the source paces itself with sleeps, so it outlives the 1s timeout regardless of machine speed. verified all three timeout tests fail when the fix is reverted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_019_bulkcopy.py | 71 +++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/tests/test_019_bulkcopy.py b/tests/test_019_bulkcopy.py index e98fe62a..05fa7d5c 100644 --- a/tests/test_019_bulkcopy.py +++ b/tests/test_019_bulkcopy.py @@ -3,6 +3,8 @@ """Basic integration tests for bulkcopy via the mssql_python driver.""" +import time + import pytest # Skip the entire module when mssql_py_core can't be loaded (e.g. it's not @@ -459,43 +461,42 @@ def test_bulkcopy_accepts_timeout_zero(cursor): cursor.connection.commit() -def test_bulkcopy_forwards_timeout_zero_unchanged(): - """GH-697: 0 must reach py-core as 0, not be swapped for the 30s default. +def test_bulkcopy_timeout_zero_disables_the_timeout(cursor): + """GH-697: a copy that times out at timeout=1 completes at timeout=0. - mssql-py-core maps a 0 timeout to an infinite deadline, so a copy that - merely succeeds does not prove the value survived the python layer. + The source paces itself with sleeps so the copy always outlives a 1 second + timeout regardless of machine speed. Same source both times, so the only + variable is the timeout value. """ - from unittest.mock import MagicMock, patch - - from mssql_python.cursor import Cursor - - mock_conn = MagicMock() - mock_conn.connection_str = "Server=localhost;Database=testdb;UID=sa;PWD=mypwd" - mock_conn._auth_type = None - mock_conn._token_provider = None - mock_conn._is_connected = True - - cursor = Cursor.__new__(Cursor) - cursor._connection = mock_conn - cursor._timeout = 0 - cursor.closed = False - cursor.hstmt = None - - pycore_cursor = MagicMock() - pycore_cursor.bulkcopy.return_value = { - "rows_copied": 1, - "batch_count": 1, - "elapsed_time": 0.1, - } - pycore_conn = MagicMock() - pycore_conn.cursor.return_value = pycore_cursor - pycore_module = MagicMock() - pycore_module.PyCoreConnection = MagicMock(return_value=pycore_conn) - - with patch.dict("sys.modules", {"mssql_py_core": pycore_module}): - cursor.bulkcopy("dbo.some_table", [(1,)], timeout=0) - - assert pycore_cursor.bulkcopy.call_args.kwargs["timeout"] == 0 + table_name = "mssql_python_bcp_timeout_zero_behaviour" + + def slow_rows(): + for i in range(30): + time.sleep(0.1) # 3s total, comfortably past the 1s timeout + yield (i, f"row{i}") + + try: + cursor.execute(f"IF OBJECT_ID('{table_name}', 'U') IS NOT NULL DROP TABLE {table_name}") + cursor.execute(f"CREATE TABLE {table_name} (id INT, name NVARCHAR(50))") + cursor.connection.commit() + + # baseline: the timeout is real and fires on this source + with pytest.raises(Exception, match="(?i)timeout"): + cursor.bulkcopy(table_name, slow_rows(), timeout=1) + + cursor.execute(f"DELETE FROM {table_name}") + cursor.connection.commit() + + # same source, timeout disabled: runs to completion + result = cursor.bulkcopy(table_name, slow_rows(), timeout=0) + assert result["rows_copied"] == 30 + + cursor.execute(f"SELECT COUNT(*) FROM {table_name}") + assert cursor.fetchone()[0] == 30 + + finally: + cursor.execute(f"IF OBJECT_ID('{table_name}', 'U') IS NOT NULL DROP TABLE {table_name}") + cursor.connection.commit() def test_bulkcopy_rejects_invalid_timeouts(cursor): From 9401c6e363fec527715dce19f2bb88a50d90aa77 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 11:52:36 +0530 Subject: [PATCH 3/3] CHORE: replace arrow timeout=0 proxy test with a live copy the old test asserted that an unrelated downstream error message did not contain the word 'timeout', which is a weak proxy for 'validation passed' and breaks if that message ever changes. a live copy with timeout=0 asserts the behaviour directly, matching how the classic path is covered in test_019. verified it fails when the fix is reverted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_024_bulkcopy_arrow.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/test_024_bulkcopy_arrow.py b/tests/test_024_bulkcopy_arrow.py index b15d4b3e..a52ead3e 100644 --- a/tests/test_024_bulkcopy_arrow.py +++ b/tests/test_024_bulkcopy_arrow.py @@ -217,14 +217,6 @@ def test_timeout_bool_rejected(self): with pytest.raises(TypeError, match="timeout"): _bare_cursor().bulkcopy_arrow("t", pa.table({"a": [1]}), timeout=flag) - def test_timeout_zero_passes_validation(self): - # GH-697: 0 means no timeout, so it must clear validation. the bare - # cursor has no connection wiring, so it fails later for an unrelated - # reason. what matters is that it is not a timeout complaint. - with pytest.raises(Exception) as exc: - _bare_cursor().bulkcopy_arrow("t", pa.table({"a": [1]}), timeout=0) - assert "timeout" not in str(exc.value).lower() - def test_missing_pycore_raises_importerror(self): cur = _bare_cursor() with patch.dict("sys.modules", {"mssql_py_core": None}): @@ -499,6 +491,22 @@ def test_table_round_trip_with_nulls(self, cursor): assert rows[2][2] is None cursor.execute(f"DROP TABLE {t}") + def test_timeout_zero_copies(self, cursor): + """GH-697: timeout=0 means no timeout, so a copy with it must succeed.""" + t = "mssql_python_arrow_timeout_zero" + _make_table(cursor, t, "id INT NOT NULL, name NVARCHAR(50) NULL") + tbl = pa.table( + { + "id": pa.array([1, 2, 3], type=pa.int32()), + "name": pa.array(["a", "b", "c"]), + } + ) + result = cursor.bulkcopy_arrow(t, tbl, timeout=0) + assert result["rows_copied"] == 3 + cursor.execute(f"SELECT COUNT(*) FROM {t}") + assert cursor.fetchone()[0] == 3 + cursor.execute(f"DROP TABLE {t}") + def test_record_batch_source(self, cursor): t = "mssql_python_arrow_batch" _make_table(cursor, t, "id INT NOT NULL")