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..192a5ccc 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,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() diff --git a/tests/test_004_cursor.py b/tests/test_004_cursor.py index 5c2be217..f756e89b 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() @@ -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 # ──────────────────────────────────────────────────────────────────────────────