ci: modernize Actions, add cargo-deny gate, clear baseline lints by MattJackson · Pull Request #440 · tiberius-rs/tiberius

MattJackson · 2026-09-02T15:07:22Z

CI modernization and supply-chain hardening, plus a batch of baseline correctness/lint fixes.

Modernize the GitHub Actions workflow; port stranded async-std tests to #[test_on_runtimes] (@jakewimmer).
Add a cargo-deny supply-chain gate; modernize dev-dependencies.
Resolve 12 panics/correctness bugs from the issue backlog.
Clear all clippy warnings and rustfmt the tree (also fixes the pre-existing runtimes-macro manual_unwrap_or_default lint under rust ≥1.98).

Supersedes #389 — cache-action update; our workflow already uses a newer action.

Part of a sequential series — please merge in order after #432 (security). This PR is based on main, so until #432 merges its diff also shows #432's 2 commits; once #432 lands, this reduces to its own 5 commits.
Reviewer note: please rebase-merge or merge-commit, not squash to preserve @jakewimmer's authorship.

aqrln

This needs a rebase due to the changes on main

aqrln · 2026-09-04T20:25:30Z

The pre-existing conflicts is probably why GitHub didn't automatically rebase the stack after merging the first PR

Two tests written before #[test_on_runtimes] existed and never updated. cyrillic_collations_should_work previously created a dedicated database with a Cyrillic default collation, requiring an admin connection and DROP DATABASE at teardown. The DROP raced against open connections on macOS/rustls CI, causing flaky failures. Replace with a session-local temp table using column-level COLLATE clauses. The code path under test (COLMETADATA collation -> encoding_rs decode) is identical. application_name_should_be_set_correctly needed the application name set before connecting. Add APP_NAME_CONN_STR embedding it in the connection string so the macro-generated harness connects with it set. (cherry picked from commit 6247684)

- Add deny.toml: fails on any vulnerability/yanked crate in the built graph; documents justified ignores for advisories that are provably dev-dependency-only or reachable solely via the opt-in sql-browser-async-std feature (not part of the default shipped lib). - Replace the broken prisma-org PR Code Security workflow (which fails at startup on the fork) with a self-contained Security audit workflow that runs cargo-deny on push/PR and weekly. - Bump dev-deps env_logger 0.9->0.11 and indicatif 0.17->0.18, dropping the unmaintained atty/number_prefix transitives from the test graph. cargo deny check advisories bans sources: advisories ok, bans ok, sources ok. (cherry picked from commit 1ce85b7)

Direct fixes for long-standing reported issues (regression tests added, 128 lib tests pass): - #211: bounds-check usize column index (try_get returns Err, not panic) - #382: match raw-identifier column names (r#type -> SQL 'type') - #418: correct swapped old/new in EnvChange Display (Database, PacketSize) - #281: lower chatty per-connection/token logs from INFO to DEBUG - #263: convert SQL smallint (I16/Intn) into i32 via FromSql - #424/#425: return Error instead of panicking on unexpected server input in the TDS decoder (incl. negotiated_encryption) - #305: error at connect time when encryption is required but no TLS feature is compiled in - #316: fix multiply-overflow panic decoding dates before 1900 - #358/#352: coerce numerics into Money/SmallMoney and strings into NText/Text columns during bulk insert - #348: send the ReadOnly intent flag in LOGIN7 when ApplicationIntent=ReadOnly (cherry picked from commit 34e5e55)

- Resolve 20 pre-existing clippy lints under `cargo clippy --features=all -- -D warnings` (legacy numeric methods/constants, unused/elidable lifetimes, redundant closure, doc list indentation, format specifier, derivable Default via #[default]); narrow justified #[allow] for the uint_enum! cast and the tds::time module inception. - rustfmt the files touched by the backlog fixes. dev green gate: fmt --check clean, build ok, clippy -D warnings ok, cargo-deny ok, 128 lib tests pass. (cherry picked from commit edda463)

The linux test lane started SQL Server and ran the suite without waiting for it to accept logins — SQL binds 1433 before the SA login/databases finish initializing, so tests raced startup and failed sporadically (scattered across DB versions and feature sets, the signature of a race). Gate on an authenticated SELECT 1 from a throwaway mssql-tools container, which works uniformly across the full server images and azure-sql-edge.

- security.yml: shorten the cargo-deny step comment - deny.toml: trim the header preamble (per-entry reasons kept) - connection.rs: drop the no-TLS comment duplicating the helper doc - row.rs: QueryIdx for usize via then_some

MattJackson · 2026-09-04T20:50:35Z

@aqrln rebased onto the merged main, all green and MERGEABLE — ready for review whenever you get a chance. Thanks!

…lback

…Size

SimSmith · 2026-09-06T18:13:19Z

This catch-all is pretty dangerous for maintenance. Shouldn't the Money variant be here?

Addressed. The Display impl no longer relies on a catch-all for correctness: money is explicit (FixedLenType::Money→money, Money4→smallmoney; VarLenType::Money len 4/8→smallmoney/money), and any type with no valid sized SQL name now returns std::fmt::Error rather than panicking or emitting a bogus Debug name. Added tests that money renders correctly and that an unknown type yields Err, not a panic.

SimSmith · 2026-09-06T18:20:13Z

                dst.put_f64_le(val);
            }
+            (ColumnData::F64(opt), Some(TypeInfo::VarLenSized(vlc)))
+                if vlc.r#type() == VarLenType::Money =>


Where is the FixedLenType::Money match?

Added. A NOT-NULL money/smallmoney arrives on the bulk path as FixedLen(Money) (8-byte) / FixedLen(Money4) (4-byte) and previously fell through to the catch-all error. There are now explicit FixedLen(Money/Money4) arms for both F64 and Numeric, encoding via money::encode_fixed/encode_numeric_fixed. Verified end-to-end against a live SQL Server — the full bulk suite passes (120/120).

…undtrip Adds server-free mock-reader unit tests for two already-fixed panic->Error::Protocol conversions that had no coverage: - column_data::int::decode: invalid Intn length (e.g. 3) returns Error::Protocol instead of hitting the unimplemented!() branch. - TokenFeatureExtAck::decode: invalid FedAuth data length and an unsupported feature id both return Error::Protocol; the module had no #[cfg(test)] mod at all before this. Also adds bulk_ntext_value_roundtrips mirroring the existing bulk_text_value_roundtrips, exercising NTEXT bulk-insert/read-back with a UTF-16-heavy string. Server-gated via test_on_runtimes; not run locally.

A NOT-NULL money column arrives from the server as TypeInfo::FixedLen(Money) (8-byte) and smallmoney as FixedLen(Money4) (4-byte), but the bulk row-encode match only had arms for the nullable MONEYN (VarLenSized) form. NOT-NULL money columns fell through to the BulkInput catch-all: invalid data type, expecting Some(FixedLen(Money)) but found F64(Some(...)) Add FixedLen(Money)/FixedLen(Money4) arms for both ColumnData::F64 and ColumnData::Numeric. FixedLen types carry the raw fixed-width bytes with NO length prefix (mirroring fixed_len::decode and the sibling Float8/Datetime FixedLen arms), so add money::encode_fixed / encode_numeric_fixed wrappers that share the existing scaling and range-check logic but omit the length byte. (cherry picked from commit 32e9d80)

…ARCHAR without NumParts (fixes 4804)

… on unknown types (cherry picked from commit 2786cbb0ce4f28b0f1e0c9aa347f85e763514e1f)

MattJackson · 2026-09-07T01:49:25Z

Updated + server-verified green. Added the missing FixedLen(Money/Money4) bulk arms, made the column-type Display non-panicking, and fixed the text/ntext/image COLMETADATA TableName — it must be a bare US_VARCHAR with no NumParts byte on the client→server bulk path (matched to Microsoft's go-mssqldb; the server→client read format differs, and the decode side is unchanged). The full bulk suite now passes 120/120 against a live SQL Server, and I added unit coverage for the money boundary cases and decode error paths. Also fixed a rustfmt drift in row.rs.

aqrln · 2026-09-08T16:11:34Z

+            Error::Protocol(msg) => {
+                assert!(msg.to_string().contains("invalid integer length"));
+            }
+            other => panic!("expected Error::Protocol, got {:?}", other),


nit:

Suggested change

other => panic!("expected Error::Protocol, got {:?}", other),

other => panic!("expected Error::Protocol, got {other:?}"),

Will fix — switching to the inline captured form.

aqrln · 2026-09-08T16:15:58Z

+
+    #[test]
+    fn display_var_len_unknown_type_yields_err_not_panic() {
+        use std::fmt::Write as _;


why not move it to the other imports on the module level?

Agreed — I'll hoist use std::fmt::Write as _; up to the mod tests import block.

aqrln · 2026-09-08T16:16:36Z

+            Error::Protocol(msg) => {
+                assert!(msg.to_string().contains("invalid data length"));
+            }
+            other => panic!("expected Error::Protocol, got {:?}", other),


Suggested change

other => panic!("expected Error::Protocol, got {:?}", other),

other => panic!("expected Error::Protocol, got {other:?}"),

Will fix — same inline-format change here.

aqrln · 2026-09-08T16:29:30Z

+        for column in columns.iter_mut() {
+            column.base.table_name = Some(table.to_string());
+        }


This will allocate a string per column. Can we reuse the string somehow? I suppose we could use something like Arc<str> here without breaking the public API and only allocate once?

Good instinct, but a couple of things make this not worth the change here:

This loop runs once per bulk-load request during metadata setup, not per row — it's N short-lived allocations where N is the column count (a handful), on a cold path, not something that shows up in a profile against the per-row encode work.

The bigger issue: table_name is a public field on BaseMetaDataColumn, which is re-exported from the crate root (lib.rs), so changing Option<String> → Option<Arc<str>> is a breaking public-API change (cargo-semver-checks flags it). I'd rather not spend an API break on a cold setup path. Happy to revisit if bulk metadata construction ever profiles hot — at which point threading the name through encode (rather than storing it per column) avoids both the allocation and the API change.

Address review nits: use the inline `{other:?}` capture form in the `match`-arm `panic!` assertions (int/feature-ext-ack/pre-login decode tests), and hoist the function-local `use std::fmt::Write as _;` to the `mod tests` import block in token_col_metadata.

MattJackson changed the title ~~stack/s2~~ ci: modernize Actions, add cargo-deny gate, clear baseline lints Sep 2, 2026

MattJackson force-pushed the stack/s2 branch from 96006ab to 94d9548 Compare September 2, 2026 19:34

MattJackson force-pushed the stack/s2 branch 3 times, most recently from 453bf96 to b632e69 Compare September 2, 2026 23:54

MattJackson force-pushed the stack/s2 branch from b632e69 to 40a1944 Compare September 3, 2026 00:01

MattJackson closed this Sep 3, 2026

MattJackson reopened this Sep 3, 2026

MattJackson force-pushed the stack/s2 branch from da3a128 to 2d282be Compare September 3, 2026 15:05

aqrln reviewed Sep 4, 2026

View reviewed changes

Base automatically changed from stack/s1 to main September 4, 2026 20:23

jakewimmer and others added 6 commits September 4, 2026 13:27

chore(ci): trim review-flagged comments; tidy row idx idiom

f77d538

- security.yml: shorten the cargo-deny step comment - deny.toml: trim the header preamble (per-entry reasons kept) - connection.rs: drop the no-TLS comment duplicating the helper doc - row.rs: QueryIdx for usize via then_some

MattJackson force-pushed the stack/s2 branch from 2d282be to f77d538 Compare September 4, 2026 20:42

MattJackson added 2 commits September 6, 2026 09:44

fix(row): match column names exactly before the r# raw-identifier fal…

bf25ac6

…lback

refactor(envchange): use named old/new fields for Database and Packet…

e2e8700

…Size

SimSmith reviewed Sep 6, 2026

View reviewed changes

MattJackson added 5 commits September 6, 2026 16:04

fix(bulk): emit text/ntext/image COLMETADATA TableName as a bare US_V…

103e044

…ARCHAR without NumParts (fixes 4804)

fix(bulk): handle money in the column-type Display and stop panicking…

385926f

… on unknown types (cherry picked from commit 2786cbb0ce4f28b0f1e0c9aa347f85e763514e1f)

style: rustfmt row.rs; make display money assertion bracket-agnostic

f4f15d7

aqrln reviewed Sep 8, 2026

View reviewed changes

aqrln approved these changes Sep 15, 2026

View reviewed changes

MattJackson merged commit 06bd65f into main Sep 15, 2026
33 checks passed

MattJackson deleted the stack/s2 branch September 15, 2026 23:02

	other => panic!("expected Error::Protocol, got {:?}", other),
	other => panic!("expected Error::Protocol, got {other:?}"),

Conversation

MattJackson commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aqrln left a comment

Choose a reason for hiding this comment

Uh oh!

aqrln commented Sep 4, 2026

Uh oh!

MattJackson commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattJackson commented Sep 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MattJackson commented Sep 2, 2026 •

edited

Loading