From a3bda6949aef498e05292dba818dc50a1f994597 Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 29 Jul 2025 17:13:39 +0530 Subject: [PATCH 1/6] FIX: Setting autocommit as False by default --- mssql_python/connection.py | 2 +- mssql_python/db_connection.py | 2 +- tests/test_003_connection.py | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/mssql_python/connection.py b/mssql_python/connection.py index d1ed6e78..71e14a48 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -147,7 +147,7 @@ def autocommit(self, value: bool) -> None: self.setautocommit(value) log('info', "Autocommit mode set to %s.", value) - def setautocommit(self, value: bool = True) -> None: + def setautocommit(self, value: bool = False) -> None: """ Set the autocommit mode of the connection. Args: diff --git a/mssql_python/db_connection.py b/mssql_python/db_connection.py index 9c688ac6..5e98056e 100644 --- a/mssql_python/db_connection.py +++ b/mssql_python/db_connection.py @@ -5,7 +5,7 @@ """ from mssql_python.connection import Connection -def connect(connection_str: str = "", autocommit: bool = True, attrs_before: dict = None, **kwargs) -> Connection: +def connect(connection_str: str = "", autocommit: bool = False, attrs_before: dict = None, **kwargs) -> Connection: """ Constructor for creating a connection to the database. diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index bef23815..0e98cb90 100644 --- a/tests/test_003_connection.py +++ b/tests/test_003_connection.py @@ -14,6 +14,7 @@ import pytest import time from mssql_python import Connection, connect, pooling +#from pyodbc import connect, Connection, pooling def drop_table_if_exists(cursor, table_name): """Drop the table if it exists""" @@ -73,7 +74,7 @@ def test_connection_string_with_odbc_param(db_connection): assert "Driver={ODBC Driver 18 for SQL Server};;APP=MSSQL-Python;Server=localhost;Uid=me;Pwd=mypwd;Database=mydb;Encrypt=yes;TrustServerCertificate=yes;" == conn_str, "Connection string is incorrect" def test_autocommit_default(db_connection): - assert db_connection.autocommit is True, "Autocommit should be True by default" + assert db_connection.autocommit is False, "Autocommit should be False by default" def test_autocommit_setter(db_connection): db_connection.autocommit = True From e4a6d76deae782892b86c46d42e7ec07b15d8248 Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 29 Jul 2025 17:16:25 +0530 Subject: [PATCH 2/6] FIX: Setting autocommit as False by default --- tests/test_003_connection.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index 0e98cb90..f82b32ed 100644 --- a/tests/test_003_connection.py +++ b/tests/test_003_connection.py @@ -14,7 +14,6 @@ import pytest import time from mssql_python import Connection, connect, pooling -#from pyodbc import connect, Connection, pooling def drop_table_if_exists(cursor, table_name): """Drop the table if it exists""" From a967dcadea525a37de249a4c14f371be777c345c Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Wed, 30 Jul 2025 15:21:51 +0530 Subject: [PATCH 3/6] Adding commit after every cursor execution --- tests/test_005_connection_cursor_lifecycle.py | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/tests/test_005_connection_cursor_lifecycle.py b/tests/test_005_connection_cursor_lifecycle.py index 5fa3d56c..14669b98 100644 --- a/tests/test_005_connection_cursor_lifecycle.py +++ b/tests/test_005_connection_cursor_lifecycle.py @@ -1,4 +1,3 @@ - """ This file contains tests for the Connection class. Functions: @@ -48,6 +47,9 @@ def test_cursor_cleanup_on_connection_close(conn_str): cursor3.execute("SELECT 3") cursor3.fetchall() + # Commit to ensure no uncommitted transactions during cleanup + conn.commit() + # Close one cursor explicitly cursor1.close() assert cursor1.closed is True, "Cursor1 should be closed" @@ -66,6 +68,8 @@ def test_cursor_cleanup_without_close(conn_str): cursor = conn_new.cursor() cursor.execute("SELECT 1") cursor.fetchall() + # Commit to ensure no uncommitted transactions + conn_new.commit() assert len(conn_new._cursors) == 1 del cursor # Remove the last reference assert len(conn_new._cursors) == 0 # Now the WeakSet should be empty @@ -81,6 +85,8 @@ def test_no_segfault_on_gc(conn_str): for cur in cursors: cur.execute("SELECT 1") cur.fetchall() +# Commit to prevent issues with uncommitted transactions during cleanup +conn.commit() del conn import gc; gc.collect() del cursors @@ -105,6 +111,8 @@ def test_multiple_connections_interleaved_cursors(conn_str): cursor.execute('SELECT 1') cursor.fetchall() cursors.append(cursor) + # Commit each connection to prevent issues during cleanup + conn.commit() del conns import gc; gc.collect() del cursors @@ -121,9 +129,18 @@ def test_cursor_outlives_connection(conn_str): cursor = conn.cursor() cursor.execute("SELECT 1") cursor.fetchall() +# Commit before deleting connection to prevent cleanup issues +conn.commit() del conn import gc; gc.collect() -cursor.execute("SELECT 2") +# This should fail gracefully instead of segfaulting +try: + cursor.execute("SELECT 2") + print("ERROR: Should not be able to execute on cursor after connection is deleted") + exit(1) +except Exception as e: + # Expected behavior - cursor should not work after connection is deleted + pass del cursor gc.collect() """ @@ -171,6 +188,9 @@ def test_cursor_cleanup_order_no_segfault(conn_str): cursor.fetchall() cursors.append(cursor) + # Commit to ensure no uncommitted transactions during cleanup + conn.commit() + # Don't close any cursors explicitly # Just close the connection - it should handle cleanup properly conn.close() @@ -212,6 +232,9 @@ def test_connection_close_idempotent(conn_str): conn = connect(conn_str) cursor = conn.cursor() cursor.execute("SELECT 1") + cursor.fetchall() + # Commit to ensure clean state + conn.commit() # First close conn.close() @@ -244,7 +267,7 @@ def test_multiple_cursor_operations_cleanup(conn_str): drop_table_if_exists(cursor_setup, "#test_cleanup") cursor_setup.execute("CREATE TABLE #test_cleanup (id INT, value VARCHAR(50))") cursor_setup.close() - + # Create multiple cursors doing different operations cursor_insert = conn.cursor() cursor_insert.execute("INSERT INTO #test_cleanup VALUES (1, 'test1'), (2, 'test2')") @@ -257,6 +280,9 @@ def test_multiple_cursor_operations_cleanup(conn_str): cursor_select2.execute("SELECT * FROM #test_cleanup WHERE id = 2") cursor_select2.fetchall() + # Commit all changes before closing to prevent cleanup issues + conn.commit() + # Close connection without closing cursors conn.close() From 95a658f9c78d60014b96140ac70a7676a7b9060f Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 31 Jul 2025 12:53:32 +0530 Subject: [PATCH 4/6] add auto rollback on conn close --- mssql_python/connection.py | 6 ++++ mssql_python/pybind/connection/connection.cpp | 7 +++- tests/test_005_connection_cursor_lifecycle.py | 32 ++----------------- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/mssql_python/connection.py b/mssql_python/connection.py index 71e14a48..e352f879 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -259,6 +259,12 @@ def close(self) -> None: # Close the connection even if cursor cleanup had issues try: if self._conn: + if not self.autocommit: + # If autocommit is disabled, rollback any uncommitted changes + # This is important to ensure no partial transactions remain + # For autocommit True, this is not necessary as each statement is committed immediately + self._conn.rollback() + # Close the connection self._conn.close() self._conn = None except Exception as e: diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index a5c5f37f..9782efd2 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -132,9 +132,14 @@ void Connection::setAutocommit(bool enable) { ThrowStdException("Connection handle not allocated"); } SQLINTEGER value = enable ? SQL_AUTOCOMMIT_ON : SQL_AUTOCOMMIT_OFF; - LOG("Set SQL Connection Attribute"); + LOG("Setting SQL Connection Attribute"); SQLRETURN ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, reinterpret_cast(static_cast(value)), 0); checkError(ret); + if(value == SQL_AUTOCOMMIT_ON) { + LOG("SQL Autocommit set to True"); + } else { + LOG("SQL Autocommit set to False"); + } _autocommit = enable; } diff --git a/tests/test_005_connection_cursor_lifecycle.py b/tests/test_005_connection_cursor_lifecycle.py index 14669b98..5fa3d56c 100644 --- a/tests/test_005_connection_cursor_lifecycle.py +++ b/tests/test_005_connection_cursor_lifecycle.py @@ -1,3 +1,4 @@ + """ This file contains tests for the Connection class. Functions: @@ -47,9 +48,6 @@ def test_cursor_cleanup_on_connection_close(conn_str): cursor3.execute("SELECT 3") cursor3.fetchall() - # Commit to ensure no uncommitted transactions during cleanup - conn.commit() - # Close one cursor explicitly cursor1.close() assert cursor1.closed is True, "Cursor1 should be closed" @@ -68,8 +66,6 @@ def test_cursor_cleanup_without_close(conn_str): cursor = conn_new.cursor() cursor.execute("SELECT 1") cursor.fetchall() - # Commit to ensure no uncommitted transactions - conn_new.commit() assert len(conn_new._cursors) == 1 del cursor # Remove the last reference assert len(conn_new._cursors) == 0 # Now the WeakSet should be empty @@ -85,8 +81,6 @@ def test_no_segfault_on_gc(conn_str): for cur in cursors: cur.execute("SELECT 1") cur.fetchall() -# Commit to prevent issues with uncommitted transactions during cleanup -conn.commit() del conn import gc; gc.collect() del cursors @@ -111,8 +105,6 @@ def test_multiple_connections_interleaved_cursors(conn_str): cursor.execute('SELECT 1') cursor.fetchall() cursors.append(cursor) - # Commit each connection to prevent issues during cleanup - conn.commit() del conns import gc; gc.collect() del cursors @@ -129,18 +121,9 @@ def test_cursor_outlives_connection(conn_str): cursor = conn.cursor() cursor.execute("SELECT 1") cursor.fetchall() -# Commit before deleting connection to prevent cleanup issues -conn.commit() del conn import gc; gc.collect() -# This should fail gracefully instead of segfaulting -try: - cursor.execute("SELECT 2") - print("ERROR: Should not be able to execute on cursor after connection is deleted") - exit(1) -except Exception as e: - # Expected behavior - cursor should not work after connection is deleted - pass +cursor.execute("SELECT 2") del cursor gc.collect() """ @@ -188,9 +171,6 @@ def test_cursor_cleanup_order_no_segfault(conn_str): cursor.fetchall() cursors.append(cursor) - # Commit to ensure no uncommitted transactions during cleanup - conn.commit() - # Don't close any cursors explicitly # Just close the connection - it should handle cleanup properly conn.close() @@ -232,9 +212,6 @@ def test_connection_close_idempotent(conn_str): conn = connect(conn_str) cursor = conn.cursor() cursor.execute("SELECT 1") - cursor.fetchall() - # Commit to ensure clean state - conn.commit() # First close conn.close() @@ -267,7 +244,7 @@ def test_multiple_cursor_operations_cleanup(conn_str): drop_table_if_exists(cursor_setup, "#test_cleanup") cursor_setup.execute("CREATE TABLE #test_cleanup (id INT, value VARCHAR(50))") cursor_setup.close() - + # Create multiple cursors doing different operations cursor_insert = conn.cursor() cursor_insert.execute("INSERT INTO #test_cleanup VALUES (1, 'test1'), (2, 'test2')") @@ -280,9 +257,6 @@ def test_multiple_cursor_operations_cleanup(conn_str): cursor_select2.execute("SELECT * FROM #test_cleanup WHERE id = 2") cursor_select2.fetchall() - # Commit all changes before closing to prevent cleanup issues - conn.commit() - # Close connection without closing cursors conn.close() From 0b2272cc027c8c0c8914a817df6934ac37686408 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 31 Jul 2025 14:39:16 +0530 Subject: [PATCH 5/6] Added tests for rollback on close --- tests/test_003_connection.py | 45 +++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index f82b32ed..422445c8 100644 --- a/tests/test_003_connection.py +++ b/tests/test_003_connection.py @@ -7,7 +7,15 @@ - test_commit: Make a transaction and commit. - test_rollback: Make a transaction and rollback. - test_invalid_connection_string: Check if initializing with an invalid connection string raises an exception. -Note: The cursor function is not yet implemented, so related tests are commented out. +- test_connection_pooling_speed: Test connection pooling speed. +- test_connection_pooling_basic: Test basic connection pooling functionality. +- test_autocommit_default: Check if autocommit is False by default. +- test_autocommit_setter: Test setting autocommit mode and its effect on transactions. +- test_set_autocommit: Test the setautocommit method. +- test_construct_connection_string: Check if the connection string is constructed correctly with kwargs. +- test_connection_string_with_attrs_before: Check if the connection string is constructed correctly with attrs_before. +- test_connection_string_with_odbc_param: Check if the connection string is constructed correctly with ODBC parameters. +- test_rollback_on_close: Test that rollback occurs on connection close if autocommit is False. """ from mssql_python.exceptions import InterfaceError @@ -140,6 +148,41 @@ def test_commit(db_connection): cursor.execute("DROP TABLE #pytest_test_commit;") db_connection.commit() +def test_rollback_on_close(conn_str, db_connection): + # Test that rollback occurs on connection close if autocommit is False + # Using a permanent table to ensure rollback is tested correctly + cursor = db_connection.cursor() + drop_table_if_exists(cursor, "pytest_test_rollback_on_close") + try: + # Create a permanent table for testing + cursor.execute("CREATE TABLE pytest_test_rollback_on_close (id INT PRIMARY KEY, value VARCHAR(50));") + db_connection.commit() + + # This simulates a scenario where the connection is closed without committing + # and checks if the rollback occurs + temp_conn = connect(conn_str) + temp_cursor = temp_conn.cursor() + temp_cursor.execute("INSERT INTO pytest_test_rollback_on_close (id, value) VALUES (1, 'test');") + + # Verify data is visible within the same transaction + temp_cursor.execute("SELECT * FROM pytest_test_rollback_on_close WHERE id = 1;") + result = temp_cursor.fetchone() + assert result is not None, "Rollback on close failed: No data found before close" + assert result[1] == 'test', "Rollback on close failed: Incorrect data before close" + + # Close the temporary connection without committing + temp_conn.close() + + # Now check if the data is rolled back + cursor.execute("SELECT * FROM pytest_test_rollback_on_close WHERE id = 1;") + result = cursor.fetchone() + assert result is None, "Rollback on close failed: Data found after rollback" + except Exception as e: + pytest.fail(f"Rollback on close failed: {e}") + finally: + drop_table_if_exists(cursor, "pytest_test_rollback_on_close") + db_connection.commit() + def test_rollback(db_connection): # Make a transaction and rollback cursor = db_connection.cursor() From ed6f6967c93c320ddd29c3555fad83ce6b4dbed7 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Fri, 1 Aug 2025 08:57:06 +0000 Subject: [PATCH 6/6] added a TODO comment to the code --- mssql_python/connection.py | 1 + 1 file changed, 1 insertion(+) diff --git a/mssql_python/connection.py b/mssql_python/connection.py index e352f879..12760df4 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -264,6 +264,7 @@ def close(self) -> None: # This is important to ensure no partial transactions remain # For autocommit True, this is not necessary as each statement is committed immediately self._conn.rollback() + # TODO: Check potential race conditions in case of multithreaded scenarios # Close the connection self._conn.close() self._conn = None