Repository navigation
feat(sqlserver): support client certificates on the ODBC connection string - #6
Merged
Merged
Conversation
… without a driver Every backend __init__ imported all of its submodules eagerly, and .backend imports the DB driver. Python executes a parent package's __init__ before any submodule, so `impl.sqlserver.expression` could not be imported without the driver present - even though no expression class touches it. Auditing the expression namespace needed a working driver; pyodbc additionally needs the OS libodbc.so, so a plain pip install was not enough and sqlserver dropped out of the audit entirely. Public names are now resolved on first access through a PEP 562 module __getattr__, with the name -> (module, attribute) map derived from the existing import statements. The exported surface is unchanged: attribute access, __all__, dir() and `from impl.sqlserver import *` all behave as before, and the previous eager imports move into an `if TYPE_CHECKING:` block so mypy and IDEs still resolve them. The existing __getattr__ is preserved as _optional_dependency_getattr, so the async backends keep raising an install hint for aioodbc instead of a bare ImportError. Importing the package, or its .expression submodule, no longer loads the driver; the driver error now surfaces at first use of the name that needs it, where it is actionable. 55 public names verified to resolve.
Drop the top-level impl/<backend>/__init__.py so the directory becomes a PEP 420 namespace portion and no longer re-exports every public name. Each name is now imported from the sub-package that defines it, so a symbol has one public path instead of two. The facade also made "expression is importable on its own" unenforceable: importing impl.<backend>.expression executed the parent __init__, which pulled in the driver. With the file gone, the sub-packages stand alone by construction rather than by a lazy __getattr__ that each backend had to maintain separately. This is a breaking change for anyone importing `from rhosocial.activerecord.backend.impl.<backend> import X`; the replacement is `...impl.<backend>.<module> import X`.
The provider gave the worker tasks a single module path and the task code looked for the async counterpart one level up. That was enough while the sync and async classes shared the impl.<backend> facade; with the facade gone they live in different modules and the one-level lookup fell through to the sync class, so an async worker would silently run the sync backend. Walk up the chain first, then try the backend's own async_backend module. No package scanning is involved: both candidates are named explicitly.
Move the sync and async classes into a backend/ package with the fixed layout: backend/backend.py and backend/async_backend.py. Eight backends kept them as two modules beside backend/, and sqlite and postgres used backend/sync.py, so the class locations differed three ways between backends. With the layout fixed the worker tasks derive the async class from the sync module's own package instead of searching for it, so the _find_async_backend helper is gone.
The move into backend/ shifted every relative import by one level, and the function-local ones were missed, so impl.bigquery could not reach its adapters module. Resolve each target against the filesystem instead of counting dots.
impl.<backend>.backend already re-exported the sync class, so leaving the async one out made the two classes reachable by different rules. Export both. The eight backends whose async driver is a hard requirement import it eagerly. SQLite and SQL Server keep a PEP 562 __getattr__, because aiosqlite and aioodbc ship as the [async] extra and importing either eagerly would make the async class a requirement for sync-only users.
The CLI tests monkeypatched SQLServerBackend on impl.sqlserver, which no longer carries the name now that the directory is a namespace. Patch impl.sqlserver.backend instead, and the async class on impl.sqlserver.backend.async_backend, which is where each is defined.
The remaining multi-line monkeypatch still targeted impl.sqlserver.backend for AsyncSQLServerBackend, but the CLI imports it from impl.sqlserver.backend.async_backend inside the handler, so the patch never reached it and the real backend was constructed.
The two CLI demos embed runnable Python as a string literal, and the import
migration rewrote the module path inside it without keeping the statement
terminator, leaving the string unterminated:
"from rhosocial.activerecord.backend.impl.oracle.backend.async_backend import OracleBackend
ast.parse rejects all four demo modules with "unterminated string literal", so
they could not even be imported, let alone run. The migration also dropped the
trailing OracleConnectionConfig from the original single-statement import; the
surrounding lines already import it from its own module, so restoring the ";"
recovers the intended statements.
The SQL Server demos additionally reached SQLServerConnectionConfig through the
impl.sqlserver facade, which no longer exists now that the package is open, so
they now name impl.sqlserver.config like every other backend.
Verified: ast.parse is clean across every src/ and tests/ module in both repos.
mixins.py and protocols.py each sat next to a package of the same name, holding nothing but a re-export of that package's contents. A regular package takes precedence over a same-named module, so both shims were unreachable: importing impl.sqlserver.mixins or impl.sqlserver.protocols always resolved to the package's __init__.py. Each re-export list is a strict subset of the package's, so nothing that resolved through a shim can break. Also corrects the path comment on mixins/ddl_table.py, which still named the file before it was renamed from table.py.
Removed the __partition__ model attribute from the partitioning example: no such attribute exists, and table partitioning is in fact unsupported by this backend -- add_partition, attach_partition, detach_partition and the three partitioning switches are all False at every probed version. The generated version table is what surfaces that. Corrected the core dependency floor, documented as >=1.0.0 and unresolvable against the 1.0.0.devN pre-release line. In the agent docs: replaced the invented DatabaseCapabilities / _initialize_capabilities / requires_capability system with the actual supports_* protocol switches, and corrected the PYTHONPATH guidance, which omitted tests.
…tring encrypt and trust_server_certificate only cover server-side encryption, so a client certificate could not be presented at all. Add client_certificate, client_key, client_key_password and host_name_in_certificate to SQLServerConnectionConfig and emit them as the ClientCertificate/ClientKey/ClientKeyPassword/HostNameInCertificate ODBC keywords from build_connection_string(), with the same four names accepted as backend kwargs. validate_config() now rejects a client_key given without a client_certificate, so the mismatch surfaces as a clear ValueError instead of an opaque driver failure at connect time.
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/v1.0.0.dev3 #6 +/- ##
=======================================================
- Coverage 73.36% 73.36% -0.01%
=======================================================
Files 133 132 -1
Lines 7332 7313 -19
Branches 1171 1171
=======================================================
- Hits 5379 5365 -14
+ Misses 1548 1540 -8
- Partials 405 408 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Replace the six divergent changelog-check variants with one implementation
that encodes the documented merge rules.
feature/*|fix/* -> release/*|maint/*|main:
- the PR must add at least one changelog.d/<number>.<type>.md
- every added fragment's <number> must equal the PR number
- fragments already present from previously merged branches are ignored,
since only files ADDED by this PR are considered
- escape hatches ([trivial] title, skip-changelog label, docs: update
CHANGELOG) waive only the "at least one" requirement; the
number-must-match rule is always enforced on fragments that were added
release/* -> main:
- CHANGELOG.md must have changed (content is not inspected)
- changelog.d/ must hold no fragments at the merge commit
- no escape hatches apply to this path
The number-must-match rule was the one documented requirement that none of
the previous variants implemented. Two latent bugs are also fixed: the
paths-ignore of "**.md" meant a CHANGELOG.md-only push never ran the check at
all, and three backends skipped release merges outright instead of applying
the release rules.
A feature/fix branch merges into a release or maintenance branch; only release/* merges into main. Opening a second PR at main from the same branch is a duplicate, and it also breaks the changelog fragment, which is numbered for exactly one PR. This pattern already cost a wasted PR in six repositories (a ->release PR merged alongside a ->main PR that was then closed), and the fragment numbering check only exposed it indirectly, by failing the leftover PR. hotfix/* -> main stays allowed; the version control policy calls for hotfixes to be merged back to main and the active release branch.
|
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.
Description
Fixes # (issue)
SQLServerConnectionConfigcould only express server-side encryption, viaencryptandtrust_server_certificate. There was no way to present a clientcertificate: no
ClientCertificate,ClientKeyorHostNameInCertificateODBCkeyword was ever emitted, and no config field existed to drive them.
This is a capability gap rather than a silently-dropped setting, but it sits in the
same area as the coordinated TLS fixes in the other backends on this branch.
Worth contrasting with the other four:
encryptdefaults toTrue, so SQL Serverconnections were already encrypted and the server certificate was already checked
(
TrustServerCertificate=no). Nothing was silently downgraded here — mutual TLS wassimply not expressible.
Reproduction Steps
SQLServerConnectionConfigand callbuild_connection_string().Expected: some way to supply one.
Actual: none exists — the four new fields are the only route, and any
options={"ClientCertificate": ...}workaround bypasses validation entirely.Type of change
fix: A bug fixtest: Adding tests for the fixBreaking Change
Applicable
rhosocial-activerecordVersion Range:>=1.0.0,<2.0.0All four fields default to unset, so existing connection strings are unchanged.
validate_config()gains one new check: aclient_keysupplied without aclient_certificatenow raisesValueErrorinstead of reaching the driver, where itwould surface as an opaque connect-time failure.
Related Repositories/Packages
This is one of five coordinated fixes. The same class of defect — SSL parameters
collected on the config but never handed to the driver — was found and fixed
alongside in
python-activerecord-postgres,python-activerecord-mysql,python-activerecord-mariadbandpython-activerecord-oracle. No sharedpython-activerecordchange is required; each backend translates the genericSSLMixinnames to its own driver's keywords.Testing
Test Plan:
New
tests/rhosocial/activerecord_sqlserver_test/feature/backend/test_ssl_connection_params.pyasserts the new fields are emitted as the correct ODBC keywords, that no client-TLS
keyword appears when they are unset, that
encrypt/trust_server_certificatebehaviour is unchanged, and that the cert-without-key case raises.
Note: the SQL Server test suite's session-scoped conftest fixture probes every
configured scenario, so running it locally without a reachable server blocks on
connection timeouts. The tests here are offline and pass in 1.8s when the scenario
file is pointed elsewhere.
Checklist
README.md, etc.).changelog.d/<issue_number>.<type>.md). — will be added aschangelog.d/<pr_number>.added.mdonce CI passes; this is new configuration surface rather than a fix, soaddedis the appropriate type.Additional Notes
This branch also carries accumulated work that predates this change: the
impl.sqlserver.backendpackage reorganisation, removal of two legacy re-exportshims, lazy backend exports, and the README/
.claudedocumentation rewrite(12 commits ahead of
release/v1.0.0.dev3in total).The commit for this change is titled
feat(sqlserver):rather thanfix(...),which is why the changelog type is
addedrather thanfixed.