You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This pull request refactors the asynchronous query layer to improve clarity, error handling, and code organization. The main changes include renaming internal variables for clarity, extracting statement execution and result fetching logic into dedicated modules, enhancing exception translation, and updating tests to reflect improved error handling. These updates make the async API more robust and maintainable.
Refactoring and Code Organization:
Renamed internal attributes from native_connection/native_cursor to py_core_async_connection/py_core_async_cursor in both AsyncConnection and AsyncCursor for improved clarity and future maintainability. (mssql_python/async_query/async_connection.py, mssql_python/async_query/async_cursor.py) [1][2]
Extracted statement execution and result fetching logic from AsyncCursor into new helper modules: async_execute.py and async_fetch.py, leading to cleaner, more modular code. (mssql_python/async_query/async_execute.py, mssql_python/async_query/async_fetch.py, mssql_python/async_query/async_cursor.py) [1][2][3]
Error Handling Improvements:
Enhanced exception translation in exception_translator.py to classify and translate lower-level driver errors into appropriate public exceptions (e.g., OperationalError, ProgrammingError), and updated the async connection tests to verify these translations. (mssql_python/async_query/exception_translator.py, tests/AsyncTest/test_002_async_connection.py) [1][2][3][4]
Behavioral and API Improvements:
Improved fetch tracking in AsyncCursor, including accurate rowcount reporting after fetch operations and resetting fetch state on nextset. (mssql_python/async_query/async_cursor.py) [1][2]
Updated column name handling in AsyncCursor.description to support automatic lowercasing based on settings, and ensured that row wrapping consistently produces Row objects with proper metadata. (mssql_python/async_query/async_cursor.py, mssql_python/async_query/async_fetch.py) [1][2]
Testing:
Updated and expanded tests to cover the improved exception translation and internal API changes, ensuring correct error propagation and behavior after connection closure. (tests/AsyncTest/test_002_async_connection.py, tests/AsyncTest/test_003_async_exceptions.py) [1][2][3]
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unresolved moderate and critical findings remain in result-set state, parameter binding, fetch handling, and credential-redaction coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors the async query API by separating execution and fetching helpers, improving error translation and row handling, and expanding test coverage.
Changes:
Extracts async execution and fetch operations into dedicated modules.
Improves exception translation, logging, row wrapping, and fetch tracking.
Expands tests for execution, fetching, errors, cursor behavior, and logging.
File summaries
File
Reviewed changes
Final review findings
tests/AsyncTest/test_007_async_fetch.py
Adds fetch, row, type, and navigation coverage.
—
tests/AsyncTest/test_006_async_execute.py
Adds execute and executemany coverage.
—
tests/AsyncTest/test_005_async_cursor.py
Updates cursor property and lifecycle tests.
—
tests/AsyncTest/test_004_async_logging.py
Adds operation logging coverage.
Critical (2 votes): Preserve broad credential-redaction assertions or verify known secrets are absent.
tests/AsyncTest/test_003_async_exceptions.py
Expands exception translation coverage.
—
tests/AsyncTest/test_002_async_connection.py
Updates connection error and lifecycle tests.
—
mssql_python/async_query/exception_translator.py
Classifies and translates async errors.
—
mssql_python/async_query/async_fetch.py
Implements fetching, row wrapping, and metadata handling.
Moderate (1 vote): Snapshot native_uuid; short-circuit non-positive fetch sizes. Nit (3 votes): Cache row metadata maps instead of rebuilding them per row.
mssql_python/async_query/async_execute.py
Implements async execution helpers.
Moderate (2 votes): Normalize Row parameter sequences like the synchronous path.
mssql_python/async_query/async_cursor.py
Delegates operations and tracks fetch state.
Moderate (2 votes): Snapshot lowercase/UUID settings and row metadata per result set.
mssql_python/async_query/async_connection.py
Renames connection internals and integrates logging.
—
Review details
Suppressed comments (2)
mssql_python/async_query/async_fetch.py:62
The wrapper passes a non-positive size through to py-core. The synchronous cursor short-circuits size <= 0 before its native call (mssql_python/cursor.py:2842-2843), and this PR's test requires fetchmany(-1) to return []; a Rust binding using an unsigned size can reject -1 instead. Short-circuit requested_size <= 0 before calling py-core.
with translate_py_core_exceptions():
if size is None:
rows = await _get_py_core_async_cursor(cursor).fetchmany()
else:
rows = await _get_py_core_async_cursor(cursor).fetchmany(size)
mssql_python/async_query/async_fetch.py:30
This consults the mutable global native_uuid at fetch time, so changing it between execute() and fetch*() changes the representation of an already-executed result. The synchronous cursor snapshots UUID conversion indices at execute (mssql_python/cursor.py:1404-1423) and tests cover this (tests/test_004_cursor.py:16785-16815); store the setting/result-set metadata when execute or nextset completes.
uuid_str_indices = (
tuple(index for index, column in enumerate(description) if column[1] is uuid.UUID)
if not get_settings().native_uuid
else None
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unresolved compatibility, correctness, and performance findings remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
mssql_python/async_query/async_cursor.py:107
This property applies the global lowercase setting on every access, so changing it after execute() changes the metadata and row key casing for an already-established result set. The synchronous cursor snapshots this at execution (tests/test_004_cursor.py:3592-3608); retain the effective casing with the async result metadata instead.
if description is None:
return None
lowercase = get_settings().lowercase
return [
((column[0].lower() if lowercase else column[0]), *column[1:]) for column in description
mssql_python/async_query/async_execute.py:27
The new async path only unwraps tuple/list parameters, but the added test_execute_accepts_dbapi_row passes a Row and expects its columns to bind as individual parameters. The synchronous implementation explicitly normalizes Row to a tuple because the downstream binder otherwise treats the whole row as one value (mssql_python/cursor.py:1739-1747); apply the same normalization before forwarding to PyAsyncCursor.execute, or that new async contract will fail.
cursor._reset_fetch_tracking() # pyright: ignore[reportPrivateUsage]
if len(parameters) == 1 and isinstance(parameters[0], (tuple, list)):
parameters = tuple(parameters[0])
mssql_python/async_query/async_fetch.py:30
native_uuid is read while each row is wrapped, so toggling the module setting after execute() but before fetch*() changes the type of rows from one result set. The synchronous cursor snapshots this setting at execution (tests/test_004_cursor.py:16336-16357); cache the UUID conversion indices when async result metadata is established.
uuid_str_indices = (
tuple(index for index, column in enumerate(description) if column[1] is uuid.UUID)
if not get_settings().native_uuid
else None
mssql_python/async_query/async_fetch.py:23
_wrap_row rebuilds the column map and, when enabled, the lowercase map for every returned row. fetchmany() and fetchall() therefore repeat O(column_count) metadata work for every row; the synchronous cursor precomputes these maps once per result set, so cache and reuse them for large async result sets.
def _wrap_row(cursor: "AsyncCursor", values: tuple[Any, ...]) -> Row:
description = cursor.description or ()
column_map = {column[0]: index for index, column in enumerate(description)}
column_map_lower = (
{name.lower(): index for name, index in column_map.items()}
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
A critical test indentation error blocks collection, and fetch behavior, performance, and documentation require fixes.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
mssql_python/async_query/async_fetch.py:24
_wrap_row is called for every row by fetchone, fetchmany, and fetchall, but this rebuilds the description-derived maps for every row, including a fresh cursor.description list and dictionaries. That makes large result sets pay O(rows × columns) Python metadata work and can make one result set change representation if lowercase/native_uuid is changed between fetches; the synchronous cursor caches these maps/settings once per result set (mssql_python/cursor.py:1838-1848). Cache the row metadata when the result set is established and reuse it here.
description = cursor.description or ()
column_map = {column[0]: index for index, column in enumerate(description)}
column_map_lower = (
{name.lower(): index for name, index in column_map.items()}
if get_settings().lowercase
mssql_python/async_query/async_fetch.py:60
Non-positive sizes are still passed to the native cursor. The synchronous cursor returns immediately for size <= 0 (mssql_python/cursor.py:2839-2843), and the new async test expects fetchmany(-1) to return []; passing -1 to a native usize-like parameter can instead raise before the wrapper's tracking guard runs.
requested_size = cursor.arraysize if size is None else size
logger.debug("AsyncCursor.fetchmany: starting; requested_size=%s", requested_size)
with translate_py_core_exceptions():
if size is None:
rows = await _get_py_core_async_cursor(cursor).fetchmany()
The new fallback at this line translates allowlisted built-in RuntimeError and TypeError instances, but the surrounding docstrings still say non-py-core errors are returned unchanged and that the context manager translates only py-core failures. Please update that documented contract so callers are not misled about which native Python errors are wrapped.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Five moderate issues remain unresolved.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
mssql_python/async_query/async_fetch.py:62
The synchronous cursor returns [] immediately for size <= 0 (mssql_python/cursor.py:2839-2843), but this path passes zero and negative sizes into mssql_py_core. That makes the new fetchmany(0/-1) contract depend on backend validation and can raise instead of returning the expected empty list; short-circuit non-positive sizes before the native call.
mssql_python/async_query/async_cursor.py:87
After a fetch sets _fetch_rowcount, rowcount bypasses the native cursor on every later read. Because close() does not clear that cache, a fetched cursor still returns its old count after closing instead of following the native closed-cursor behavior used by the other properties and operations. Clear fetch tracking when close succeeds before logging completion.
await self._py_core_async_cursor.close()
mssql_python/async_query/async_cursor.py:62
This removes the existing public use_prepare keyword from AsyncCursor.executemany; callers that used the previous signature now fail with TypeError, and the native helper no longer receives the flag. Retain and forward the keyword (as execute still does), or deliberately version/document this breaking API change.
This unwraps only tuples/lists, so a Row passed as the sole parameter is forwarded to PyAsyncCursor.execute as one bound value. The new test_execute_accepts_dbapi_row expects the row's two values to bind to two ? markers; mirror the synchronous cursor's Row-to-tuple normalization (mssql_python/cursor.py:1742-1747) before calling the native API.
if len(parameters) == 1 and isinstance(parameters[0], (tuple, list)):
parameters = tuple(parameters[0])
mssql_python/async_query/async_fetch.py:30
_wrap_row runs once for every returned row, but each invocation rereads description, rebuilds both column maps, and rescans all columns for UUIDs. Large fetchall()/fetchmany() results therefore repeat O(column_count) metadata work and allocate a map per row; cache these result-set maps on AsyncCursor when execution/nextset() changes and reuse them here.
description = cursor.description or ()
column_map = {column[0]: index for index, column in enumerate(description)}
column_map_lower = (
{name.lower(): index for name, index in column_map.items()}
if get_settings().lowercase
else None
)
uuid_str_indices = (
tuple(index for index, column in enumerate(description) if column[1] is uuid.UUID)
if not get_settings().native_uuid
else None
All five moderate issues were valid and are now resolved:
fetchmany(size <= 0) returns [] without calling py-core, while preserving closed-cursor errors.
Successful close() clears cached fetch rowcount.
executemany(..., use_prepare=...) is restored and forwarded.
A sole DB-API Row is normalized to positional parameters.
Row metadata/maps are cached once per result set and refreshed on nextset().
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Five moderate findings remain unresolved.
Review details
Suppressed comments (5)
mssql_python/async_query/async_execute.py:65
len(seq_of_parameters) is evaluated before the cursor's closed state is checked. A closed cursor passed an iterator (or another object without len) therefore raises a raw TypeError instead of the ProgrammingError expected for closed-cursor operations and used by the synchronous cursor, which checks closure before inspecting parameters. Check the cursor before computing batch_count.
Because this branch only checks the wrapper's _closed flag, it never consults the native cursor or connection. After AsyncConnection.close() the existing AsyncCursor wrapper is not marked closed, so await cursor.fetchmany(0) returns [] while the other cursor operations surface the closed-connection error. Keep cursor lifecycle state synchronized with connection close or perform an equivalent closed-state check before returning here.
For a closed cursor, fetchmany("invalid") reaches requested_size <= 0 before _check_closed() and leaks Python's comparison TypeError rather than translating the operation to ProgrammingError. The synchronous cursor checks closure before validating the size, so perform the closed check before reading arraysize/evaluating the requested size.
requested_size = cursor.arraysize if size is None else size
logger.debug("AsyncCursor.fetchmany: starting; requested_size=%s", requested_size)
if requested_size <= 0:
cursor._check_closed() # pyright: ignore[reportPrivateUsage]
The translation table only maps the exact RuntimeError("Cursor is closed") case. Fetching from an open cursor before any result set is another py-core built-in runtime error, so fetchone/fetchall/fetchmany can still escape as RuntimeError even though the new test_fetch_without_result_set_raises_programming_error expects ProgrammingError. Add the py-core no-result-set message/prefix (or expose it as a native ProgrammingError) here.
_PROGRAMMING_RUNTIME_ERRORS = ("Cursor is closed",)
tests/AsyncTest/test_004_async_logging.py:53
This changes the redaction regression guard from rejecting any password text to rejecting only the password= spelling. Since this PR now forwards the default logger to py-core, a diagnostic formatted as password: ... or another non-= form could include a credential and still pass; assert the actual secret is absent or cover all sensitive-key formats instead.
Change SQL query in test_execute_binds_representative_sync_parameter_types to select value instead of throwing an error.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Fetch cancellation can leave stale result state, same-schema nextset transitions are misdetected, and one added test currently fails due to an incomplete fake cursor.
Get a fresh assessment by requesting another Copilot review.
asyncio.CancelledError is a BaseException, so cancellation of an in-flight fetchone bypasses this reconciliation path. If the native fetch consumes a row before cancellation is delivered, the wrapper keeps the old generation/count and the next successful fetch under-reports rowcount; if native state was discarded, stale metadata remains published. Handle cancellation through the same best-effort reconciliation used by execute/nextset, then re-raise it.
This issue also appears in the following locations of the same file:
line 103
line 123
Fake cursor missing rowcount attribute
tests/AsyncTest/test_007_async_fetch.py:18
This fake cursor has no rowcount, but async_fetch.fetchone() always evaluates cursor.rowcount for its completion log after fetching. The test therefore raises AttributeError at that point instead of reaching either equality assertion. Give the fake the same rowcount attribute that every other fetch fake in this file defines.
Handle cancellation during fetchone reconciliation
mssql_python/async_query/async_fetch.py:71
asyncio.CancelledError is not an Exception on supported Python versions, so cancellation while the native fetchone() is awaited bypasses reconciliation. The native fetch may already have consumed or invalidated the result while the wrapper continues exposing stale metadata and rowcount. Catch CancelledError alongside Exception, reconcile, and re-raise, as execute() and nextset() already do.
This issue also appears in the following locations of the same file:
Use result identity to preserve cursor state after failed execution
mssql_python/async_query/async_execute.py:30
Comparing only description and rowcount cannot tell whether execution preserved the old result. If a dispatched execute installs a new result with the same schema/rowcount and is then cancelled or raises, this branch treats it as unchanged, so the previous generation and fetched-row count survive; the first fetch from the new result can then report (for example) rowcount 2 instead of 1. Track native result identity/generation, or distinguish pre-dispatch validation failures from failures after execution may have changed the cursor, rather than using value equality as the preservation signal.
Reconcile cursor state when cancellation interrupts close
mssql_python/async_query/async_cursor.py:184
Cancellation can leave a closed native cursor looking live. If the native close() changes its state and then the task is cancelled, execution skips _closed = True and both cache resets, so this wrapper still exposes the old description/fetched rowcount; fetchmany(0) can even return [] without touching the now-closed native cursor. Handle CancelledError (and other potentially state-changing close failures) like the execute/nextset reconciliation paths, while preserving state only for known non-mutating rejections such as the busy error.
SQL Server error 2714 (“object already exists”) still falls through to generic DatabaseError, although the public ProgrammingError contract includes already-existing tables and the synchronous SQLSTATE mapper classifies 42S01 that way (mssql_python/exceptions.py:348-350). Add 2714 so async execution reports the same public exception.
Ensure deterministic row order before testing divide-by-zero
tests/AsyncTest/test_007_async_fetch.py:607
This test assumes the n = 1 row is delivered before the divide-by-zero row, but SQL Server does not guarantee row order without ORDER BY. It can encounter n = 0 during fetchone(), so the test fails before reaching the fetchall() assertion it is meant to exercise. Add an explicit ordinal and order by it.
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
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
This pull request refactors the asynchronous query layer to improve clarity, error handling, and code organization. The main changes include renaming internal variables for clarity, extracting statement execution and result fetching logic into dedicated modules, enhancing exception translation, and updating tests to reflect improved error handling. These updates make the async API more robust and maintainable.
Refactoring and Code Organization:
native_connection/native_cursortopy_core_async_connection/py_core_async_cursorin bothAsyncConnectionandAsyncCursorfor improved clarity and future maintainability. (mssql_python/async_query/async_connection.py,mssql_python/async_query/async_cursor.py) [1] [2]AsyncCursorinto new helper modules:async_execute.pyandasync_fetch.py, leading to cleaner, more modular code. (mssql_python/async_query/async_execute.py,mssql_python/async_query/async_fetch.py,mssql_python/async_query/async_cursor.py) [1] [2] [3]Error Handling Improvements:
exception_translator.pyto classify and translate lower-level driver errors into appropriate public exceptions (e.g.,OperationalError,ProgrammingError), and updated the async connection tests to verify these translations. (mssql_python/async_query/exception_translator.py,tests/AsyncTest/test_002_async_connection.py) [1] [2] [3] [4]Behavioral and API Improvements:
AsyncCursor, including accuraterowcountreporting after fetch operations and resetting fetch state onnextset. (mssql_python/async_query/async_cursor.py) [1] [2]AsyncCursor.descriptionto support automatic lowercasing based on settings, and ensured that row wrapping consistently producesRowobjects with proper metadata. (mssql_python/async_query/async_cursor.py,mssql_python/async_query/async_fetch.py) [1] [2]Testing:
tests/AsyncTest/test_002_async_connection.py,tests/AsyncTest/test_003_async_exceptions.py) [1] [2] [3]