FEAT: Add opt-in/opt-out ODBC provider selection (msodbcsql18 / mssql-odbc) - #730
FEAT: Add opt-in/opt-out ODBC provider selection (msodbcsql18 / mssql-odbc)#730gargsaumya wants to merge 10 commits into
Conversation
Resolve the ODBC provider from MSSQL_PYTHON_ODBC_PROVIDER env var, the mssql_python.odbc_provider module property, then a default (msodbcsql18). Selection resolves once and freezes at first connect; unknown values fail closed. The native loader imports the selected provider package and resolves a provider-specific driver path. Adds get_odbc_provider_info() diagnostics and unit tests.
There was a problem hiding this comment.
Pull request overview
Adds a process-wide, resolve-once ODBC provider selection mechanism so mssql-python can switch at runtime between the classic ODBC Driver 18 provider (msodbcsql18) and a future Rust provider (mssql-odbc), while keeping the existing public connection API unchanged.
Changes:
- Introduces
ProviderManagerto resolve provider selection (env var → module property → default), freeze it at first resolution, and fail closed on invalid selections. - Wires provider resolution into
Connection.__init__and pushes the selected provider into the native loader viaddbc_bindings.set_odbc_provider. - Adds a new unit test module covering precedence/normalization/freeze behavior and missing-provider fail-closed behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_026_odbc_provider.py | Adds unit tests for provider precedence, normalization, freeze semantics, and fail-closed behavior. |
| mssql_python/pybind/ddbc_bindings.cpp | Adds native-side provider selection plumbing and uses provider-specific package/dist names during driver resolution. |
| mssql_python/odbc_provider.py | Implements the Python-side provider selection engine (ProviderManager). |
| mssql_python/connection.py | Freezes/verifies provider selection and pushes it into the native layer before driver load. |
| mssql_python/init.py | Exposes mssql_python.odbc_provider and get_odbc_provider_info() as public diagnostics/surface area. |
💡 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/pybind/ddbc_bindings.cppLines 1083-1094 1083 if (e.matches(PyExc_ModuleNotFoundError)) {
1084 // Phase 2: the standalone package is required. Turn the missing
1085 // dependency into a clear, actionable error instead of a fallback.
1086 LOG("GetOdbcLibsBaseDir: required package %s is not installed (%s)",
! 1087 packageName.c_str(), e.what());
1088 ThrowStdException(
! 1089 "The required '" + distName + "' package (which ships the ODBC driver "
! 1090 "binaries) is not installed. Install it with: pip install " + distName);
1091 }
1092 // A different import-time error means the package is installed but
1093 // broken; surface it instead of silently masking the real problem.
1094 LOG("GetOdbcLibsBaseDir: importing %s failed unexpectedly (%s); "📋 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: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.6%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.__init__.py: 83.1%
mssql_python.pybind.connection.connection.cpp: 84.3%
mssql_python.logging.py: 85.5%
mssql_python.helpers.py: 89.3%🔗 Quick Links
|
…ype stubs - Remove the C++ env-var fallback (banned getenv/DevSkim finding); Python is already the sole authoritative resolver and pushes the selection via set_odbc_provider(). PoolingManager.enable() now also resolves+pushes so an explicit pooling() call before any connect still honors the selection. - Fix two GetOdbcLibsBaseDir log messages that hardcoded 'mssql_python_odbc' regardless of the selected provider. - Widen the public odbc_provider setter type hint to Optional[str] to match ProviderManager.set_property(). - Add odbc_provider and get_odbc_provider_info() to mssql_python.pyi (PEP 561 stubs). - Make the missing-provider test deterministic by patching import_module instead of relying on the package being absent.
Vahid (Vahid-b)
left a comment
There was a problem hiding this comment.
Summary
Adds a process-wide, resolve-once selector between msodbcsql18 and the Rust mssql-odbc driver, with msodbcsql18 as the Phase 1 default and the Rust path failing closed until its wheel ships. The design is sound — Python as the sole resolver, the native side purely a receiver, freeze at first connect, no public API change. One finding I'd treat as blocking; the rest are suggestions and nits.
I verified the load ordering the whole design rests on: loadDriver() is lazy behind std::call_once, and the only entry into it from a fresh process is the Connection constructor at pybind/connection/connection.cpp:25. connection.py:375-376 pushes the provider well before ddbc_bindings.Connection(...) at line 741. That part is correct.
All of copilot-pull-request-reviewer's findings and the DevSkim alert look addressed in e9372ba7 / 094dc2f3 / f854f8dc; I am not re-filing any of them. The one that is only half-closed is the Optional[str] stub, noted inline.
Blocking
The pooling hook (pooling.py:66-71) — its stated reason is false, and its only real effect is an unwanted early freeze. Detail inline. enable_pooling does not load the native driver, so the hook protects nothing, but it does make mssql_python.pooling() freeze the provider and become able to raise ImportError.
Suggestions
Six inline: the source field that is always None before the freeze, effective() raising on a bad env var, the native side's silent coercion plus unguarded set_odbc_provider, the mssql-auth.dll justification, the Linux path layout versus the libc split, and the Optional[str] stub.
One that has no line to sit on: no test covers the pooling freeze path. That is where the blocking finding lives, and a test asserting PoolingManager.enable() does not freeze the selection would have caught it. Worth adding alongside the fix.
Nits
Three inline: the mid-file #include, the recwarn assertion, and the resolve-before-validate ordering in Connection.__init__. Plus one that spans the change rather than a line: NormalizeProviderId (ddbc_bindings.cpp:993-1003) strips interior whitespace while Python's _normalize (odbc_provider.py:44-54) only does .strip(). Harmless while Python is the sole resolver, but the two should agree if the native one ever becomes authoritative.
What I ran
pytest isn't installed here and the package needs the compiled ddbc_bindings extension, so I could not run the suite — CI is the gate for that. The two runtime observations below came from loading odbc_provider.py standalone against a stubbed mssql_python.logging:
A. get_info BEFORE resolve, env=mssql-odbc: {'id': 'mssql-odbc', 'package': 'mssql_python_rust_odbc', 'source': None, 'frozen': False}
C. get_info AFTER resolve: {'id': 'mssql-odbc', 'package': 'mssql_python_rust_odbc', 'source': 'environment', 'frozen': True}
D. get_info with bad env RAISED: ValueError Unknown ODBC provider 'bogus-value'. ...
E. effective() with bad env RAISED: ValueError Unknown ODBC provider 'bogus-value'. ...
The cross-repo claims (mssql-auth.dll, artifact filenames, the Linux libc split) were checked against microsoft/mssql-rs source rather than from memory; file and line are cited in each comment.
Reviewed with GitHub Copilot on behalf of Vahid (@Vahid-b), then checked by hand. Not an approval — push back on anything that looks wrong.
| # ODBC provider first so an explicit pooling() before any connect | ||
| # still honors the selection (mirrors Connection.__init__). | ||
| _provider = ProviderManager.ensure_available() | ||
| ddbc_bindings.set_odbc_provider(_provider) |
There was a problem hiding this comment.
Blocking: the comment above is factually wrong, and the code has a side effect that works against the feature this PR adds.
enable_pooling does not load the native driver. It is ddbc_bindings.cpp:6068-6078 → ConnectionPoolManager::configure() (pybind/connection/connection_pool.cpp:486-493, pure state assignment) plus setAccepting(true) (:525-528). There is no loadDriver() call anywhere in pybind/connection/*.cpp except the Connection constructor at connection.cpp:25.
So the hook protects nothing, but it does two user-visible things:
mssql_python.pooling()now freezes the provider.ms.pooling(max_size=50)followed byms.odbc_provider = "mssql-odbc"silently becomes a no-op plus aRuntimeWarning. That is exactly the opt-in route this PR exists to add, andpooling()is a natural first line in a startup block — so the ordering trap is easy to hit and gives no error, just a warning.pooling()can now raiseImportError. A configuration call that previously could only raiseValueErroron bad parameters now fails when the selected provider's wheel is absent.
Deleting lines 69-70 loses nothing. On the auto-enable path, Connection.__init__ already pushed at connection.py:376 before reaching PoolingManager.enable() at line 737; on the explicit path, the next connect() pushes before the driver loads either way.
If you'd rather keep it as defence-in-depth against a future enable_pooling that does load the driver, use the non-freezing accessor:
ddbc_bindings.set_odbc_provider(ProviderManager.effective())Either way the comment needs correcting — as written it will stop the next reader from removing this.
| return { | ||
| "id": provider, | ||
| "package": _PACKAGE_BY_PROVIDER[provider], | ||
| "source": cls._source, |
There was a problem hiding this comment.
Suggestion: source is always None until the provider freezes, so the diagnostic is blank in exactly the window you'd call it.
_compute() returns the source, but get_info reads cls._source, which only resolve() ever assigns. Confirmed by running the module standalone with MSSQL_PYTHON_ODBC_PROVIDER=mssql-odbc set:
BEFORE resolve: {'id': 'mssql-odbc', 'package': 'mssql_python_rust_odbc', 'source': None, 'frozen': False}
AFTER resolve: {'id': 'mssql-odbc', 'package': 'mssql_python_rust_odbc', 'source': 'environment', 'frozen': True}
Reporting the id but not where it came from is the opposite of useful when someone is trying to work out why they're getting a provider they didn't expect — which is the pre-connect case.
if cls._resolved is not None:
provider, source = cls._resolved, cls._source
else:
provider, source = cls._compute()test_get_info_before_and_after_resolve (tests/test_026_odbc_provider.py:97-109) asserts id, package and frozen before resolve but skips source, which is why this passes today.
| with cls._lock: | ||
| if cls._resolved is not None: | ||
| return cls._resolved | ||
| provider, _ = cls._compute() |
There was a problem hiding this comment.
Suggestion: _compute() raises ValueError on an unrecognized env var, and that propagates out of effective() — so the diagnostics can't diagnose the one misconfiguration they exist for. Verified by running the module standalone with MSSQL_PYTHON_ODBC_PROVIDER=bogus-value:
get_info() RAISED: ValueError Unknown ODBC provider 'bogus-value'. Valid providers are: msodbcsql18, mssql-odbc.
effective() RAISED: ValueError Unknown ODBC provider 'bogus-value'. Valid providers are: msodbcsql18, mssql-odbc.
Failing closed at resolve() / ensure_available() is right and I'm not suggesting changing that. The problem is the read-only paths that share _compute():
get_odbc_provider_info()raises instead of reporting the bad value.effective()also backs the getter formssql_python.odbc_provider(__init__.py:605-613), so plain attribute access raises. And since"odbc_provider"is in__all__(__init__.py:528),from mssql_python import *would raiseValueErrorat import — before any connection is attempted, from a line that has nothing to do with providers.
Consider having the non-freezing paths report the raw invalid value (e.g. an error key in get_info, and the default from the getter) and keep the hard failure at resolve time, where it is actionable.
| // Effective provider id: the value pushed from Python, else the classic default. | ||
| // Python is the authoritative resolver (env var -> module property -> default) | ||
| // and pushes the result via set_odbc_provider() before the driver loads. | ||
| std::string GetSelectedProviderId() { |
There was a problem hiding this comment.
Suggestion: the native side silently coerces anything unrecognized to classic, which contradicts the Python layer's fail-closed contract.
GetSelectedProviderId() returns kProviderMsodbcsql18 for any value that isn't exactly mssql-odbc, and SetSelectedProvider above just overwrites the global with no validation and no check that the driver has already loaded. ProviderManager can't produce a bad value today, so this is defence-in-depth rather than a live bug — but m.def("set_odbc_provider", ...) at line 6108 is an unprefixed, public-looking native entry point, and anything that calls it directly bypasses both the validation and the freeze invariant the Python layer maintains. Silently loading a different driver than the caller asked for is the failure mode the Python side goes out of its way to avoid.
Two cheap options, not mutually exclusive: reject an unknown id in SetSelectedProvider (throw, mirroring _normalize), and rename the binding to _set_odbc_provider so it reads as internal plumbing. Making it a no-op or throw once DriverLoader has run would close the ordering hole too.
| authDllPath.string().c_str()); | ||
| ThrowStdException("mssql-auth.dll not found. If you are using Entra " | ||
| "ID, please ensure it is present."); | ||
| // mssql-auth.dll ships with the classic driver; the Rust provider does |
There was a problem hiding this comment.
Suggestion: the behavior is right but the stated reason isn't — the Rust driver does use mssql-auth.dll.
From microsoft/mssql-rs: mssql-odbc/src/auth/msqa.rs:63 is const MSQA_LIBRARY: &str = "mssql-auth.dll";, auth/interactive.rs:10 describes it as "a Windows-only library it loads with", and msqa.rs:212 documents "Loads mssql-auth.dll and resolves the entry points, once per process". It is required for ActiveDirectoryInteractive.
What actually differs is when and from where. msqa.rs:62 notes it is "The library msodbcsql loads from System32" — the Rust driver resolves it lazily through the normal DLL search path, whereas the classic driver needs it co-located at load time. So not treating its absence as fatal for the Rust provider is correct.
Two things worth changing: restate the comment as "loaded lazily from the system search path, so it is not a load-time dependency for this provider", and note the consequence — a user on mssql-odbc doing interactive Entra auth will now get a failure from inside the Rust driver rather than this clear load-time message.
| // finalized alongside that wheel build. | ||
| if (GetSelectedProviderId() == kProviderMssqlOdbc) { | ||
| #ifdef __linux__ | ||
| return (basePath / "libs" / "linux" / arch / "lib" / "mssql-odbc.so").string(); |
There was a problem hiding this comment.
Suggestion: the Rust Linux path drops the distro segment the classic path has, and there's a known reason it will be needed.
Classic Linux resolves to libs/linux/<platform>/<arch>/lib/... with platform one of alpine / rhel / suse / debian_ubuntu (lines 1205-1230). macOS and Windows in this new block correctly mirror their classic shapes; only Linux is flattened to libs/linux/<arch>/lib/.
That matters because microsoft/mssql-rs is building two libc flavours per architecture — see mssql-rs #394, "Build both Linux libc flavours per architecture on one agent". A flat libs/linux/<arch>/ has nowhere to put both, so the musl and glibc artifacts would collide. Since the completeness check in GetOdbcLibsBaseDir does fs::exists() on exactly this path, whatever the wheel ships has to match it.
I realise the description calls this layout "finalized alongside that wheel build", so this is a deliberate open question rather than an oversight — but the libc split is a requirement today, not a future one, and it is cheaper to settle before the wheel ships than after.
Separately, the filenames here match mssql-rs #397 ("Ship mssql-odbc driver artifacts with product filenames"), which is still open. Worth naming that dependency in the Deferred list alongside the others.
| # Module 设置 - Properties that can be get/set at module level | ||
| lowercase: bool # Controls column name case behavior | ||
| native_uuid: bool # Controls UUID type handling | ||
| odbc_provider: str # Selects the ODBC provider ('msodbcsql18' or 'mssql-odbc') |
There was a problem hiding this comment.
Suggestion: this only half-closes the earlier review point about Optional.
ProviderManager.set_property() and the module setter at __init__.py:614-617 both accept Optional[str] (documented as "or None to clear"), so mssql_python.odbc_provider = None works at runtime but is a type error for anyone consuming the stubs. A module-level .pyi variable can't express asymmetric get/set types, so it has to be one or the other: either annotate Optional[str] here, or drop None support from the public setter.
I'd lean toward dropping it — clearing the selection has no test, and given the freeze semantics there's a narrow window in which it does anything at all.
| // not read the environment itself; if the push has not happened yet, it falls | ||
| // back to the hardcoded classic default. | ||
| // ----------------------------------------------------------------------------- | ||
| #include <cctype> |
There was a problem hiding this comment.
Nit: #include <cctype> sits ~985 lines into the file rather than with the other includes at the top. It works, but it's the only one down here.
| def test_same_value_after_freeze_does_not_warn(recwarn): | ||
| ProviderManager.resolve() | ||
| ProviderManager.set_property(PROVIDER_MSODBCSQL18) | ||
| assert len(recwarn) == 0 |
There was a problem hiding this comment.
Nit: this asserts no warnings at all, not "no RuntimeWarning". Any unrelated DeprecationWarning raised during the call would fail it. pytest.warns(None) is deprecated, so the targeted form is:
assert not [w for w in recwarn if issubclass(w.category, RuntimeWarning)]| # Resolve and freeze the ODBC provider before the native driver loads, | ||
| # then hand the selection to the native loader so it imports the matching | ||
| # provider package. | ||
| _provider = ProviderManager.ensure_available() |
There was a problem hiding this comment.
Nit: this resolves and freezes the provider before the native_uuid type validation just below, so a call that is about to raise TypeError still freezes the selection as a side effect. Moving the two lines after the argument validation is free and keeps the freeze tied to a connection attempt that actually proceeds.
Saurabh Singh (saurabh500)
left a comment
There was a problem hiding this comment.
The native load-order issue below blocks the Rust opt-in. I also agree with the existing PoolingManager.enable() thread: native enable_pooling() only configures pool state, so it should not freeze provider selection when the documented boundary is the first connection. I did not duplicate that thread.
The authentication comment below reflects the clarified requirement that the Rust provider also requires mssql-auth.dll for its Windows interactive-authentication path.
| # Resolve and freeze the ODBC provider before the native driver loads, | ||
| # then hand the selection to the native loader so it imports the matching | ||
| # provider package. | ||
| _provider = ProviderManager.ensure_available() |
There was a problem hiding this comment.
Blocking: this provider resolution happens after the native extension has already attempted to load the classic driver. Importing mssql_python loads the extension, whose PYBIND11_MODULE calls DriverLoader::loadDriver() at ddbc_bindings.cpp:6302. That call is protected by std::call_once, so it permanently records either the classic driver handle or the classic load error before this line can push mssql-odbc.
I reproduced this by selecting mssql-odbc in a fresh process with stand-in provider packages; connect() still surfaced the mssql-python-odbc completeness error. Please remove the import-time loadDriver() block and rely on the existing lazy load from the native connection path, so this push really runs first. A subprocess test should select Rust with incomplete stand-in packages and assert that the native error names mssql-python-rust-odbc, not the classic distribution.
| package = _PACKAGE_BY_PROVIDER[provider] | ||
| try: | ||
| importlib.import_module(package) | ||
| except ImportError as exc: |
There was a problem hiding this comment.
Suggestion: this rewrites every ImportError as “the provider package is not installed.” If the package is present but its initialization or a transitive dependency fails, the actionable underlying error is hidden behind an incorrect installation hint.
Catch ModuleNotFoundError and translate it only when exc.name == package; otherwise re-raise the original exception. The test double should construct ModuleNotFoundError(..., name=name) so it matches real import behavior.
except ModuleNotFoundError as exc:
if exc.name != package:
raise
dist = _DIST_BY_PROVIDER[provider]
...| if (GetSelectedProviderId() != kProviderMssqlOdbc) { | ||
| ThrowStdException("mssql-auth.dll not found. If you are using Entra " | ||
| "ID, please ensure it is present."); | ||
| } |
There was a problem hiding this comment.
Suggestion: the Rust provider also requires mssql-auth.dll for its Windows interactive-authentication path. This exemption is also unreachable in the missing-file case because GetOdbcLibsBaseDir() already rejects every Windows provider whose driver directory lacks the DLL at lines 1064–1068.
If the provider wheel is required to include mssql-auth.dll, keep that completeness check, remove this Rust-specific exception, and update the PR description/comment that currently calls the DLL nonfatal.
| if (GetSelectedProviderId() != kProviderMssqlOdbc) { | |
| ThrowStdException("mssql-auth.dll not found. If you are using Entra " | |
| "ID, please ensure it is present."); | |
| } | |
| ThrowStdException("mssql-auth.dll not found. If you are using Entra " | |
| "ID, please ensure it is present."); |
Linked work item: AB#47445
Summary
Introduces runtime selection between the classic Microsoft ODBC Driver 18 (
msodbcsql18) and the Rust ODBC driver (mssql-odbc), without changing the public API or install command.msodbcsql18remains the default (Phase 1); the Rust driver is opt-in and fails closed until its provider wheel ships.Motivation
Design-review feedback on the Rust ODBC provider integration: hardcode the two providers and switch on an env var + module property, and use customer-friendly provider strings
msodbcsql18/mssql-odbc(not "classic").What's implemented
mssql_python/odbc_provider.py,ProviderManager): precedenceMSSQL_PYTHON_ODBC_PROVIDERenv var ->mssql_python.odbc_providermodule property -> default (msodbcsql18); values normalized and validated; unknown value fails closed (no fallback).RuntimeWarning(connection-pool model).mssql_python.odbc_providerproperty +get_odbc_provider_info()diagnostic.connection.py,pooling.py):Connection.__init__andPoolingManager.enable()both resolve + verify the provider (ensure_available()), then push the selection to native (ddbc_bindings.set_odbc_provider) before the driver loads, so an explicitpooling()call before anyconnect()still honors the selection.pybind/ddbc_bindings.cpp): Python is the sole resolver; the native side only reads the value pushed viaset_odbc_provider(), falling back to the hardcoded classic default if nothing was pushed yet (no environment variable read in C++).GetOdbcLibsBaseDirimports the selected provider's package;GetDriverPathCpphas a Rust branch (mssql-odbc.dll/.so/.dylib, nolibprefix, under the mssql-python-ownedlibs/layout); Windowsmssql-auth.dllis non-fatal for the Rust provider. Classic path is unchanged.Behavior
mssql-odbcselected with its package absent -> clear fail-closed error: "...package 'mssql_python_rust_odbc' is not installed. Install it with: pip install mssql-python-rust-odbc." No dummy wheels required.Testing
tests/test_026_odbc_provider.py— 17 unit tests (precedence, normalization, fail-closed, freeze, post-freeze warning, diagnostics, public surface).mssql-odbcfail-closed behavior end-to-end.Deferred (tracked separately)
mssql-python-rust-odbcwheel pipeline -> PyPI dependency.mssql-odbc.