diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index 9515ace2..addaf854 100644 --- a/mssql_python/cursor.py +++ b/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) diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index 590bc0c4..9ac33a59 100644 --- a/tests/test_003_connection.py +++ b/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()