tds/odbc: unified execute + statement-wise result navigation API (msodbcsql parity) - #143
tds/odbc: unified execute + statement-wise result navigation API (msodbcsql parity)#143David-Engel wants to merge 19 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds statement-wise result navigation for ODBC parity while preserving existing result-set behavior for other consumers.
Changes:
- Adds
StatementResult-based TDS navigation. - Maps no-row results through ODBC execution, fetch, and
SQLMoreResults. - Updates parity-focused end-to-end tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
mssql-tds/src/connection/tds_client.rs |
Implements statement-boundary navigation and tests. |
mssql-odbc/src/api/exec_direct.rs |
Enables navigation for plain batches. |
mssql-odbc/src/api/exec_common.rs |
Preserves pending no-row results. |
mssql-odbc/src/api/more_results.rs |
Maps statement results to ODBC returns. |
mssql-odbc/src/api/fetch.rs |
Rejects fetches from zero-column results. |
mssql-odbc/tests/e2e/tests/exec_direct_test.cpp |
Tests separate informational statements. |
mssql-odbc/tests/e2e/tests/more_results_test.cpp |
Tests navigation through PRINT results. |
…t; clarify docs Address PR #143 review feedback and CI test failures: - fetch: the no-row (0-column) guard returned SQLSTATE 24000 too early, before the busy-with-other-statement (HY000) and already-drained (SQL_NO_DATA) checks, breaking two mssql-odbc unit tests. Move the guard into fetch_rows_next after those checks, where active_stmt == self is established; restore the client so the connection stays busy. Fixes fetch_busy_with_other_statement_returns_hy000 and fetch_after_cursor_drained_returns_no_data. - tds_client: add move_to_next_statement_end_after_draining_final_rowset, a timeout-bounded regression test for the post-drain has_open_batch re-check (the reported hang). Enhance the mock transport's receive_row_into to replay queued tokens so drain paths are testable. - docs: correct advance_to_result_boundary / execute_multi_statement / StatementResult doc comments to state the count-or-message predicate; pure no-op statements (e.g. bare CREATE TABLE) are collapsed and never surface as NoRows. All mssql-tds statement tests, mssql-odbc fetch tests, and e2e (14/14, parity) pass; clippy + fmt clean.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-odbc/src/api/exec_common.rsmssql-odbc/src/api/exec_direct.rsmssql-odbc/src/api/execute.rsmssql-odbc/src/api/fetch.rsmssql-odbc/src/api/free_handle.rsmssql-odbc/src/api/more_results.rsmssql-py-core/src/cursor.rsmssql-tds/src/connection/tds_client.rsmssql-tds/src/test_client_support.rs🔗 Quick Links |
Adds an msodbcsql-aligned navigation model where each statement in a batch
that returns rows, carries a row count (DONE COUNT flag), or produces
messages (PRINT / low-severity RAISERROR) is individually navigable, instead
of collapsing all no-row statements into the next row-returning result set
the way execute()/move_to_next() do. Pure no-op statements (e.g. CREATE with
no count and no messages) are still collapsed, matching msodbcsql — verified
directly: `CREATE; INSERT; SELECT` exposes the INSERT's row count and the
SELECT, not the bare CREATE.
New public API on TdsClient:
- enum StatementResult { RowSet, NoRows { rows_affected }, End }
- execute_multi_statement(): runs a batch, positions on its first result.
- move_to_next_statement(): advances to the next statement's result.
Internals: move_to_column_metadata() is refactored onto a shared
advance_to_result_boundary(expose_norow) helper; with expose=false it keeps
the exact existing (collapsing) behavior used by execute()/move_to_next()
and the JS/Python consumers. The batch-send prologue is factored into
send_query_batch(). No existing caller changes.
Foundation for aligning mssql-odbc SQLMoreResults with msodbcsql. Unit tests
cover PRINT/RAISERROR (each surfaced), a single no-row statement, the
no-op-collapse + row-count-surface rule, and the no-open-batch case.
Wire the ODBC layer to mssql-tds's additive statement-wise navigation so each no-row statement (PRINT, low-severity RAISERROR, DML with rowcount) is individually navigable via SQLMoreResults instead of collapsing into the next row-returning result set. - tds_client: has_open_batch() accessor; move_to_next_statement re-checks has_open_batch after draining so a batch whose final DONE was consumed by the row reader returns End instead of blocking on an empty wire. - exec_direct: plain batch uses execute_multi_statement. - exec_common::finish_execute: expose no-row statement result, keep connection busy/cursor positioned when the batch has more statements. - more_results: map StatementResult (RowSet/NoRows/End). - fetch: 0-column no-row result returns SQLSTATE 24000. - e2e: exec_direct/more_results tests updated for statement-wise behavior. Parity: 14 parity, 0 bugs vs msodbcsql.
…t; clarify docs Address PR #143 review feedback and CI test failures: - fetch: the no-row (0-column) guard returned SQLSTATE 24000 too early, before the busy-with-other-statement (HY000) and already-drained (SQL_NO_DATA) checks, breaking two mssql-odbc unit tests. Move the guard into fetch_rows_next after those checks, where active_stmt == self is established; restore the client so the connection stays busy. Fixes fetch_busy_with_other_statement_returns_hy000 and fetch_after_cursor_drained_returns_no_data. - tds_client: add move_to_next_statement_end_after_draining_final_rowset, a timeout-bounded regression test for the post-drain has_open_batch re-check (the reported hang). Enhance the mock transport's receive_row_into to replay queued tokens so drain paths are testable. - docs: correct advance_to_result_boundary / execute_multi_statement / StatementResult doc comments to state the count-or-message predicate; pure no-op statements (e.g. bare CREATE TABLE) are collapsed and never surface as NoRows. All mssql-tds statement tests, mssql-odbc fetch tests, and e2e (14/14, parity) pass; clippy + fmt clean.
Raise diff coverage of the statement-wise navigation changes by adding Rust unit tests for the client-driven ODBC paths that were previously only exercised by the C++ e2e suite (which does not feed llvm-cov). - mssql-tds: add `test_client_support` (gated behind the existing `test-util` feature) exposing `tds_client_from_tokens` plus opaque token constructors. A token-replay transport drives a real TdsClient through statement-wise navigation without a live server. - mssql-odbc: add `test-util` dev-dependency and unit tests injecting a scripted client into DbcState to cover SQLMoreResults (RowSet / NoRows / End / no-active-client), SQLFetch's no-row 24000 guard, and SQLExecDirect's finish_execute no-row branch. No production behavior change.
71786a3 to
e3a5ecc
Compare
…; exclude test scaffolding from coverage - exec_direct: add a no-row-with-message test covering the finish_execute SQL_SUCCESS_WITH_INFO branch. - more_results: add a transport-failure test covering the SQLMoreResults error arm (cursor reset + connection released). - CI: exclude test_client_support.rs (test-only, test-util-gated scaffolding) from the lcov/cobertura coverage reports so diff-cover measures product code only. Diff coverage of the statement-wise changes is ~94%.
Redesign the mssql-tds result-navigation and execute* surface into one
lossless, statement-wise primitive, per the agreed design:
- StatementResult { Rows, NoRows { rows_affected: Option<u64> }, End } and a
single `execute(sql, opts) -> StatementResult` + `advance() -> StatementResult`,
with `advance_to_rows() -> bool` as the collapse convenience and `on_rows()`.
- ExecuteOptions { timeout, cancel, column_encryption } (Default/builder/From<()>)
threaded through every execute* method; deletes the
execute_sp_executesql_with_encryption_setting twin.
- Row-producing execute* (execute, sp_executesql, stored_procedure, sp_prepexec,
sp_execute) now return StatementResult; sp_prepare -> i32, sp_unprepare -> ().
- Removes ResultSetClient (get_current_resultset/move_to_next),
execute_multi_statement/move_to_next_statement, move_to_column_metadata.
- Migrates all consumers: mssql-odbc, mssql-js (query_raw/fetch_chunk restructured,
DML-only close wart gone), mssql-py-core, mssql-tds-cli, benches, and the
integration test suite.
Robustness fix (surfaced by the migration): get_next_row_into's error path now
clears has_open_batch so close_query/advance no longer block on a drained stream
after a mid-row server error.
Passing: mssql-tds lib 1550, mssql-odbc lib 333, mock 13, query_results,
info_messages, cursor_ops, tvp; clippy + fmt clean. A few behavioral integration
tests (rpc_results, and SQL-setup-gated suites) still need updates for the
statement-wise semantics.
…ntics Statement-wise execute() leaves the batch positioned on a DML row count or a stored proc's no-row result (has_open_batch stays true); callers that then run another operation or read output params must drain to End. Update rpc_results tests (sp_executesql seed insert; stored-proc output params) to advance_to_rows after the DML/proc, matching how the ODBC/JS/py-core layers already drain.
The unified statement-wise execute made execute_sp_execute/execute_sp_prepexec return the DML's row count as a NoRows result with the batch still positioned (has_open_batch true), so finish_execute left a 0-column cursor open. A prepared statement runs a single statement, so SQLExecute of a DML must leave it idle and immediately re-executable (msodbcsql parity) — re-executing hit SQLSTATE 24000. Drain non-Rows results to End in the SQLExecute path before finish_execute, restoring the pre-statement-wise collapse behavior for prepared statements. The row-returning path is unchanged (cursor kept for SQLFetch; sp_prepexec @handle RETURNVALUE captured at drain time). e2e --compare-with-msodbcsql: 14 parity, 0 bugs (was 13 parity / 1 bug: PrepareExecuteLiveTest.PreparedParamDmlReusesHandle).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 66 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
mssql-js/src/connection.rs:157
- A procedure that first emits a row count is now left on
NoRows. The unchangedcreateResultFast()stops when its firstfetchChunk()returnsNone, so it neither reaches later row sets nor drains the stream beforeget_return_values()is called; result sets and output parameters can both be missing. Collapse to the first row set before returning to the TypeScript consumer.
let result = client
.execute_stored_procedure(stored_proc_name, None, Some(named_params), ())
.await;
… doc links Address PR #143 review feedback: - mssql-py-core cursor.execute and mssql-js execute/executeWithParams/executeProc now advance to the first row-returning result set after the unified (statement- wise) execute, matching query_raw. Without this, a leading no-row statement (e.g. UPDATE ...; SELECT ...) left the client off on_rows() and the consumer's fetch path returned no rows, dropping the SELECT. - Repoint doc links that referenced the removed execute_multi_statement / move_to_next_statement / ResultSetClient APIs to the unified execute / advance / ResultSet surface, including the mssql-tds crate-level quick-start doctest (now compiles) and the newly test-util-exported test_client_support module.
|
Looks good. Would be good to make sure we're covering most edge cases etc in out e2e/unit tests |
The cargo-fuzz crate (mssql-tds/fuzz) is built by CI's Build stage via 'cargo +nightly fuzz build' but is outside the default workspace, so the unified execute-API migration and local 'cargo build/clippy' missed it. Its execute / execute_sp_executesql / execute_stored_procedure calls still passed the removed trailing timeout/cancel args, failing the Build stage with cargo exit 101. Drop the trailing None, None args and pass () (ExecuteOptions default). Verified: RUSTFLAGS="--cfg fuzzing" cargo +nightly fuzz build fuzz_tds_client.
Point the 'unexpected ColMetadata while reading rows' usage error at the unified advance_to_rows() API instead of the removed move_to_next(). Found during a full codebase scan for callers of the changed execute/navigation APIs — no remaining old-signature call sites exist (verified across all 9 crates including the workspace-excluded fuzz / mssql-py-core / mssql-mock-tds-py targets).
…row() in tests
The get_current_resultset() -> Option<&mut Self> method existed only as a
compatibility shim (cfg(test-util)) so the migrated tests could keep the old
'resultset handle' call shape. It re-introduced exactly the result-set-object
model the unified execute/advance API removed, so drop it and rewrite the 156
call sites across 32 test files to the real API:
if let Some(rs) = client.get_current_resultset() { rs.next_row().await? ... }
-> if client.on_rows() { client.next_row().await? ... }
Behavior is identical (the shim returned Some(self) iff on_rows()). The test-util
feature stays (still gates crypto key-gen helpers for AE tests). No shipped-API
change. Verified: cargo test --no-run (all-features + default), clippy -D warnings,
and the rewritten suites (query_results/info_messages/cursor_ops/rpc_results/
client_read_apis/tvp/bulk_copy) all green against SQL 2022.
Resolve the unified-execute refactor against main's typed/columnar fetch work (#144) and PLP/sparse row-column read APIs (#109): - tds_client.rs: main extracted the inline row-read token match into handle_row_read_token(); re-applied this branch's two edits there — the Tokens::Error batch-state clear (has_open_batch/current_metadata) that fixes the post-error close_query/advance hang, and the Tokens::ColMetadata usage message (move_to_next -> advance_to_rows). - test_client_support.rs: implement the new TdsTokenStreamReader methods resume_row_into / read_active_plp_bytes on the scripted mock (unreachable for it — it never pauses rows — so they error like the drained-stream case). - Migrate main's newly-added tests (sparse row-column reads in test_client_read_apis.rs, paused-AE-PLP in test_always_encrypted.rs) to the unified API: execute(.., ()) and on_rows()/next_row_into() instead of execute(.., None, None) / get_current_resultset(). Verified: workspace build + clippy -D warnings clean, fuzz build, mssql-odbc lib 354/354, tds client_read_apis 19/19 (incl. sparse-column) and rpc/info suites green.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 68 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
mssql-tds/src/connection/tds_client.rs:1755
- This documentation still describes the removed
expose_norow_statementsfalse/true modes. The function now has no such parameter and always applies the count-or-message predicate, so this guidance misstates the unified API's behavior.
/// With `expose_norow_statements = false` (result-set navigation used by
/// batch execution and the JS/Python consumers), a no-row statement's DONE
/// token carrying the MORE flag is skipped so the method advances to the
/// next COLMETADATA — consecutive no-row statements collapse into the
First full CI run (earlier builds died at fuzz/clippy) surfaced 5 test_vector_type
failures — all from the unified statement-wise navigation:
- 4 output-parameter tests: retrieve_output_params() returned None because the
proc's trailing RETURNVALUE tokens are only read once the batch is drained past
the proc body. Add client.advance_to_rows() after execute_stored_procedure
before reading output params (same fix already used in test_rpc_results).
- test_vector_parameter_in_where_clause: the CREATE TABLE + INSERTs setup is a
no-row multi-statement batch; 'while client.on_rows() { advance_to_rows() }'
never entered the loop (on_rows is false on a no-row boundary), leaving the
batch open -> the next execute_sp_executesql hit 'open batch must be closed or
fully consumed'. Replace all such setup drains with
'while client.advance_to_rows().await.unwrap() {}', which drains no-row
statements to end.
Can't run VECTOR tests locally (needs SQL 2025); verified compile + clippy and
the analogous test_rpc_results output-param tests pass on SQL 2022.
Address PR #143 review comments: - tds_client: begin_command() now also clears return_values (output params), so a new plain batch or sp_executesql after a fully-navigated prior RPC no longer reports stale get_return_values()/retrieve_output_params(). Removes the four now-redundant per-path clears. - tests/common validate_results: rewrite to be statement-wise — thread the StatementResult returned by execute, iterate with advance(), validate NoRows.rows_affected against Update(n) and row counts against Result(n), and assert the batch is fully consumed. Pure no-op statements (DDL/SET/ROLLBACK) collapse and are expressed as Update(0) (consume no boundary). Previously the helper collapsed every no-row boundary and skipped Update validation entirely. Verified locally (SQL 2022): query_results/transaction/reset_connection/ timeout_and_cancel 39/39, rpc_results 11/11, tds lib return-value units, clippy -D warnings clean.
… build 163010; Linux+MacOS full suite green)
The Windows-only LocalDB test used `while client.on_rows() { .. }` as the
outer drain loop. `on_rows()` reflects current_metadata.is_some(), which is
only cleared by advance()/advance_to_rows() — draining rows via next_row()
leaves it true, so the outer loop spun forever after the single row of
SELECT @@Version was consumed. This hung the Windows test job to the 60-min
cap (builds 163010, 163021). Linux/macOS never caught it: the test is
#[cfg(windows)] (LocalDB is Windows-only).
Use `if on_rows()` to drain the first rowset, then
`while advance_to_rows()` to drain any additional rowsets, matching the
correct pattern in test_simple_query in this same file.
…sql-py-core test_tracing_end_to_end async-flush race)
|
@saurabh500 - This is a breaking API change. But I think it cleans things up nicely and is worth it. Let me know what you think. |
|
LGTM |
Summary
Replaces the split execute/result-navigation surface in
mssql-tdswith a single unified API, and makes statement-wise result navigation (msodbcsql parity) the default across every consumer.This started (superseding #141) as additive statement-wise
SQLMoreResultsnavigation formssql-odbc. In review we decided that, since these APIs have no external consumers to preserve, a clean breaking redesign is better than carrying two parallel navigation paths forever. This PR now delivers that unified API and migrates all consumers plus the full test suite.New unified API (
mssql-tds)execute(sql, options) -> StatementResult, whereoptions: impl Into<ExecuteOptions>so()selects defaults.StatementResult:Rows/NoRows { rows_affected }/End.advance() -> StatementResult— drain the current statement and move to the next statement boundary.advance_to_rows() -> bool— collapse forward to the next row-returning statement (preserves the old collapsing behavior where a caller wants it).on_rows()andhas_open_batch()accessors.ExecuteOptions { timeout, cancel, column_encryption }(withDefault+ builder methods) replaces the trailingtimeout, cancel(and encryption-setting) parameters on everyexecute_*method.msodbcsql rule (verified by probing the reference driver): a statement is a navigable result iff it returns rows, carries a row count (DONE
COUNTflag), or produced messages (INFO). Pure no-op statements (e.g. a bareCREATE TABLE) collapse. Predicate:has_count || saw_message.Breaking changes
Removed in favor of the unified surface:
execute_multi_statement,move_to_next_statementResultSetClienttrait (get_current_resultset/move_to_next)execute_sp_executesql_with_encryption_setting/_coreexpose_norowbool on the result-boundary loop — statement-wise navigation is now unconditional.Every
execute_*method now takesimpl Into<ExecuteOptions>instead of positionaltimeout/cancelargs.Consumers migrated
mssql-odbc,mssql-js,mssql-py-core,mssql-tds-cli, andmssql-tds-bench, plus the fullmssql-tdsunit + integration test suite (~40 files).Production fix
get_next_row_into's error arm now clearshas_open_batch/ result-set state after draining, soclose_query/advanceno longer hang after a mid-row server error.New test scaffolding:
mssql-tds/src/test_client_support.rsThis PR adds a small
test-util-gated module that builds aTdsClienton top of a mock transport which replays a scripted sequence of TDS tokens (COLMETADATA,DONE/DONEPROCwith specific flags,INFO) instead of talking to a live server. It is not shipped API and is excluded from coverage reporting.Why it's needed. The unified statement-wise navigation pushes real logic into the seam between the TDS layer and the ODBC layer — no-row results, message-only statements, end-of-batch, and the defensive error guards. Several of those paths are impossible or flaky to reach through a real server:
COLMETADATA, or an exactDONEflag combination such asMORE|COUNT.SQLSTATEs, cursor open/closed) that the black-box ODBC C API does not expose.Benefits.
--compare-with-msodbcsqlparity run, which cover the happy paths end-to-end.Why it lives in
mssql-tdsbehind a feature. Constructing a positionedTdsClientrequires crate-internal transport traits (TdsTransport,TdsTokenStreamReader, …) andTdsClient::new. Exposing these helpers through thetest-utilfeature — opted into bymssql-odbc's dev-dependency — is the idiomatic Rust pattern; a separate testkit crate would instead force widening many internals topub.Tests & parity
clippy+fmtclean.mssql-tdslib + integration suites,mssql-odbclib, and the mock / cursor / rpc / tvp / reset-connection / bulk-copy suites are green (local SQL Server 2022; VECTOR and a timing-flaky table-lock case aside, which are environment/flake, not regressions).run_e2e.sh --compare-with-msodbcsql: 14 parity / 0 bugs. The comparison caught one real regression — prepared DML left a 0-column cursor open (SQLSTATE24000on reuse) — fixed by draining non-Rowsprepared results to idle in theSQLExecutepath.