Skip to content

feat(config): add opt-in lossy code-page decoding - #453

Merged
MattJackson merged 1 commit into
tiberius-rs:mainfrom
t8y2:dev/lossy-codepage-decoding
Sep 25, 2026
Merged

MattJackson merged 1 commit into
tiberius-rs:mainfrom
t8y2:dev/lossy-codepage-decoding

Conversation

@t8y2

@t8y2 t8y2 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add Config::lossy_codepage_decoding(bool), its getter, and a ConfigBuilder counterpart, following the opt-in policy used for lossy_utf16_decoding in Error: UTF-16 error #325/fix: tolerate malformed UTF-16 row values #426.
  • Keep strict decoding as the default. When explicitly enabled, invalid bytes in CHAR, VARCHAR/VARCHAR(MAX), TEXT, and intrinsic CHAR/VARCHAR values inside SQL_VARIANT become U+FFFD instead of aborting row decoding.
  • Always use the declared collation, without BOM sniffing or BOM stripping.
  • Keep unknown collations and malformed framing as errors. Unicode decoding remains independently controlled; XML, metadata/protocol strings, and outgoing encoding are unaffected. No dependency changes.

Why

A legacy character column can contain bytes that are invalid in its declared collation, for example 0x81 0x20 under GB18030. The current strict decoder returns Error::Encoding("invalid sequence") before the row reaches the caller. Applications cannot display the remaining valid data or continue consuming the result through that error.

This offers the same explicit, conservative recovery choice as the new UTF-16 option without weakening default behavior. In particular, using decode_without_bom_handling is intentional: EF BB BF 61 62 63 is valid GB18030 data (锘縜bc), not a UTF-8 BOM followed by abc.

let mut config = tiberius::Config::new();
config.lossy_codepage_decoding(true);

Regression coverage

  • Raw TDS cases reproduce strict decoding failures on the base revision and pass when the new option is enabled; disabled-mode assertions preserve the original errors.
  • Shift-JIS, EUC-KR, and GB18030 invalid sequences; CHAR, VARCHAR, TEXT, chunked VARCHAR(MAX), and SQL_VARIANT character values.
  • BOM-like bytes, empty/NULL values, unknown collations, truncated frames, independent Unicode/XML behavior, strict outgoing encoding, and successful decoding of the following value after a malformed value.
  • Config/builder defaults, setters, and independence from the UTF-16 option.

Validation

  • cargo fmt --all -- --check — passed.
  • cargo nextest run --lib --no-default-features — 874 passed.
  • cargo nextest run --lib --no-default-features --features tds73 — 918 passed.
  • cargo nextest run --lib --no-default-features --features rustls,chrono,time,tds73 — 1,003 passed, including a rerun in a dedicated build directory.
  • cargo test --doc --no-default-features --features rustls,chrono,time,tds73 client::config — 7 passed, including the new API example.
  • cargo clippy --lib --no-default-features --features rustls,chrono,time,tds73 — completed with the same two existing warnings as the unmodified base (needless_borrow in client/connection.rs and large_enum_variant in client/tls.rs). -D warnings also fails on the base for those warnings.
  • Full doctests with the same features: 32 passed, 1 ignored, 7 failed because their examples require a live SQL Server (ConnectionRefused). An isolated run against unmodified base 7fd75bb reproduced exactly the same seven failures (31 passed, 1 ignored).

Unit/raw-TDS-frame and doctest validation only; no live SQL Server run.

@MattJackson

Copy link
Copy Markdown
Contributor

Nice, the opt-in design mirrors lossy_utf16_decoding well, and the default stays strict. Live against 2022/2019/2017 with Chinese_PRC, Japanese, Korean_Wansung and Chinese_Taiwan_Stroke collations, invalid bytes decode to U+FFFD and every following row, including multi-byte values crossing chunk boundaries, matches the server. With the option off, behaviour is unchanged. Some suggestions:

  1. This conflicts with fix(collation): support legacy CP437 and CP850 code pages #452. If fix(collation): support legacy CP437 and CP850 code pages #452 lands first, the lossy branch could go through the new codec, e.g. a CollationCodec::decode_lossy. Otherwise CP437/CP850 columns would still fail with lossy on. A test with sort ID 30/49 plus lossy would guard this.
  2. It looks like nothing tests the setting reaching the decoder from Config: removing the set_lossy_codepage line in connection.rs still passes every lib test. An integration test would need a database created with COLLATE Chinese_PRC_CI_AS, since COLLATE on a temp table converts the bytes to '?'.
  3. It might be worth documenting that replacement follows encoding_rs/WHATWG rather than SQL Server: 61 81 20 62 becomes "a\u{FFFD} b" where the server gives "a?b", and a lone 0xFF becomes U+FFFD where the server gives a private-use character.
  4. Optional: the "had replacements" flag from decode_without_bom_handling could drive a tracing event so users can see when replacement happened.
  5. A doc comment on Context::lossy_codepage would round it out. Keeping the separate bool seems right, given lossy_utf16_decoding shipped in 0.13.0.

Please let me know if I'm wrong on any assumptions. I look forward to working with you on this!

@t8y2

t8y2 commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed feedback. Updated in 0783b0e. I merged the current #452 branch into this one to resolve the overlapping decoder changes while preserving the existing commits, so this PR now includes that dependency.

Strict and lossy decoding now use the same CollationCodec, with a decode_lossy implementation for the built-in encodings and CP437/CP850. Regression cases cover sort IDs 30 and 49 with the option both off and on, including chunked VARCHAR(MAX) and SQL_VARIANT. Restoring the old lossy path through Collation::encoding() makes the new compatibility regression fail.

I also added a connection-level regression using its own Chinese_PRC_CI_AS database. It checks the default, explicitly disabled, and enabled settings through Config, with malformed bytes followed by valid results. The API docs now explain the encoding_rs/WHATWG replacement behavior versus SQL Server conversion, and the context preference is documented.

The combined library suite passes with 882 tests under --no-default-features and 1,011 with rustls,chrono,time,tds73; the seven configuration doctests also pass. I left the optional tracing event for a separate follow-up.

* fix(collation): compose lossy decoding with legacy code pages
@MattJackson
MattJackson force-pushed the dev/lossy-codepage-decoding branch from 0783b0e to bbed888 Compare September 25, 2026 03:06

@MattJackson MattJackson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for 0783b0e, and for turning this around so thoroughly.

I went through each point against the code and on real servers:

  • Lossy decoding now goes through CollationCodec::decode_lossy in all three places (VARCHAR/PLP, TEXT, SQL_VARIANT). When I routed each call site back through Collation::encoding() one at a time, codepage_lossy_preserves_legacy_codepages failed every time, so sort IDs 30/49 with lossy on are properly covered.
  • lossy_codepage_config_reaches_decoder does exactly what I hoped for. Removing the set_lossy_codepage line in connection.rs makes it fail on SQL Server 2022, 2019 and 2017, and under -Dwarnings it doesn't even compile.
  • The WHATWG vs SQL Server note and the Context doc comments look good. Deferring the tracing event is totally fine.
  • fmt, clippy, the lib tests, the -Dwarnings integration-matrix builds and the MSRV check are all clean.

#452 is merged now, so I rebased this branch onto main to save you the trouble. It's a single commit with your authorship (bbed888), and the tree is exactly your 0783b0e on top of main, with no content changes. The full query (201) and bulk (132) suites pass on 2022, 2019 and 2017 with it. If you'd rather keep your original two commits, just say and I'll put them back.

Approving.

@MattJackson
MattJackson merged commit 35605c9 into tiberius-rs:main Sep 25, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants