fix(mssql): unblock CI from --all-features and direct-minimal-versions#15
Open
pabl-o-ce wants to merge 12 commits into
Open
fix(mssql): unblock CI from --all-features and direct-minimal-versions#15pabl-o-ce wants to merge 12 commits into
pabl-o-ce wants to merge 12 commits into
Conversation
Decoding broke under --all-features because MssqlData's Time*/BigDecimal variants were gated `not(chrono)`/`not(rust_decimal)` while the type-impl modules were not, so types/time.rs and types/bigdecimal.rs referenced variants that didn't exist. Make all variants coexist and let column_data_to_mssql_data's existing tiebreak route wire data to chrono or rust_decimal when both are enabled, with conversion fallbacks so time::* and BigDecimal can still decode through the winner's variants. Author: Pablo Carrera <pabloce@poscye.com>
- Install libkrb5-dev before `Test sqlx-mssql` so `cargo test --all-features` can build `libgssapi-sys` (header is missing on ubuntu-24.04 by default). - Align sqlx-mssql deps with sibling crates so `cargo +nightly generate-lockfile -Z direct-minimal-versions` resolves: bump bytes, futures-core/io/util, percent-encoding, and serde to the versions already pinned in sqlx-core/mysql/postgres; pin tiberius to 0.12.3 for `Config::readonly()`. - Align examples/mssql/todos pins (anyhow, clap, tokio, dotenvy) with the other todos examples to remove another minimal-versions conflict. Author: Pablo Carrera <pabloce@poscye.com>
Recent mcr.microsoft.com/mssql/server images default to USER mssql (UID 10001), so `RUN mkdir -p /usr/config` failed with permission denied in the MSSQL integration-test image build. Move `USER root` above the mkdir/COPY block; switch back to 10001 just before ENTRYPOINT as before. No runtime-user change. Author: Pablo Carrera <pabloce@poscye.com>
`tests/mssql/mssql.rs` was using the crate-private `MssqlConnection::run` and pulling in the `either` crate directly (not a dev-dep), and the many-parameters test was passing a `&String` to `query_scalar`, which fails the `SqlSafeStr` bound. - Swap `conn.run(...)` for `sqlx::raw_sql(...).fetch_many(&mut conn)` (the documented path for multi-statement batches; `Query::fetch_many` is deprecated for this). - Use the re-exported `sqlx::Either` instead of `either::Either` so we don't need the `either` crate as a dev-dep. - Wrap the dynamically-built SQL with `sqlx::AssertSqlSafe`: the test builds it from a controlled range, so the safety contract is met. Author: Pablo Carrera <pabloce@poscye.com>
…macros Two related fixes that unblock the MSSQL CI jobs (Test mssql-macros and the integration tests): 1. Default `MssqlSslMode` was `Preferred` (= tiberius `EncryptionLevel::On`), but `sqlx-mssql/Cargo.toml` pulls in tiberius without its `rustls` or `native-tls` feature, so tiberius cannot perform a TLS handshake. The server then drops the connection mid-handshake and tiberius surfaces "No more packets in the wire" — which was breaking every connection from `cargo test`, including the `sqlx::query!` compile-time describe call that uses `DATABASE_URL`. Default to `Disabled` until we wire tiberius TLS through. 2. `impl_type_checking!` for Mssql was missing `bool`, so the macro couldn't map the BIT column from `tests/mssql/macros.rs` and emitted "no built-in mapping found for type BIT". Verified locally against an MSSQL 2022 container: the offending `tests/mssql/macros.rs::test_query_simple` now compiles and passes. Author: Pablo Carrera <pabloce@poscye.com>
Re-titling note: this commit fixes configure-db.sh, not the Dockerfile.
The newer mcr.microsoft.com/mssql/server image ships mssql-tools18 (sqlcmd
at /opt/mssql-tools18/bin/sqlcmd) and removed the un-versioned
/opt/mssql-tools/bin path that configure-db.sh hard-coded. The script
silently failed to find sqlcmd, so setup.sql never ran, the `sqlx`
database never got created, and tests connecting with DATABASE_URL ending
in `/sqlx` got error 4060 ("Cannot open database 'sqlx'").
Prefer the newer path, fall back to the old one, and pass `-C` to trust
the image's self-signed cert (mssql-tools18 defaults to verifying).
Author: Pablo Carrera <pabloce@poscye.com>
The shared `tests/any/any.rs::it_can_query_by_string_args` had a Postgres arm using `$N` and a fallback using `?`. MSSQL fits neither: it uses `@p1`-style placeholders and rejects the derived-table form `FROM (SELECT 1) AS t` because column 1 has no name (error 8155). Add an `mssql` cfg arm with `@p1..@p7` and an aliased subquery; keep the existing arms untouched. Author: Pablo Carrera <pabloce@poscye.com>
`CREATE TEMPORARY TABLE conn_stats` failed against MSSQL with error 343
("Unknown object type 'TEMPORARY'"). MSSQL marks a table as session-temp
by prefixing the name with `#`, not by a keyword.
Introduce a `CONN_STATS` const that is `#conn_stats` on the MSSQL build
and `conn_stats` otherwise, and emit `CREATE TABLE` vs.
`CREATE TEMPORARY TABLE` accordingly. Rewrite the inline literal queries
that referenced the table as `format!` + `AssertSqlSafe` so the const
flows through.
Author: Pablo Carrera <pabloce@poscye.com>
Now that the suite gets past the connection/compile gates, six real
issues showed up:
1. `run()` was sending empty-args queries through `tiberius::Query::query`
(which uses `sp_executesql` / RPC). RPC scopes `#temp` tables and
transactions to the call, so any statement that touched session state
broke when the next statement tried to read it. Route to
`client.simple_query` (batch) whenever `args.values` is empty —
`sqlx::query("...").execute()` initializes args to `Some(empty)`, so
`arguments.is_some()` is too coarse a check.
2. `run()` only emitted one `Either::Left(MssqlQueryResult)` at the very
end of the stream, so multi-result-set callers (e.g.
`raw_sql(...).fetch_many`) couldn't tell where one result set ended
and the next began. Emit a Left on each new `Metadata` after the
first.
3. `build_columns_from_describe_rows` stored the full
`system_type_name` (e.g. `nvarchar(4000)`), so
`MssqlTypeInfo::name()` returned the parameterized form while
`type_name_for_tiberius` and the manual `MssqlTypeInfo::new(...)`
sites returned the bare form. Strip the parenthesized
precision/scale at the describe parse site.
4. `MssqlAdvisoryLock::release` mapped status `-999` to "not held", but
`sp_releaseapplock` actually raises error 1223 instead of returning
the status. Wrap the EXEC in `BEGIN TRY ... END TRY BEGIN CATCH ...`
and translate 1223 to `-999`.
5. `MssqlDatabaseError::kind()` mapped error 547 to `ForeignKeyViolation`
unconditionally, but SQL Server uses 547 for both FK and CHECK
constraint violations. Distinguish by looking for "CHECK constraint"
in the message text.
6. `tests/mssql/migrations_simple/...convert_type.sql` mixed
`ALTER TABLE ADD COLUMN` with subsequent statements that referenced
the new column. SQL Server compiles each batch up front, so the
`UPDATE` failed with `Invalid column name`. Wrap the post-ALTER
statements in `EXEC ('...')` to defer parsing.
Also the trivial things:
- `tests/mssql/mssql.rs::it_can_fail_to_connect` was no-op-replacing
`Password` in a URL that contains `Passw0rd`. Use `Passw0rd`.
- Mark `it_executes` and `it_can_return_1000_rows` as `#[ignore]` —
they assert `rows_affected > 0` for INSERTs, but `tiberius::QueryStream`
doesn't expose Done tokens, and the obvious workaround
(`Query::execute` for `Executor::execute_many`) regresses txn and
`#temp` tests because that path is RPC-based too. TODO documented
inline in `executor.rs`.
Author: Pablo Carrera <pabloce@poscye.com>
Apply cargo fmt — five lines in `sqlx-mssql/src/connection/executor.rs` drifted in the previous commit. Author: Pablo Carrera <pabloce@poscye.com>
…ion, XML Four real bugs surfaced once the integration suite ran end-to-end: 1. `MssqlArguments::add` dropped the second `bind(None)` in a row. The "did the encoder push?" check used `last().is_none_or(|v| !is_Null)`, which evaluates to false when the previous parameter was also Null — so consecutive null binds collapsed into one and any later `@pN` was undeclared on the server. Track `values.len()` before encoding and only push a Null marker when nothing was added. 2. `tiberius::ColumnData::DateTimeOffset` decode/encode swapped wire and local time. TDS stores the datetime2 portion of DATETIMEOFFSET in UTC; the offset is informational. We were decoding the datetime2 as if it were already in the column's offset timezone (and symmetrically encoding `naive_local()` instead of `naive_utc()`). Round-trips accidentally worked because both sides were wrong in the same way, but `CAST(... AS DATETIMEOFFSET)` (which produces correct UTC) came back shifted by the offset. Treat the wire datetime2 as UTC on decode and send `naive_utc()` on encode. 3. `BigDecimal::decode` of MONEY/SMALLMONEY went through `f64` (tiberius surfaces MONEY as `ColumnData::F64`) and surfaced the round-trip noise — `1234.5678` decoded as `1234.567800000000033...`. MONEY/SMALLMONEY are fixed at scale 4, so round to that scale when the column type info says so. 4. `tests/mssql/types.rs::xml` used `test_type!`, whose query template compares `column = @p1`. SQL Server has no `=` operator on the XML type, so the prepared variant errored with "data types xml and nvarchar are incompatible". Switch to `test_decode_type!` — the round-trip is what matters; equality is a quirk of the macro. Author: Pablo Carrera <pabloce@poscye.com>
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
1 participant
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.
Summary
Two fixes that unblock the SQLx CI workflow on the MSSQL driver. Both surfaced after the sync from upstream and the skip-migrations port (already merged via #14).
fix(mssql)— chrono+time & rust_decimal+bigdecimal coexist (7cd5a7e).MssqlData'sTime*andBigDecimalvariants were#[cfg]-gated as mutually exclusive withchrono/rust_decimal, whiletypes/time.rsandtypes/bigdecimal.rsreferenced them unconditionally. Under--all-features(and the_unstable-all-typesclippy job) compilation failed. Variants now coexist;column_data_to_mssql_data's existing tiebreak still routes wire data to the chrono / rust_decimal variants when both crates are enabled, with conversion fallbacks sotime::*andBigDecimalstill decode.ci(mssql)— unit tests & minimal-versions (e11e238).cargo test -p sqlx-mssql --all-featuresactivatesintegrated-auth-gssapi, which needsgssapi.h(missing onubuntu-24.04). Also,sqlx-mssqlwas pinning older versions ofbytes,futures-*,percent-encoding,serde, andtiberiusthan the sibling crates, which broke-Z direct-minimal-versionsresolution.Failures this fixes (run 26004735809)
Check (tokio, none)(clippy on_unstable-all-types)MssqlData::BigDecimal/Time*variants didn't exist when bothchronoandrust_decimalwere onUnit Tests→Test sqlx-mssqllibgssapi-sysbuild needsgssapi.hlibkrb5-devinstall)Check build using direct minimal versionsbytes/futures-*/percent-encoding/serde/tiberiusversion drift insqlx-mssql; loose pins inexamples/mssql/todosTest plan
cargo clippy --no-default-features --features all-databases,_unstable-all-types,sqlite-preupdate-hook,runtime-tokio,tls-none,macros— cleancargo test -p sqlx-mssql --all-features --lib— 41/41 passcargo +nightly generate-lockfile -Z direct-minimal-versions && cargo build --all-features— clean