From 53237f88dfe8428d113d7fc4d40953a6fa63d33b Mon Sep 17 00:00:00 2001 From: Rene Date: Fri, 23 Oct 2026 18:22:28 +0200 Subject: [PATCH 1/2] fix(dialect): delegate do_rollback to dbapi connection Currently, DatabendDialect.do_rollback is a pass statement under the comment '# No transactions'. While Databend does not support multi-statement ACID transactions, swallowing rollback calls hides writes that have already been committed/executed on the server when a caller expects rollback to either undo changes or raise NotSupportedError. Propagate do_rollback to dbapi_connection.rollback() so that the underlying connector can either handle it or raise NotSupportedError as designed. Also add unit tests verifying delegation and error propagation. Closes #77 --- databend_sqlalchemy/databend_dialect.py | 3 +-- tests/unit/test_databend_dialect.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/databend_sqlalchemy/databend_dialect.py b/databend_sqlalchemy/databend_dialect.py index 9af3ef3..c4aefc6 100644 --- a/databend_sqlalchemy/databend_dialect.py +++ b/databend_sqlalchemy/databend_dialect.py @@ -1817,8 +1817,7 @@ def get_multi_table_comment( ) def do_rollback(self, dbapi_connection): - # No transactions - pass + dbapi_connection.rollback() def _check_unicode_returns(self, connection, additional_tests=None): # We decode everything as UTF-8 diff --git a/tests/unit/test_databend_dialect.py b/tests/unit/test_databend_dialect.py index 122e037..9f7adc7 100644 --- a/tests/unit/test_databend_dialect.py +++ b/tests/unit/test_databend_dialect.py @@ -3,12 +3,14 @@ from unittest import mock import sqlalchemy +import pytest from conftest import MockCursor, MockDBApi from pytest import mark from sqlalchemy.engine import url from sqlalchemy.sql import text import databend_sqlalchemy # SQLAlchemy package +from databend_sqlalchemy.errors import NotSupportedError from databend_sqlalchemy.databend_dialect import ( DatabendCompiler, DatabendDialect, @@ -56,6 +58,22 @@ def test_do_execute( cursor.execute.reset_mock() dialect.do_execute(cursor, "SELECT *", (1, 22), None) + def test_do_rollback_propagates_rollback(self, dialect: DatabendDialect): + mock_conn = mock.Mock() + dialect.do_rollback(mock_conn) + mock_conn.rollback.assert_called_once() + + def test_do_rollback_raises_not_supported_error(self, dialect: DatabendDialect): + mock_conn = mock.Mock() + mock_conn.rollback.side_effect = NotSupportedError( + "Transactions are not supported" + ) + with pytest.raises( + NotSupportedError, + match="Transactions are not supported", + ): + dialect.do_rollback(mock_conn) + def test_table_names( self, dialect: DatabendDialect, connection: mock.Mock(spec=MockDBApi) ): From de12ad9fec34346bdaecb71c03bf7b50b797c2a4 Mon Sep 17 00:00:00 2001 From: Rene Date: Fri, 23 Oct 2026 18:47:06 +0200 Subject: [PATCH 2/2] fix(dialect): safely suppress NotSupportedError on rollback during pool lifecycle Ensure do_rollback catches NotSupportedError and AttributeError when delegating to dbapi_connection.rollback(). This allows underlying drivers supporting rollback to execute it, while preventing connection checkout/cleanup and pool reset_on_return from raising NotSupportedError on non-transactional connectors. --- databend_sqlalchemy/databend_dialect.py | 17 ++++++++++++++++- tests/unit/test_databend_dialect.py | 19 +++++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/databend_sqlalchemy/databend_dialect.py b/databend_sqlalchemy/databend_dialect.py index c4aefc6..2d751d8 100644 --- a/databend_sqlalchemy/databend_dialect.py +++ b/databend_sqlalchemy/databend_dialect.py @@ -27,6 +27,7 @@ import re import operator import datetime +import logging from types import NoneType import sqlalchemy.engine.reflection @@ -43,6 +44,9 @@ Select, Subquery, ) +from databend_sqlalchemy.errors import NotSupportedError + +logger = logging.getLogger(__name__) from sqlalchemy.dialects.postgresql.base import PGCompiler, PGIdentifierPreparer from sqlalchemy import Table, MetaData, Column from sqlalchemy.types import ( @@ -1817,7 +1821,18 @@ def get_multi_table_comment( ) def do_rollback(self, dbapi_connection): - dbapi_connection.rollback() + dbapi = getattr(self, "dbapi", None) + not_supported = ( + getattr(dbapi, "NotSupportedError", NotSupportedError), + NotSupportedError, + AttributeError, + ) + try: + dbapi_connection.rollback() + except not_supported as e: + logger.debug( + "Database rollback not supported by underlying connection: %s", e + ) def _check_unicode_returns(self, connection, additional_tests=None): # We decode everything as UTF-8 diff --git a/tests/unit/test_databend_dialect.py b/tests/unit/test_databend_dialect.py index 9f7adc7..a242cdc 100644 --- a/tests/unit/test_databend_dialect.py +++ b/tests/unit/test_databend_dialect.py @@ -63,16 +63,23 @@ def test_do_rollback_propagates_rollback(self, dialect: DatabendDialect): dialect.do_rollback(mock_conn) mock_conn.rollback.assert_called_once() - def test_do_rollback_raises_not_supported_error(self, dialect: DatabendDialect): + def test_do_rollback_safely_handles_not_supported_error( + self, dialect: DatabendDialect + ): mock_conn = mock.Mock() mock_conn.rollback.side_effect = NotSupportedError( "Transactions are not supported" ) - with pytest.raises( - NotSupportedError, - match="Transactions are not supported", - ): - dialect.do_rollback(mock_conn) + # Must not raise to ensure pool checkout/checkin lifecycle succeeds + dialect.do_rollback(mock_conn) + mock_conn.rollback.assert_called_once() + + def test_do_rollback_safely_handles_attribute_error( + self, dialect: DatabendDialect + ): + mock_conn = object() + # Must not raise if connection object lacks rollback method + dialect.do_rollback(mock_conn) def test_table_names( self, dialect: DatabendDialect, connection: mock.Mock(spec=MockDBApi)