FEAT: Integrating async connection with py-core - #765
FEAT: Integrating async connection with py-core#765Subrata (subrata-ms) wants to merge 18 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
AsyncConnection.close() is currently non-idempotent (unlike the existing sync Connection.close() contract) and the new tests/behavior should be reconciled to avoid repeated-close failures with real native connections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a new mssql_python.AsyncQuery subpackage that exposes an AsyncConnection wrapper backed by the native mssql_py_core extension, plus a translation layer that maps py-core DB-API exceptions into the public mssql_python.exceptions hierarchy.
Changes:
- Introduces
AsyncConnectionwith async connect/transaction/context-management methods, and centralizes py-core import validation via_native.load_py_core(). - Adds
exception_translatorto translate py-core DB-API exception types to the public exception classes (including cause chaining via a context manager). - Adds a dedicated
tests/AsyncTest/suite covering py-core loading, async connection delegation, exception translation, and logging redaction/boundaries.
File summaries
| File | Description |
|---|---|
| mssql_python/AsyncQuery/init.py | Exposes the new async-query public surface (connection, exceptions, loader). |
| mssql_python/AsyncQuery/_native.py | Implements the py-core import boundary and required-type validation. |
| mssql_python/AsyncQuery/async_connection.py | Adds the public AsyncConnection wrapper and lifecycle logging. |
| mssql_python/AsyncQuery/exception_translator.py | Adds translation utilities + context manager for py-core exceptions. |
| tests/AsyncTest/test_001_async_query_native.py | Tests _native.load_py_core() behavior and required-type enforcement. |
| tests/AsyncTest/test_002_async_connection.py | Tests AsyncConnection delegation, context manager behavior, and close semantics. |
| tests/AsyncTest/test_003_async_exceptions.py | Tests exception translation mapping, diagnostics preservation, and chaining. |
| tests/AsyncTest/test_004_async_logging.py | Tests logging boundary behavior (no secret context/error detail leakage). |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/async_query/_connection_context.pyLines 9-17 9
10 def build_async_connection_context(connection_str: str, timeout: int) -> dict[str, Any]:
11 """Parse and validate a public connection string for py-core."""
12 if not isinstance(connection_str, str):
! 13 raise TypeError("connection_str must be a string")
14 if "\x00" in connection_str:
15 raise InterfaceError(
16 driver_error="Connection string must not contain a NUL (\\x00) character.",
17 ddbc_error="Embedded NUL in connection string.",Lines 18-26 18 )
19 if isinstance(timeout, bool) or not isinstance(timeout, int):
20 raise TypeError("Login timeout must be an integer")
21 if timeout < 0:
! 22 raise ValueError("Login timeout cannot be negative")
23
24 parser = _ConnectionStringParser(validate_keywords=True)
25 params = parser._parse(connection_str)
26 if not any(params.get(key) for key in ("server", "addr", "address")):mssql_python/async_query/_native.pyLines 7-16 7 def load_py_core() -> ModuleType:
8 """Load the PyO3 extension that owns asynchronous TDS operations."""
9 try:
10 py_core = import_module("mssql_py_core")
! 11 except ImportError as exc:
! 12 raise ImportError(
13 "Async query support requires the bundled mssql_py_core extension."
14 ) from exc
15
16 required_types = ("PyAsyncConnection", "PyAsyncCursor")mssql_python/async_query/exception_translator.pyLines 39-47 39 if error_type.__module__ != "mssql_py_core":
40 continue
41 public_type = _EXCEPTION_TYPES.get(error_type.__name__)
42 if public_type is None:
! 43 continue
44
45 logger.debug(
46 "Async exception translation: %s -> %s",
47 error_type.__name__,mssql_python/helpers.pyLines 336-345 336
337 if strict:
338 unsupported = sorted(set(params) - set(_PYCORE_CONNECTION_KEY_MAP))
339 if unsupported:
! 340 names = ", ".join(unsupported)
! 341 raise ValueError(
342 f"Connection parameters are not supported for mssql-py-core connections: {names}"
343 )
344
345 return pycore_params📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 85.5%
mssql_python.async_query._native.py: 85.7%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a mixed-case public package name (mssql_python.AsyncQuery) that is inconsistent with existing package naming and can create avoidable portability/import-footgun risk across platforms.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
mssql_python/AsyncQuery/init.py:4
- The public package name
mssql_python.AsyncQueryuses mixed case, which is inconsistent with the rest ofmssql_python(all-lowercase modules/packages) and with PEP 8 package naming. Mixed-case package names can also be a portability footgun because import paths become sensitive to filesystem case behavior across platforms. Consider renaming the package directory to a lowercase name (e.g.,async_query) and updating imports/tests accordingly; if you need to preserve the existing path, add a small compatibility shim that re-exports from the lowercase package.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The current async API exposes raw native cursor objects and the exception translation semantics risk surfacing sensitive native messages via driver_error, both of which should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new public async API layer tightly coupled to a native extension boundary, which warrants final human review for compatibility and long-term API/support implications.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
It introduces inconsistent and potentially confusing error semantics/documentation in shared connection-string handling (see stored PR comments for concrete fixes).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
mssql_python/helpers.py:305
- connstr_to_pycore_params() is documented as a bulk-copy helper, but strict-mode errors currently hardcode an async-specific message. Since this function lives in shared helpers (and is also used by bulkcopy), the error text and docs should be consistent and context-neutral to avoid confusing callers and future reuse.
def connstr_to_pycore_params(params: dict, *, strict: bool = False) -> dict:
"""Translate parsed ODBC connection-string params for py-core's bulk copy path.
When ``cursor.bulkcopy()`` is called, mssql-python opens a *separate*
connection through mssql-py-core.
py-core's ``connection.rs`` expects a Python dict with snake_case keys —
different from the ODBC-style keys that ``_ConnectionStringParser._parse``
returns.
This function bridges that gap: it maps lowercase ODBC keys (e.g.
``"trustservercertificate"``) to py-core keys (``"trust_server_certificate"``)
and converts numeric strings to ``int`` for timeout/size params.
Boolean params (TrustServerCertificate, MultiSubnetFailover) are passed as
strings — ``connection.rs`` validates Yes/No and rejects invalid values.
Unrecognised keys are silently dropped unless ``strict`` is enabled. Strict
mode also rejects invalid integer values instead of using py-core defaults.
"""
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
New tests include credential-bearing connection strings with non-localhost server names (against repo guidance), and one strict-mode error message is misleading given the helper’s broader usage.
Review details
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
mssql_python/helpers.py:345
- connstr_to_pycore_params() is used by both the bulk copy path (cursor.py) and the new async-query path, but the strict-mode error message hard-codes "async queries". This can be confusing when the helper is reused elsewhere; make the message generic (or move async-specific wording up into the async context builder).
tests/AsyncTest/test_002_async_connection.py:78 - This test uses a connection string containing UID/PWD with a non-localhost server name. Repo guidance requires committed credential-bearing connection strings to use localhost (or 127.0.0.1) with dummy values; please change the host to localhost here.
This issue also appears in the following locations of the same file:
- line 92
- line 142
tests/AsyncTest/test_004_async_logging.py:54
- Unit tests include a connection string with UID/PWD where the server host is not localhost. Repo guidance requires committed connection strings containing credentials to use SERVER=localhost (or 127.0.0.1) with dummy values, even in tests, to avoid accidental leakage patterns.
tests/AsyncTest/test_002_async_connection.py:99
- After updating the test connection string host to localhost, the expected captured context should also assert server='localhost' to match the new input.
"context": {
"server": "test-server.example.invalid",
"database": "db;name",
"user_name": "test-user",
"password": "p}ass;word",
"encryption": "Yes",
"connect_timeout": 12,
},
tests/AsyncTest/test_002_async_connection.py:145
- This test uses a credential-bearing connection string (UID/PWD) with a non-localhost server. Update it to localhost (or 127.0.0.1) per repo guidance, and adjust the expected context accordingly.
context = build_async_connection_context(
"Server=test-server.example.invalid;Authentication=SqlPassword;UID=user;PWD=password",
0,
)
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
New tests commit UID/PWD connection strings with non-local servers (violating repo credential-scanning guidance), and a shared helper’s strict-mode error message is async-specific.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tests/AsyncTest/test_002_async_connection.py:152
- This test also commits a UID/PWD connection string with a non-local server. For consistency with repo guidance, switch this example to Server=localhost (or 127.0.0.1).
context = build_async_connection_context(
"Server=test-server.example.invalid;Authentication=SqlPassword;UID=user;PWD=password",
0,
)
assert context == {
"server": "test-server.example.invalid",
"authentication": "SqlPassword",
"user_name": "user",
"password": "password",
}
tests/AsyncTest/test_002_async_connection.py:93
- After switching the test connection string to use localhost (per repo guidance for committed UID/PWD examples), update the expected parsed context to match.
"server": "test-server.example.invalid",
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A newly added async connection test has an input/expectation mismatch that will fail and needs correction before the PR can be safely approved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
mssql_python/async_query/init.py:1
- Prior review thread indicates this async_query surface is not intended to be public yet, but this package-level docstring/exports read like a supported public API. If it is still experimental, consider marking it explicitly here (and aligning the PR description/import path) to avoid accidental support commitments.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There’s a docstring inconsistency in connstr_to_pycore_params() that misstates its scope now that it’s used by the new async path.
Review details
Suppressed comments (1)
mssql_python/helpers.py:296
- The connstr_to_pycore_params() docstring now claims it’s only used by the bulk copy path (cursor.bulkcopy), but this helper is also used by the new async_query connection context builder. This can mislead future maintainers about the function’s scope and the meaning of strict mode in this context.
When ``cursor.bulkcopy()`` is called, mssql-python opens a *separate*
connection through mssql-py-core.
py-core's ``connection.rs`` expects a Python dict with snake_case keys —
different from the ODBC-style keys that ``_ConnectionStringParser._parse``
returns.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new async connection stack backed by a native extension, and should receive a final human review for API/stability expectations and native-integration correctness.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
mssql_python/async_query/async_connection.py:1
- The module docstring calls this a "Public" async connection, but earlier context indicates the async API is still under development/not yet public; leaving "Public" in user-facing docstrings can mislead readers about stability/support level. Consider removing "Public" or explicitly marking the API as experimental.
mssql_python/async_query/async_cursor.py:1 - The module docstring calls this a "Public" async cursor; if the async_query surface is not yet considered public/stable, this wording can misrepresent the intended support level. Consider removing "Public" or marking it experimental to match the project’s current stance.
mssql_python/async_query/exception_translator.py:38 - The docstring says this returns the equivalent public exception or the original non-py-core error, but the function also returns original mssql_py_core exceptions when their type name is not in _EXCEPTION_TYPES. Updating the docstring to reflect the actual behavior will avoid confusion for future maintenance.
mssql_python/helpers.py:305 - This comment says unknown/reserved keys are silently dropped, but in strict=True mode the function raises ValueError for unsupported keys. Updating the comment to mention strict-mode behavior will keep the inline documentation consistent with the implementation.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
PR #765: Add real native async integration coverage.
The new tests validate wrapper delegation using fake native connection and cursor objects, but none exercises the packaged mssql_py_core PyO3/Tokio bridge.
Please add focused integration coverage for a real async connection and query, independent-connection concurrency and event-loop responsiveness, timeout or failure cleanup, and connection/cursor close behavior. Wrapper-only tests cannot detect blocking or native lifecycle and ownership failures at the boundary introduced by PR #765.
| raise ValueError("SERVER parameter is required in connection string") | ||
|
|
||
| authentication = params.get("authentication", "").strip().lower() | ||
| if authentication and authentication != "sqlpassword": |
There was a problem hiding this comment.
PR #765: Preserve supported authentication behavior for async connections.
The connection-string parsing and internal py-core translation now follow the expected pattern, but this branch rejects every Authentication= mode except SqlPassword.
That means an existing mssql-python connection string using a supported Entra authentication mode cannot move to AsyncConnection without changing authentication behavior.
Please implement a genuinely asynchronous token-acquisition path and internally translate the resulting access token or token factory for py-core. If Entra support is intentionally deferred, please keep this API explicitly private or preview-only and document the limitation until the public async connection contract is complete.
Work Item / Issue Reference
Summary
This pull request introduces a new asynchronous query interface for SQL Server, backed directly by the
mssql-py-corenative extension. It provides a thin Python wrapper (AsyncConnection) for asynchronous operations, robust exception translation to the public DB-API, and comprehensive logging. The changes also include thorough unit tests for native integration, error translation, and logging boundaries.New asynchronous query interface:
AsyncConnection, a public async connection class that wraps the nativemssql_py_core.PyAsyncConnectionand exposes async methods for connecting, transaction management, and context management, with logging at key lifecycle points.load_py_corein_native.pyto dynamically import the native extension and validate required async types, enforcing a strict dependency boundary.Exception translation and error handling:
exception_translator.pyto map nativemssql_py_coreexceptions to the public DB-API exception hierarchy, preserving diagnostics and error chaining.Testing and validation:
Public API and module organization:
mssql_python.AsyncQuerypackage’s__init__.pyfor convenient imports.