Skip to content

FIX: fetchone/fetchmany/fetchall type-checking under ty (#620)#631

Merged
gargsaumya merged 7 commits into
mainfrom
saumya/gh-620
Jun 11, 2026
Merged

FIX: fetchone/fetchmany/fetchall type-checking under ty (#620)#631
gargsaumya merged 7 commits into
mainfrom
saumya/gh-620

Conversation

@gargsaumya

@gargsaumya gargsaumya commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

AB#45498

GitHub Issue: #620


Summary

This pull request refactors how column name mappings are handled for catalog/metadata result sets in the mssql_python/cursor.py module, specifically to avoid dynamically reassigning fetch methods (like fetchone, fetchmany, and fetchall) as instance attributes. This change improves compatibility with static type checkers and ensures cleaner, more predictable method behavior. A regression test is also added to prevent future regressions.

Refactoring of column mapping and fetch method handling:

  • Refactored the _prepare_metadata_result_set method to build and cache the column name to index map (_column_map) directly, merging in specialized aliases when provided, and removed all logic that dynamically wraps or reassigns fetch methods on the instance. This ensures fetch methods remain class methods and improves static type checking compatibility. [1] [2]
  • Updated the __init__ method of the cursor to initialize the new _column_map attribute, and clarified the usage of cached column maps.
  • Removed obsolete logic in execute that restored original fetch methods, since fetch methods are no longer replaced at runtime.

Testing and regression prevention:

Catalog/metadata helpers reassigned cursor.fetchone/fetchmany/fetchall to closures on the instance, which made static type checkers infer a union of bound method and unbound function and report a spurious missing 'self' argument at call sites.

Route the catalog column map (including lowercase and specialized aliases) through the existing cached-map state that the standard fetch methods already consume, so metadata rows pick up catalog column names without any per-call method reassignment. Adds a regression test ensuring the fetch methods are never shadowed on the instance.
@gargsaumya gargsaumya changed the title Fix fetchone/fetchmany/fetchall type-checking under ty (#620) FIX: fetchone/fetchmany/fetchall type-checking under ty (#620) Jun 10, 2026
@github-actions github-actions Bot added the pr-size: medium Moderate update size label Jun 10, 2026
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

87%


🎯 Overall Coverage

80%


📈 Total Lines Covered: 6666 out of 8238
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/cursor.py (87.5%): Missing lines 1641

Summary

  • Total: 8 lines
  • Missing: 1 line
  • Coverage: 87%

mssql_python/cursor.py

Lines 1637-1645

  1637         # Some catalog helpers expose additional friendly aliases (e.g. ODBC 2.x
  1638         # vs 3.x column names) via a specialized mapping. Merge them in so they
  1639         # resolve to the same column indices.
  1640         if specialized_mapping:
! 1641             column_map.update(specialized_mapping)
  1642 
  1643         # Route the map through the shared fetch path. The standard fetchone/
  1644         # fetchmany/fetchall methods read these cached maps when constructing Row
  1645         # objects, so metadata rows pick up the catalog column names without any


📋 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.row.py: 76.9%
mssql_python.__init__.py: 77.3%
mssql_python.pybind.connection.connection.cpp: 77.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.6%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@gargsaumya gargsaumya marked this pull request as ready for review June 10, 2026 12:26
Copilot AI review requested due to automatic review settings June 10, 2026 12:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors how catalog/metadata result sets set up column name mappings in mssql_python/cursor.py to avoid dynamically reassigning fetchone/fetchmany/fetchall on the cursor instance (which can confuse static type checkers like ty). It also adds a regression test to ensure fetch methods are never shadowed as instance attributes.

Changes:

  • Refactored _prepare_metadata_result_set to build/cache a column name → index map instead of wrapping/replacing fetch methods.
  • Removed now-obsolete “restore original fetch methods” logic from execute.
  • Added a regression test asserting fetch methods remain class methods (not instance attributes) across metadata helpers and normal execute.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
mssql_python/cursor.py Stops runtime reassignment of fetch methods by routing metadata column mappings through the shared cached map path.
tests/test_004_cursor.py Adds a regression test to prevent fetch methods being shadowed on the cursor instance (GH #620).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mssql_python/cursor.py Outdated
…ase=True (#620)

_prepare_metadata_result_set() hard-coded _cached_column_map_lower = None.
When lowercase=True the description column names are already lowercased, so
forcing the lowercase map to None dropped the Row case-insensitive fallback
and broke original-cased lookups (row.TABLE_NAME / row["TABLE_NAME"]) on
catalog result sets - a regression vs the previous wrapper-based code.

Build the lowercase map the same way execute() does (including any
specialized aliases). Also remove the now-dead self._column_map attribute
(nothing reads it; fetch methods use _cached_column_map) and add a
lowercase=True regression test for catalog column access.
Comment thread mssql_python/cursor.py
Comment thread tests/test_004_cursor.py Outdated
sumitmsft
sumitmsft previously approved these changes Jun 11, 2026

@sumitmsft sumitmsft left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved with minor changes

Comment thread mssql_python/cursor.py Outdated
subrata-ms
subrata-ms previously approved these changes Jun 11, 2026
cursor.py: _prepare_metadata_result_set() iterated self.description directly.
If DDBCSQLDescribeCol fails (swallowed above) and no fallback_description is
supplied (getTypeInfo() is the only such caller), description becomes None and
the loop raised TypeError. Iterate `self.description or ()` so the cursor yields
no rows instead. Not a pre-existing regression, but cheap to harden here.

test: the GH-620 shadowing test only compared row values (cursor.fetchall()
== [[1]]), which would not notice if the getTypeInfo() column-name cache leaked
into the following SELECT. Fetch the row and assert row.one == 1 and that
row.TYPE_NAME / row["TYPE_NAME"] raise, locking in the per-query cache rebuild.
@gargsaumya gargsaumya dismissed stale reviews from subrata-ms and sumitmsft via 846d7b8 June 11, 2026 10:00
@gargsaumya gargsaumya merged commit 9308c73 into main Jun 11, 2026
29 of 30 checks passed
@gargsaumya gargsaumya mentioned this pull request Jun 12, 2026
gargsaumya added a commit that referenced this pull request Jun 12, 2026
[AB#45750](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/45750)

Release mssql-python v1.9.0. Bumps `__version__`, `setup.py` version,
and updates `PyPI_Description.md` with this release's customer-facing
changes.

### Summary

#### Enhancements
- **Row Objects in Bulk Copy** - `bulkcopy` now accepts `Row` objects
(and lists) directly, converting each row to a tuple automatically
(#615).
- **Cached Parameter Type Resolution for NULLs** - `SQLDescribeParam`
results are cached per statement when binding `NULL` parameters,
removing redundant server round-trips and fixing incorrect type
fallbacks for all-NULL columns and VARBINARY types (#614).

#### Bug Fixes
- **macOS / Linux Import Failure** - simdutf is now always statically
linked via FetchContent, fixing import/symbol failures on machines
without simdutf at the CI build path (#608). Resolves #607, #628.
- **executemany Large Decimal Handling** - Fixed a `SQL_C_NUMERIC` type
mismatch when inserting `Decimal` values outside the SQL Server `MONEY`
range via `executemany` (#611). Resolves #609.
- **Exception Pickling** - DB-API exception subclasses and
`ConnectionStringParseError` now implement `__reduce__` for correct
pickle/unpickle round-trips (#616). Resolves #587.
- **PRINT Messages in nextset()** - Diagnostic messages from subsequent
result sets are captured on `SQL_SUCCESS_WITH_INFO` during `nextset()`
(#618). Resolves #612.
- **Row Objects in executemany DAE Path** - `executemany` converts `Row`
objects to tuples in the DAE fallback path, fixing `varchar(max)` writes
(#630). Resolves #629.
- **Static Type-Checking of Fetch Methods** -
`fetchone`/`fetchmany`/`fetchall` are no longer reassigned as instance
attributes, fixing type-checking under `ty` (#631). Resolves #620.

#### Version bump
- `mssql_python/__init__.py`: `__version__` 1.8.0 → 1.9.0
- `setup.py`: version 1.8.0 → 1.9.0
- `PyPI_Description.md`: updated "What's new" section to v1.9.0

Bundled `mssql_py_core` version unchanged (0.1.4).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: medium Moderate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants