From 9eb1a7107edf5bbe19c708801e0ebf0e3a9449a1 Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 28 Jul 2026 12:28:43 +0530 Subject: [PATCH 1/4] FIX: dispatch output converters on integer ODBC SQL type codes (#684) add_output_converter documents an integer ODBC SQL type key, but dispatch keyed on the Python type in cursor.description[i][1], so integer keys silently never fired. Store per-column raw SQL type codes and dispatch on the integer code first, then Python type, then the legacy WVARCHAR catch-all. Also build the converter map for catalog metadata result sets so converters apply consistently. --- mssql_python/connection.py | 27 ++++++++---- mssql_python/cursor.py | 34 ++++++++++++++-- mssql_python/mssql_python.pyi | 4 +- tests/test_003_connection.py | 77 +++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 13 deletions(-) diff --git a/mssql_python/connection.py b/mssql_python/connection.py index 4ef00180..abef94ec 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -1212,7 +1212,7 @@ def cursor(self) -> Cursor: logger.debug("cursor: Cursor created successfully - total_cursors=%d", len(self._cursors)) return cursor - def add_output_converter(self, sqltype: int, func: Callable[[Any], Any]) -> None: + def add_output_converter(self, sqltype: Union[int, type], func: Callable[[Any], Any]) -> None: """ Register an output converter function that will be called whenever a value with the given SQL type is read from the database. @@ -1225,13 +1225,24 @@ def add_output_converter(self, sqltype: int, func: Callable[[Any], Any]) -> None vulnerabilities. This API should never be exposed to untrusted or external input. Args: - sqltype (int): The integer SQL type value to convert, which can be one of the - defined standard constants (e.g. SQL_VARCHAR) or a database-specific - value (e.g. -151 for the SQL Server 2008 geometry data type). - func (callable): The converter function which will be called with a single parameter, - the value, and should return the converted value. If the value is NULL - then the parameter passed to the function will be None, otherwise it - will be a bytes object. + sqltype (int or type): The type to convert. Either: + - an integer ODBC SQL type code (pyodbc-compatible), which can be one of + the standard constants (e.g. SQL_VARCHAR, SQL_DECIMAL) or a + database-specific value (e.g. -151 for the SQL Server geometry type). The + converter fires for columns whose ODBC SQL type matches exactly, so + distinct types such as DECIMAL and NUMERIC can have separate converters; or + - a Python type (e.g. ``decimal.Decimal``, ``str``, ``bytes``), which fires + for every column whose value materializes to that Python type (so, for + example, a single ``decimal.Decimal`` converter matches DECIMAL, NUMERIC, + MONEY and SMALLMONEY columns alike). + When both an integer-keyed and a Python-type-keyed converter could apply to + the same column, the integer SQL-type converter takes precedence. + func (callable): The converter function, called with a single parameter (the + value) that returns the converted value. If the value is NULL the + parameter is None. Otherwise the parameter is the value already + materialized as its Python type (e.g. a ``decimal.Decimal`` or + ``datetime.datetime``); string values are passed as their + UTF-16LE-encoded ``bytes``. Returns: None diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index bc2956d7..ecf1cc26 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -152,6 +152,10 @@ def __init__(self, connection: "Connection", timeout: int = 0) -> None: self._cached_column_map = None self._cached_column_map_lower = None self._cached_converter_map = None + # Raw ODBC SQL type codes (from SQLDescribeCol) per column, parallel to + # self.description. Kept so output-converter dispatch can key on the integer + # ODBC SQL type code (pyodbc-compatible), not just the mapped Python type. See #684. + self._column_sql_types = None self._uuid_str_indices = None # Pre-computed UUID column indices for str conversion # Cache the effective native_uuid setting for this cursor's connection. # Resolution order: connection._native_uuid (if not None) → module-level setting. @@ -1063,9 +1067,13 @@ def _initialize_description(self, column_metadata: Optional[Any] = None) -> None """Initialize the description attribute from column metadata.""" if not column_metadata: self.description = None + self._column_sql_types = None return description = [] + # Raw ODBC SQL type codes, parallel to description, for output-converter + # dispatch by integer SQL type (see _build_converter_map / #684). + sql_type_codes = [] for _, col in enumerate(column_metadata): # Get column name - lowercase it if the lowercase flag is set column_name = col["ColumnName"] @@ -1074,6 +1082,7 @@ def _initialize_description(self, column_metadata: Optional[Any] = None) -> None if get_settings().lowercase: column_name = column_name.lower() + sql_type_codes.append(col["DataType"]) # Add to description tuple (7 elements as per PEP-249) description.append( ( @@ -1087,6 +1096,7 @@ def _initialize_description(self, column_metadata: Optional[Any] = None) -> None ) ) self.description = description + self._column_sql_types = sql_type_codes def _build_converter_map(self): """ @@ -1101,15 +1111,23 @@ def _build_converter_map(self): ): return None + sql_type_codes = self._column_sql_types converter_map = [] - for desc in self.description: + for i, desc in enumerate(self.description): if desc is None: converter_map.append(None) continue - sql_type = desc[1] - converter = self.connection.get_output_converter(sql_type) - # If no converter found for the SQL type, try the WVARCHAR converter as a fallback + converter = None + # 1) pyodbc-compatible: dispatch on the raw ODBC integer SQL type code + # (e.g. SQL_DECIMAL). This is the key add_output_converter documents. #684 + if sql_type_codes is not None and i < len(sql_type_codes): + converter = self.connection.get_output_converter(sql_type_codes[i]) + # 2) fall back to the Python type stored in description[i][1] + # (e.g. decimal.Decimal) - the pre-existing mssql-python key style. + if converter is None: + converter = self.connection.get_output_converter(desc[1]) + # 3) last-resort WVARCHAR converter fallback (legacy behavior) if converter is None: from mssql_python.constants import ConstantsDDBC @@ -1567,6 +1585,7 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state except Exception as e: # pylint: disable=broad-exception-caught # If describe fails, it's likely there are no results (e.g., for INSERT) self.description = None + self._column_sql_types = None # Reset rownumber for new result set (only for SELECT statements) if self.description: # If we have column descriptions, it's likely a SELECT @@ -1636,6 +1655,11 @@ def _prepare_metadata_result_set( # pylint: disable=too-many-statements if not self.description and fallback_description: self.description = fallback_description + # Build the converter map so output converters (including integer SQL-type + # keys) apply to metadata result sets consistently with normal result sets. + # See GH #684. + self._cached_converter_map = self._build_converter_map() + # Build the column-name -> index map for this metadata result set. # Both the exact name and its lowercase alias are stored so that rows # support case-insensitive lookup regardless of the global ``lowercase`` @@ -2437,6 +2461,7 @@ def executemany( # pylint: disable=too-many-locals,too-many-branches,too-many-s self._initialize_description(column_metadata) except Exception: # pylint: disable=broad-exception-caught self.description = None + self._column_sql_types = None if self.description: self.rowcount = -1 @@ -2766,6 +2791,7 @@ def nextset(self) -> Optional[bool]: self._cached_column_map = None self._cached_column_map_lower = None self._cached_converter_map = None + self._column_sql_types = None self._uuid_str_indices = None # Skip to the next result set diff --git a/mssql_python/mssql_python.pyi b/mssql_python/mssql_python.pyi index ad18756e..cb9d8928 100644 --- a/mssql_python/mssql_python.pyi +++ b/mssql_python/mssql_python.pyi @@ -266,7 +266,9 @@ class Connection: ) -> None: ... def getdecoding(self, sqltype: int) -> Dict[str, Union[str, int]]: ... def set_attr(self, attribute: int, value: Union[int, str, bytes, bytearray]) -> None: ... - def add_output_converter(self, sqltype: int, func: Callable[[Any], Any]) -> None: ... + def add_output_converter( + self, sqltype: Union[int, type], func: Callable[[Any], Any] + ) -> None: ... def get_output_converter(self, sqltype: Union[int, type]) -> Optional[Callable[[Any], Any]]: ... def remove_output_converter(self, sqltype: Union[int, type]) -> None: ... def clear_output_converters(self) -> None: ... diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index 590bc0c4..65050823 100644 --- a/tests/test_003_connection.py +++ b/tests/test_003_connection.py @@ -21,6 +21,7 @@ from mssql_python.exceptions import InterfaceError, ProgrammingError, DatabaseError import mssql_python +import decimal import sys import pytest import time @@ -1908,6 +1909,82 @@ def test_converter_integration(db_connection): db_connection.clear_output_converters() +def test_output_converter_integer_sql_type_key_gh684(db_connection): + """Integer ODBC SQL-type keys must dispatch (pyodbc-compatible). Regression for GH #684. + + add_output_converter documents an integer SQL type code, but dispatch used to key on + the Python type in cursor.description[i][1], so integer keys silently never fired. + """ + cursor = db_connection.cursor() + decimal_query = "SELECT CAST(19.99 AS DECIMAL(10, 2)) AS price" + + try: + # 1) Integer SQL-type key fires (the reported bug: it used to silently no-op). + db_connection.add_output_converter( + mssql_python.SQL_DECIMAL, lambda v: None if v is None else float(v) + ) + cursor.execute(decimal_query) + value = cursor.fetchone()[0] + assert isinstance(value, float), "Integer SQL_DECIMAL converter did not fire" + assert value == 19.99 + db_connection.clear_output_converters() + + # 2) Exact-type dispatch: an SQL_INTEGER converter must NOT touch a DECIMAL column. + db_connection.add_output_converter(mssql_python.SQL_INTEGER, lambda v: "SHOULD_NOT_FIRE") + cursor.execute(decimal_query) + value = cursor.fetchone()[0] + assert isinstance(value, decimal.Decimal), "SQL_INTEGER converter wrongly hit DECIMAL" + # ...but it does fire on an INTEGER column. + cursor.execute("SELECT CAST(42 AS INT) AS n") + assert cursor.fetchone()[0] == "SHOULD_NOT_FIRE", "SQL_INTEGER converter did not fire" + db_connection.clear_output_converters() + + # 3) Integer SQL-type key takes precedence over a Python-type key on the same column. + db_connection.add_output_converter(decimal.Decimal, lambda v: "python-type") + db_connection.add_output_converter(mssql_python.SQL_DECIMAL, lambda v: "int-type") + cursor.execute(decimal_query) + assert cursor.fetchone()[0] == "int-type", "Integer key should win over Python-type key" + db_connection.clear_output_converters() + + # 4) Distinct converters for DECIMAL vs NUMERIC (exact type, not collapsed to Decimal). + db_connection.add_output_converter(mssql_python.SQL_DECIMAL, lambda v: "D") + db_connection.add_output_converter(mssql_python.SQL_NUMERIC, lambda v: "N") + cursor.execute("SELECT CAST(1.5 AS DECIMAL(10, 2)) AS x") + assert cursor.fetchone()[0] == "D" + cursor.execute("SELECT CAST(1.5 AS NUMERIC(10, 2)) AS x") + assert cursor.fetchone()[0] == "N" + db_connection.clear_output_converters() + + # 5) NULL still yields None regardless of an integer-keyed converter. + db_connection.add_output_converter( + mssql_python.SQL_DECIMAL, lambda v: None if v is None else float(v) + ) + cursor.execute("SELECT CAST(NULL AS DECIMAL(10, 2)) AS price") + assert cursor.fetchone()[0] is None + db_connection.clear_output_converters() + + # 6) String path: an integer SQL_WVARCHAR key fires on an NVARCHAR column and + # receives the raw value as UTF-16LE bytes (the documented string contract). + db_connection.add_output_converter( + mssql_python.SQL_WVARCHAR, + lambda v: None if v is None else "CONV:" + v.decode("utf-16-le"), + ) + cursor.execute("SELECT CAST(N'hello' AS NVARCHAR(50)) AS s") + assert cursor.fetchone()[0] == "CONV:hello", "Integer SQL_WVARCHAR converter did not fire" + db_connection.clear_output_converters() + + # 7) Metadata result sets also honor output converters (GH #684 metadata path). + # Registering any converter must cause the metadata converter map to be built. + db_connection.add_output_converter(mssql_python.SQL_WVARCHAR, lambda v: v) + cursor.tables() + assert ( + cursor._cached_converter_map is not None + ), "Metadata result sets must build a converter map so output converters apply" + finally: + db_connection.clear_output_converters() + cursor.close() + + def test_output_converter_with_null_values(db_connection): """Test that output converters handle NULL values correctly""" cursor = db_connection.cursor() From 51ac0f0f1b954e65b95b70b56bbdc1a3f9d1c6c0 Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 28 Jul 2026 15:21:00 +0530 Subject: [PATCH 2/4] FIX: update output-converter exception test for GH #684 integer-key dispatch test_row_output_converter_general_exception registered a converter under integer SQL type 12 (SQL_VARCHAR), which previously never fired due to the GH #684 dispatch bug. Now that integer keys dispatch correctly, the converter fires and receives UTF-16LE bytes, so the old value=="test_value" guard no longer matches. Make the converter raise unconditionally to faithfully exercise the "converter raised -> keep original value" path, and move the converter restore into finally so a failed assertion can never leak the converter onto the shared connection (which was cascading into 6 unrelated VARCHAR test failures). --- tests/test_004_cursor.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/tests/test_004_cursor.py b/tests/test_004_cursor.py index 5c2be217..ced17db2 100644 --- a/tests/test_004_cursor.py +++ b/tests/test_004_cursor.py @@ -14870,6 +14870,12 @@ def problematic_converter(value): def test_row_output_converter_general_exception(cursor, db_connection): """Test Row output converter general exception handling (Lines 198-206).""" + # Snapshot converters up front so the finally can ALWAYS restore them, even if + # an assertion below fails. Otherwise the {12: failing_converter} entry would + # leak onto the shared connection and corrupt every later VARCHAR fetch. + had_converters_attr = hasattr(cursor.connection, "_output_converters") + original_converters = getattr(cursor.connection, "_output_converters", {}) + try: # Create a table with string column drop_table_if_exists(cursor, "#pytest_exception_test") @@ -14887,17 +14893,17 @@ def test_row_output_converter_general_exception(cursor, db_connection): ) db_connection.commit() - # Create a custom output converter that will raise a general exception + # A converter that always raises, to exercise the "converter raised -> + # keep the original value" path. Registered under integer SQL type 12 + # (SQL_VARCHAR); after the GH #684 fix this integer key actually + # dispatches and string values arrive as UTF-16LE bytes, so we raise + # unconditionally rather than guarding on the decoded text. def failing_converter(value): - if value == "test_value": - raise RuntimeError("Custom converter error for testing") - return value + raise RuntimeError("Custom converter error for testing") # Add the converter to the connection (if supported) - original_converters = {} - if hasattr(cursor.connection, "_output_converters"): - original_converters = getattr(cursor.connection, "_output_converters", {}) - cursor.connection._output_converters = {12: failing_converter} # VARCHAR SQL type + if had_converters_attr: + cursor.connection._output_converters = {12: failing_converter} # SQL_VARCHAR # Fetch the data - this should trigger lines 198-206 in row.py cursor.execute("SELECT id, text_col FROM #pytest_exception_test") @@ -14912,13 +14918,13 @@ def failing_converter(value): # The exception should be handled and original value kept assert row[1] == "test_value", "Value should be kept as original due to exception handling" - # Restore original converters - if hasattr(cursor.connection, "_output_converters"): - cursor.connection._output_converters = original_converters - except Exception as e: pytest.fail(f"Output converter general exception test failed: {e}") finally: + # Always restore converters (even on assertion failure) so a leaked + # converter can never poison subsequent tests on the shared connection. + if had_converters_attr: + cursor.connection._output_converters = original_converters drop_table_if_exists(cursor, "#pytest_exception_test") db_connection.commit() From 504607bef13ac14ac52cacdee3acd3138cef7005 Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 28 Jul 2026 15:27:08 +0530 Subject: [PATCH 3/4] FIX: make GH #684 metadata converter test black-box Assert user-visible behavior (a catalog string column is actually passed through the registered SQL_WVARCHAR converter) instead of the private cursor._cached_converter_map cache field, which was brittle to internal refactors. tables() row string columns now come back as "CONV:..." while NULL stays None. --- tests/test_003_connection.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index 65050823..192a5ccc 100644 --- a/tests/test_003_connection.py +++ b/tests/test_003_connection.py @@ -1974,12 +1974,18 @@ def test_output_converter_integer_sql_type_key_gh684(db_connection): db_connection.clear_output_converters() # 7) Metadata result sets also honor output converters (GH #684 metadata path). - # Registering any converter must cause the metadata converter map to be built. - db_connection.add_output_converter(mssql_python.SQL_WVARCHAR, lambda v: v) + # Black-box check: a string column from a catalog result set must be passed + # through the registered SQL_WVARCHAR converter, not just cached internally. + db_connection.add_output_converter( + mssql_python.SQL_WVARCHAR, + lambda v: None if v is None else "CONV:" + v.decode("utf-16-le"), + ) cursor.tables() - assert ( - cursor._cached_converter_map is not None - ), "Metadata result sets must build a converter map so output converters apply" + row = cursor.fetchone() + assert row is not None, "tables() should return at least one catalog row" + assert any( + isinstance(col, str) and col.startswith("CONV:") for col in row + ), "Metadata result sets must apply output converters to their string columns" finally: db_connection.clear_output_converters() cursor.close() From 714e84492af7f0e04a5747b14ab102a66b6fa6eb Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Tue, 28 Jul 2026 15:37:03 +0530 Subject: [PATCH 4/4] TEST: cover execute() describe-failure reset of _column_sql_types (GH #684) Adds test_execute_describe_col_exception_resets_description_and_sql_types, which patches ddbc_bindings.DDBCSQLDescribeCol to raise during execute() and asserts both self.description and self._column_sql_types are reset to None. Seeds a real SELECT first so the reset has a populated SQL-type list to clear. Covers the previously-uncovered defensive reset line in the execute() describe-failure except branch (diff coverage gap flagged by the coverage bot). --- tests/test_004_cursor.py | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/test_004_cursor.py b/tests/test_004_cursor.py index ced17db2..f756e89b 100644 --- a/tests/test_004_cursor.py +++ b/tests/test_004_cursor.py @@ -16407,6 +16407,50 @@ def describe_raises(*args, **kwargs): mssql_python.native_uuid = original +def test_execute_describe_col_exception_resets_description_and_sql_types(conn_str): + """execute() must reset description AND _column_sql_types when DDBCSQLDescribeCol raises. + + Guards the except branch in execute() (GH #684) that sets both + self.description = None and self._column_sql_types = None, so a stale + per-column SQL-type list can't survive into the next converter-map build. + """ + conn = mssql_python.connect(conn_str) + cursor = conn.cursor() + try: + # Run a normal SELECT first so description and the parallel SQL-type + # codes are populated (the reset below then has something to clear). + cursor.execute("SELECT CAST(1 AS INT) AS n") + cursor.fetchall() + assert cursor.description is not None + assert cursor._column_sql_types is not None + + call_count = 0 + + def describe_raises(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise RuntimeError("Simulated DDBCSQLDescribeCol failure") + + # Force DDBCSQLDescribeCol to raise so execute()'s except branch runs. + with patch.object( + mssql_python.cursor.ddbc_bindings, + "DDBCSQLDescribeCol", + side_effect=describe_raises, + ): + cursor.execute("SELECT CAST(1 AS INT) AS n") + + assert call_count >= 1, "DDBCSQLDescribeCol mock should have been called" + assert ( + cursor.description is None + ), "description should be None after DDBCSQLDescribeCol raises" + assert ( + cursor._column_sql_types is None + ), "_column_sql_types should be reset to None after DDBCSQLDescribeCol raises" + finally: + cursor.close() + conn.close() + + # ────────────────────────────────────────────────────────────────────────────── # native_uuid concurrency & thread-safety tests # ──────────────────────────────────────────────────────────────────────────────