feat(config): add opt-in lossy code-page decoding - #453
Conversation
|
Nice, the opt-in design mirrors
Please let me know if I'm wrong on any assumptions. I look forward to working with you on this! |
|
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 I also added a connection-level regression using its own The combined library suite passes with 882 tests under |
* fix(collation): compose lossy decoding with legacy code pages
0783b0e to
bbed888
Compare
MattJackson
left a comment
There was a problem hiding this comment.
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_lossyin all three places (VARCHAR/PLP, TEXT, SQL_VARIANT). When I routed each call site back throughCollation::encoding()one at a time,codepage_lossy_preserves_legacy_codepagesfailed every time, so sort IDs 30/49 with lossy on are properly covered. lossy_codepage_config_reaches_decoderdoes exactly what I hoped for. Removing theset_lossy_codepageline inconnection.rsmakes it fail on SQL Server 2022, 2019 and 2017, and under-Dwarningsit doesn't even compile.- The WHATWG vs SQL Server note and the
Contextdoc comments look good. Deferring the tracing event is totally fine. - fmt, clippy, the lib tests, the
-Dwarningsintegration-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.
Summary
Config::lossy_codepage_decoding(bool), its getter, and aConfigBuildercounterpart, following the opt-in policy used forlossy_utf16_decodingin Error: UTF-16 error #325/fix: tolerate malformed UTF-16 row values #426.CHAR,VARCHAR/VARCHAR(MAX),TEXT, and intrinsicCHAR/VARCHARvalues insideSQL_VARIANTbecomeU+FFFDinstead of aborting row decoding.Why
A legacy character column can contain bytes that are invalid in its declared collation, for example
0x81 0x20under GB18030. The current strict decoder returnsError::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_handlingis intentional:EF BB BF 61 62 63is valid GB18030 data (锘縜bc), not a UTF-8 BOM followed byabc.Regression coverage
CHAR,VARCHAR,TEXT, chunkedVARCHAR(MAX), andSQL_VARIANTcharacter values.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_borrowinclient/connection.rsandlarge_enum_variantinclient/tls.rs).-D warningsalso fails on the base for those warnings.ConnectionRefused). An isolated run against unmodified base7fd75bbreproduced exactly the same seven failures (31 passed, 1 ignored).Unit/raw-TDS-frame and doctest validation only; no live SQL Server run.