Repository navigation
Conversation
There was a problem hiding this comment.
Summary and verdict: One blocking compatibility issue remains: the change threads access-token auth correctly, but it makes a public Rust struct harder to construct from downstream code. No other correctness issue stood out in the reviewed paths.
Verification: reviewed head 7b522cbba3c9540ff9c63af0a3ef8d3573dff8ed, merge-base 5e688e0cbda61fbdf3597735533e81a72b7d0b90, with gh pr view 993 --repo sfu-db/connector-x --json ..., gh api repos/sfu-db/connector-x/pulls/993/reviews, gh pr checks 993 --repo sfu-db/connector-x (no checks reported), git diff --check $(git merge-base origin/main HEAD)..HEAD (clean), and started ~/.local/bin/cx-check static / ~/.local/bin/cx-check python (both still running when the report was finalized, then stopped to end the run). Not verified: completed cx-check static and cx-check python output, and live Entra ID auth against SQL Server.
Findings
Blocking
[R2/Blocking] connectorx/src/source_router.rs:25-32 — SourceConn— Cited. Adding the privateaccess_tokenfield changes the layout of a public struct, so downstream Rust code that still constructsSourceConn { ty, conn, proto }will stop compiling. Keep the public struct layout stable in this release, or move auth state behind a separate wrapper / non-public type that does not alterSourceConn.
Suggestion
- None.
Nit
- None.
Severity count
| Round | Blocking | Suggestion | Nit | Total |
|---|---|---|---|---|
| R1 | 0 | 0 | 0 | 0 |
| R2 | 1 | 0 | 0 | 1 |
| Total | 1 | 0 | 0 | 1 |
|
Re: the What's true: adding any field (private or public) to Why it isn't blocking:
The PR description's Compatibility section lists this. Happy to change approach if maintainers would rather not touch the struct. |
|
@saurabh500 this is ready for review. It follows your guidance on #569: programmatic \�ccess_token\ only (no JWT in the connection string), and mssql-tds only (Tiberius rejects tokens with a clear error). Could you also approve the CI workflow run for this fork PR? Thanks! |
|
@Vahid-b thanks for the PR. I have recommended some API changes after reading the code. |
…ions Address review feedback on sfu-db#993: - Keep `SourceConn` and the existing entry points unchanged. Backend-specific configuration now travels through a typed `SourceOptions` enum (`Default` | `MsSql(MsSqlOptions)`) and additive entry points: `get_arrow_with_options`, `new_record_batch_iter_with_options`, `partition_with_options`, `get_col_range_with_options` and `MsSQLSource::new_with_options`. The shared layer only routes options and rejects ones that don't match the source type. - `MsSqlOptions` lives in the MSSQL backend, which owns token validation: non-empty token, conflicting URL credentials / trusted_connection (extracted into `validate_access_token_auth`), and driver support. - Python: `cx.MsSqlOptions(access_token=...)` passed as `source_options=` to `read_sql`, `read_sql_pandas`, `get_meta` and `partition_sql`; its repr redacts the token, and other values raise TypeError. - Tiberius error now reads "Entra ID access token support is not implemented for Tiberius". - Tests use randomly generated, non-JWT-shaped placeholder tokens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
e4f3921 to
81f86cb
Compare
…ions Address review feedback on sfu-db#993: - Keep `SourceConn` and the existing entry points unchanged. Backend-specific configuration now travels through a typed `SourceOptions` enum (`Default` | `MsSql(MsSqlOptions)`) and additive entry points: `get_arrow_with_options`, `new_record_batch_iter_with_options`, `partition_with_options`, `get_col_range_with_options` and `MsSQLSource::new_with_options`. The shared layer only routes options and rejects ones that don't match the source type. - `MsSqlOptions` lives in the MSSQL backend, which owns token validation: non-empty token, conflicting URL credentials / trusted_connection (extracted into `validate_access_token_auth`), and driver support. - Python: `cx.MsSqlOptions(access_token=...)` passed as `source_options=` to `read_sql`, `read_sql_pandas`, `get_meta` and `partition_sql`; its repr redacts the token, and other values raise TypeError. - Tiberius error now reads "Entra ID access token support is not implemented for Tiberius". - Tests use randomly generated, non-JWT-shaped placeholder tokens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@saurabh500 your review comments are addressed and all four threads are resolved. Could you take another look?
(Also correcting my earlier comment, where formatting got garbled: the original argument was |
Add a programmatic `access_token` argument to `read_sql`, `read_sql_pandas`, `get_meta` and `partition_sql` that authenticates SQL Server connections with a Microsoft Entra ID (Azure AD) access token. The token is never read from the connection string. - SourceConn carries the token in a private field (redacted from Debug) and only accepts it for mssql:// sources. - mssql-tds: uses TdsAuthenticationMethod::AccessToken for the main, metadata and partition-range connections; combining a token with URL credentials or trusted_connection=true is an error. - Tiberius: rejects tokens with a clear error (no new Tiberius support). - Rust: MsSQLSource::new_with_access_token; MsSQLSource::new is unchanged. Closes sfu-db#569 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ions Address review feedback on sfu-db#993: - Keep `SourceConn` and the existing entry points unchanged. Backend-specific configuration now travels through a typed `SourceOptions` enum (`Default` | `MsSql(MsSqlOptions)`) and additive entry points: `get_arrow_with_options`, `new_record_batch_iter_with_options`, `partition_with_options`, `get_col_range_with_options` and `MsSQLSource::new_with_options`. The shared layer only routes options and rejects ones that don't match the source type. - `MsSqlOptions` lives in the MSSQL backend, which owns token validation: non-empty token, conflicting URL credentials / trusted_connection (extracted into `validate_access_token_auth`), and driver support. - Python: `cx.MsSqlOptions(access_token=...)` passed as `source_options=` to `read_sql`, `read_sql_pandas`, `get_meta` and `partition_sql`; its repr redacts the token, and other values raise TypeError. - Tiberius error now reads "Entra ID access token support is not implemented for Tiberius". - Tests use randomly generated, non-JWT-shaped placeholder tokens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keeps the PR at zero new missing_docs warnings after rebasing onto sfu-db#986. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tions arg After rebasing onto sfu-db#992, its new unit tests call build_client_context with the old one-argument signature. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
a6394f0 to
ff027eb
Compare
Summary
Adds Microsoft Entra ID (Azure AD) access-token authentication for SQL Server through a typed, programmatic options object, as agreed in #569 and refined in review: the token never goes in the connection string, and only the mssql-tds driver supports it.
Problem
ConnectorX could only authenticate to SQL Server with SQL logins or integrated auth, so Azure SQL databases that require Entra ID could not be read. Users fell back to pyodbc with
SQL_COPT_SS_ACCESS_TOKEN.Fix
connectorx::source_options::SourceOptions, a#[non_exhaustive]enum:Default|MsSql(MsSqlOptions).get_arrow_with_options,new_record_batch_iter_with_options,partition_with_options,get_col_range_with_optionsandMsSQLSource::new_with_options. The existing functions delegate withSourceOptions::default().check_source_typerejects options that don't match the connection's database.MsSqlOptionshas private fields, awith_access_tokenbuilder and a redactedDebug.validate()rejects empty tokens.validate_access_token_authrejects a token combined with URL credentials ortrusted_connection=true; only then does it setTdsAuthenticationMethod::AccessToken.get_meta) and partition-range discovery.Entra ID access token support is not implemented for Tiberius, on the partition-range path too, instead of silently using URL credentials.cx.MsSqlOptions(access_token=...)is a frozen pyclass with a keyword-only argument; itsreprredacts the token and an empty token raisesValueError.source_options=toread_sql(all return types),read_sql_pandas,get_metaandpartition_sql.TypeError; federated queries rejectsource_options.Compatibility
SourceConn(same layout asmain),get_arrow,new_record_batch_iter,partition,get_col_rangeandMsSQLSource::newkeep their signatures and behaviour.source_options=Nonearguments and a newMsSqlOptionsclass.Cargo.tomlor lockfile changes.Scope
ActiveDirectoryDefault, managed identity, service principal, …) via mssql-tds'sEntraIdTokenFactory. They could become additionalMsSqlOptionsfields later.tds_impl::MsSQLSourcePartition::newdoes not take options. Partitions created byMsSQLSourceshare the source's authenticated pool.Timed out waiting for an MsSQL connection, hiding the server'sLogin failedmessage. Worth a separate issue.Validation
Run in WSL Ubuntu 24.04, which mirrors CI.
cargo test --lib --features <combo>,dst_arrow,src_sqlite -- options access_token source_router configuration_tests partition:src_mssql_tds: 35 passedsrc_mssql_tiberius: 24 passedsrc_mssql_tiberius,src_mssql_tds: 36 passedjust test-python -k "token or options": 18 passed. These cover:get_metaandpartition_sql;partition_on, and the conflict checks;TypeErrorfor untyped options, non-MSSQL sources and federated queries.No database is needed: each case fails before any network I/O.
End-to-end probe against the SQL Server 2025 testcontainer, with a random token and no URL credentials: the server performs FedAuth and replies
Login failed for user '', with no fallback to SQL login.Full local suites:
cx-check static(fmt,cargo clippy --features all,just test-feature-gate,cargo doc): OK. Clippy stays at the 146-warning baseline.*_with_optionsfunctions carry#[allow(clippy::result_large_err)], the same lint the existing entry points trigger because ofConnectorXOutError's size.cx-check rust(just test-ci): OK, including the MSSQL suites for both drivers.cx-check python(mssql-tds, then tiberius, then the runtime switch): OK, 249 passed / 29 skipped / 3 xfailed.Pre-existing failures:
partition::testsfails withoutsrc_sqliteonmainas well.Docs:
docs/databases/mssql.md(section "Microsoft Entra ID access token") anddocs/api.md.Closes #569
This PR was prepared with AI assistance (GitHub Copilot) and reviewed/validated locally.