FIX: gate SQL_WVARCHAR output-converter fallback to string/binary columns (#691)

mssql_python/cursor.py

@@ -1124,11 +1124,13 @@ def _build_converter_map(self):
                 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
-            if converter is None:
-                from mssql_python.constants import ConstantsDDBC
-
-                converter = self.connection.get_output_converter(ConstantsDDBC.SQL_WVARCHAR.value)
+            # Legacy WVARCHAR fallback: only apply it when the column's mapped type
+            # is str/bytes, so a registered SQL_WVARCHAR converter is never used as an
+            # unconditional catch-all for INT/DECIMAL/DATE/etc. columns (GH #691). This
+            # mirrors the isinstance(value, (str, bytes)) gate in
+            # Row._apply_output_converters.
+            if converter is None and sql_type in (str, bytes):
+                converter = self.connection.get_output_converter(ddbc_sql_const.SQL_WVARCHAR.value)
 
             converter_map.append(converter)
 

tests/test_003_connection.py

@@ -1908,6 +1908,203 @@ def test_converter_integration(db_connection):
     db_connection.clear_output_converters()
 
 
+def test_output_converter_wvarchar_not_catchall_for_non_string_columns_gh691(db_connection):
+    """GH #691: a SQL_WVARCHAR converter must not be an unconditional catch-all.
+
+    A converter registered only for SQL_WVARCHAR must be invoked for string and
+    binary columns only, and never for INT / DECIMAL / DATE columns. Before the
+    fix, the WVARCHAR converter was placed on every column that lacked a direct
+    type-keyed converter.
+
+    The converter here is a spy that records every value it receives and returns
+    an observable marker for non-bytes input instead of raising. That matters:
+    the optimized apply path swallows converter exceptions, so a converter doing
+    ``value.decode(...)`` would raise-and-be-swallowed on int/Decimal/date and
+    leave the original value intact -- hiding the bug. Recording invocations (and
+    returning a marker) makes the catch-all regression detectable: the call-count
+    assertion below fails on the unfixed code (converter fired on all columns)
+    and passes once the fallback is gated to str/bytes columns.
+    """
+    import decimal
+    import datetime
+
+    sql_wvarchar = ConstantsDDBC.SQL_WVARCHAR.value
+
+    converter_calls = []
+
+    def wvarchar_spy(value):
+        # Record the invocation first, before anything that could raise, so the
+        # spy captures calls even on non-string values (int/Decimal/date).
+        converter_calls.append(value)
+        if isinstance(value, bytes):
+            return "CONV:" + value.decode("utf-16-le")
+        return "CONV_NON_STRING"
+
+    cursor = db_connection.cursor()
+    db_connection.add_output_converter(sql_wvarchar, wvarchar_spy)
+    try:
+        cursor.execute("""
+            SELECT
+                CAST(42 AS INT)                  AS int_col,
+                CAST(3.14 AS DECIMAL(10, 2))     AS dec_col,
+                CAST('2020-01-02' AS DATE)       AS date_col,
+                CAST(N'hello' AS NVARCHAR(50))   AS str_col,
+                CAST(0x41004200 AS VARBINARY(8)) AS bin_col
+            """)
+        row = cursor.fetchone()
+
+        # Non-string columns must be untouched by the WVARCHAR converter.
+        assert isinstance(row[0], int) and row[0] == 42, f"INT column mangled: {row[0]!r}"
+        assert row[1] == decimal.Decimal("3.14"), f"DECIMAL column mangled: {row[1]!r}"
+        assert row[2] == datetime.date(2020, 1, 2), f"DATE column mangled: {row[2]!r}"
+
+        # The string column must still be converted by the WVARCHAR converter.
+        assert row[3] == "CONV:hello", f"NVARCHAR column not converted: {row[3]!r}"
+
+        # The binary column is also handled by the WVARCHAR fallback -- the gate
+        # intentionally includes bytes, mirroring Row._apply_output_converters.
+        assert row[4] == "CONV:AB", f"VARBINARY column not handled by fallback: {row[4]!r}"
+
+        # Contract check that actually catches GH #691: the converter must fire
+        # for exactly the two str/bytes columns, never as a catch-all on the
+        # INT / DECIMAL / DATE columns.
+        assert len(converter_calls) == 2, (
+            "SQL_WVARCHAR converter must fire only on the NVARCHAR and VARBINARY "
+            f"columns; it was invoked {len(converter_calls)} times on "
+            f"{converter_calls!r}"
+        )
+    finally:
+        db_connection.clear_output_converters()
+        cursor.close()
+
+
+def test_output_converter_wvarchar_null_string_gh691(db_connection):
+    """GH #691: a NULL string value stays None and the WVARCHAR converter is never called.
+
+    The gated fallback is placed on the NVARCHAR column (mapped type str), but the apply path
+    short-circuits on NULL, so the converter must not be invoked for a NULL value.
+    """
+    sql_wvarchar = ConstantsDDBC.SQL_WVARCHAR.value
+    calls = []
+
+    def wvarchar_spy(value):
+        calls.append(value)
+        return "CONV:" + value.decode("utf-16-le")
+
+    cursor = db_connection.cursor()
+    db_connection.add_output_converter(sql_wvarchar, wvarchar_spy)
+    try:
+        cursor.execute("SELECT CAST(NULL AS NVARCHAR(20)) AS null_str")
+        value = cursor.fetchone()[0]
+        assert value is None, f"NULL NVARCHAR must remain None, got {value!r}"
+        assert calls == [], "WVARCHAR converter must not be invoked for a NULL value"
+    finally:
+        db_connection.clear_output_converters()
+        cursor.close()
+
+
+def test_output_converter_wvarchar_applies_to_varchar_column_gh691(db_connection):
+    """GH #691: the gated WVARCHAR fallback still applies to VARCHAR columns (mapped type str).
+
+    VARCHAR maps to the Python type ``str`` just like NVARCHAR, so a SQL_WVARCHAR converter
+    fires on it and receives the value as UTF-16LE bytes.
+    """
+    sql_wvarchar = ConstantsDDBC.SQL_WVARCHAR.value
+    calls = []
+
+    def wvarchar_spy(value):
+        calls.append(value)
+        return "CONV:" + value.decode("utf-16-le")
+
+    cursor = db_connection.cursor()
+    db_connection.add_output_converter(sql_wvarchar, wvarchar_spy)
+    try:
+        cursor.execute("SELECT CAST('abc' AS VARCHAR(20)) AS vchar")
+        value = cursor.fetchone()[0]
+        assert value == "CONV:abc", f"VARCHAR column not converted by WVARCHAR fallback: {value!r}"
+        assert len(calls) == 1, f"WVARCHAR converter should fire once on VARCHAR, got {len(calls)}"
+    finally:
+        db_connection.clear_output_converters()
+        cursor.close()
+
+
+def test_output_converter_wvarchar_mixed_result_set_gh691(db_connection):
+    """GH #691: in a mixed row, the WVARCHAR converter fires only on the string column.
+
+    INT and DECIMAL columns (non str/bytes mapped types) must be untouched; only the NVARCHAR
+    column is converted, and the converter is invoked exactly once.
+    """
+    import decimal
+
+    sql_wvarchar = ConstantsDDBC.SQL_WVARCHAR.value
+    calls = []
+
+    def wvarchar_spy(value):
+        calls.append(value)
+        if isinstance(value, bytes):
+            return "CONV:" + value.decode("utf-16-le")
+        return "CONV_NON_STRING"
+
+    cursor = db_connection.cursor()
+    db_connection.add_output_converter(sql_wvarchar, wvarchar_spy)
+    try:
+        cursor.execute(
+            "SELECT CAST(1 AS INT) AS i, "
+            "CAST('abc' AS NVARCHAR(10)) AS s, "
+            "CAST(12.34 AS DECIMAL(10, 2)) AS d"
+        )
+        i_val, s_val, d_val = cursor.fetchone()
+        assert isinstance(i_val, int) and i_val == 1, f"INT column mangled: {i_val!r}"
+        assert s_val == "CONV:abc", f"NVARCHAR column not converted: {s_val!r}"
+        assert d_val == decimal.Decimal("12.34"), f"DECIMAL column mangled: {d_val!r}"
+        assert len(calls) == 1, (
+            "WVARCHAR converter must fire only on the NVARCHAR column; "
+            f"invoked {len(calls)} times on {calls!r}"
+        )
+    finally:
+        db_connection.clear_output_converters()
+        cursor.close()
+
+
+def test_output_converter_wvarchar_cached_map_multiple_fetches_gh691(db_connection):
+    """GH #691: the cached converter map behaves identically across multiple fetched rows.
+
+    The gated WVARCHAR fallback is computed once per statement; every row in a multi-row
+    result must be converted consistently, and the INT column must never be touched.
+    """
+    sql_wvarchar = ConstantsDDBC.SQL_WVARCHAR.value
+    calls = []
+
+    def wvarchar_spy(value):
+        calls.append(value)
+        if isinstance(value, bytes):
+            return "CONV:" + value.decode("utf-16-le")
+        return "CONV_NON_STRING"
+
+    cursor = db_connection.cursor()
+    db_connection.add_output_converter(sql_wvarchar, wvarchar_spy)
+    try:
+        cursor.execute(
+            "SELECT CAST(10 AS INT) AS i, CAST(N'a' AS NVARCHAR(10)) AS s "
+            "UNION ALL SELECT CAST(20 AS INT), CAST(N'b' AS NVARCHAR(10)) "
+            "UNION ALL SELECT CAST(30 AS INT), CAST(N'c' AS NVARCHAR(10))"
+        )
+        rows = cursor.fetchall()
+        assert [r[0] for r in rows] == [10, 20, 30], "INT column must be untouched on every row"
+        assert [r[1] for r in rows] == [
+            "CONV:a",
+            "CONV:b",
+            "CONV:c",
+        ], "NVARCHAR column must be converted consistently on every row"
+        # Exactly one invocation per row -- the string column only, never the INT column.
+        assert (
+            len(calls) == 3
+        ), f"WVARCHAR converter must fire once per row (3 total), got {len(calls)}: {calls!r}"
+    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()