FIX: gate SQL_WVARCHAR output-converter fallback to string/binary columns (#691) - #692
Open
jahnvi480 wants to merge 1 commit into
Open
FIX: gate SQL_WVARCHAR output-converter fallback to string/binary columns (#691)#692jahnvi480 wants to merge 1 commit into
jahnvi480 wants to merge 1 commit into
Conversation
…umns (#691) Cursor._build_converter_map applied a converter registered for SQL_WVARCHAR as an unconditional catch-all to every column that lacked a direct type-keyed converter, so a lone SQL_WVARCHAR converter mangled INT / DECIMAL / DATE values. Gate the fallback on the column's mapped Python type being str/bytes, mirroring the isinstance(value, (str, bytes)) guard already used in Row._apply_output_converters, so the optimized and fallback apply paths agree. Also drop the now-redundant in-loop import of ConstantsDDBC (already imported at module scope as ddbc_sql_const). Adds a regression test that spies on converter invocations and asserts the converter fires only on the NVARCHAR / VARBINARY columns (a call-count contract). This is necessary because the optimized apply path swallows converter exceptions: a plain value.decode() converter raises AttributeError on int/Decimal/date and is silently swallowed, so a value-only assertion would pass with and without the fix. The rewritten test fails on the unfixed code and passes once the fallback is gated.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an output-converter dispatch bug in the optimized fetch path: a converter registered for SQL_WVARCHAR was being treated as an unconditional fallback for any column without a direct converter, which could corrupt non-string types (e.g., INT, DECIMAL, DATE). The change gates the legacy SQL_WVARCHAR fallback so it only applies to columns whose mapped Python type is str or bytes, aligning the optimized converter map path with the existing runtime guard behavior.
Changes:
- Gate the
SQL_WVARCHARfallback inCursor._build_converter_map()to only apply whendesc[1]maps tostr/bytes. - Add a regression test covering mixed
INT/DECIMAL/DATEwithNVARCHARandVARBINARY, including a converter invocation count assertion to detect the previously-masked behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mssql_python/cursor.py |
Restricts the legacy SQL_WVARCHAR fallback converter to string/binary-mapped columns during converter map construction. |
tests/test_003_connection.py |
Adds a regression test ensuring SQL_WVARCHAR converters do not act as a catch-all for non-string columns and asserts call count. |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.3%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 83.7%
mssql_python.connection.py: 84.7%🔗 Quick Links
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
A SQL_WVARCHAR output converter was being applied as an unconditional
catch-all to non-string columns.
Cursor._build_converter_mapfell back to theconverter registered for
SQL_WVARCHARfor any column that had no directtype-keyed converter, so registering a single
SQL_WVARCHARconverter mangledINT/DECIMAL/DATEvalues (e.g.42came back converted).This PR gates that legacy fallback on the column's mapped Python type being
str/bytes, mirroring theisinstance(value, (str, bytes))guard alreadypresent in
Row._apply_output_converters. The optimized and fallback applypaths now behave consistently.
Root cause
sql_type = desc[1]is the column's mapped Python type. The fallback ranwhenever no direct converter matched, regardless of the column type:
Row._apply_output_convertersgates the same fallback onisinstance(value, (str, bytes)), so the two paths disagreed.Fix
bytesis intentionally included for parity withRow._apply_output_converters(a
SQL_WVARCHARconverter still applies toVARBINARY, which the new testasserts). Also removed the now-redundant in-loop
ConstantsDDBCimport.Test
tests/test_003_connection.py::test_output_converter_wvarchar_not_catchall_for_non_string_columns_gh691registers only a
SQL_WVARCHARconverter and selectsINT,DECIMAL,DATE,NVARCHAR, andVARBINARYcolumns. It asserts the non-string columns areuntouched, the string/binary columns are converted, and — via a spy — that the
converter is invoked exactly twice (only the
str/bytescolumns).The call-count assertion matters: the optimized apply path swallows converter
exceptions, so a plain
value.decode()converter raisesAttributeErroronint/Decimal/dateand is silently swallowed. A value-only assertion wouldtherefore pass with and without the fix. Verified (via
git stash) that thetest fails on the unfixed code and passes once the fallback is gated.
Validation
black --check --line-length=100clean on both files.tests/test_003_connection.py: 151 passed, 2 skipped.tests/test_004_cursor.py: 517 passed, 4 skipped.