FIX: dispatch output converters on integer ODBC SQL type codes (#684)

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

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

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: ...

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,88 @@ 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).
+        #    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()
+        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()
+
+
 def test_output_converter_with_null_values(db_connection):
     """Test that output converters handle NULL values correctly"""
     cursor = db_connection.cursor()

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()
 
@@ -16401,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
 # ──────────────────────────────────────────────────────────────────────────────