Skip to content

Rewrite ODBC connection string parser to mirror msodbcsql ParseAttrStr - #107

Merged
Theekshna merged 12 commits into
mainfrom
saurabh500-odbc-connstr-parser
Jul 16, 2026
Merged

Rewrite ODBC connection string parser to mirror msodbcsql ParseAttrStr#107
Theekshna merged 12 commits into
mainfrom
saurabh500-odbc-connstr-parser

Conversation

@saurabh500

@saurabh500 saurabh500 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

AB#46370

Replaces the two-phase tokenize + interpret connection-string parser in mssql-odbc/src/connection/mod.rs with a single-pass, character-by-character state machine that reproduces the shipping msodbcsql driver's ParseAttrStr (Sql/Ntdbms/sqlncli/odbc/sqlcconn.cpp) — including its quirks, so apps migrating from msodbcsql18 see identical parsing semantics.

The public signature parse_connection_string(&str) -> Result<(ConnectionParams, bool), InvalidAttrValue> is unchanged, so api/driver_connect.rs (which maps has_warnings01S00 and Err → connect failure) needs no change.

Reproduced msodbcsql behaviors

Quirk Behavior
Key scan reads through ; A token without its own = merges with following text
Missing = in remainder S_FALSEstop parsing, keep what was parsed
No whitespace trimming Only leading whitespace/; before a key is skipped; Server =h no longer matches; value spaces are preserved verbatim
{braced} values End at a single }; }} escapes a literal }
Junk after } S_FALSE — stop (value still stored)
Unterminated { S_FALSE — swallow rest, stop
Recognized-but-unsupported keys No warning (full x_rgLookup set)
Unknown keywords 01S00 warning, never fail
Invalid value on validated key E_FAILErr (hard connect failure)

Diagnostic mapping: S_OKOk((_, false)); S_FALSEOk((_, true)) (01S00 / SQL_SUCCESS_WITH_INFO); E_FAILErr(InvalidAttrValue).

Notable changes

  • Expanded KNOWN_IGNORED_KEYS to msodbcsql's recognized-but-not-acted-on set (driver, dsn, app, wsid, applicationintent, multisubnetfailover, columnencryption, tnir, deprecated keys, …) so recognized keys stay silent.
  • Dropped the ADO.NET/OLE DB-only aliases Initial Catalog, User Id, and Password — the msodbcsql ODBC parser does not recognize them; they are now treated as unknown keys, matching the driver.
  • Deliberately not mirrored: the fixed-buffer MAXPATHLEN/MAXKEYLEN length caps (artifacts, not contract).

Tests

Exhaustive unit tests covering every quirk plus value validation, first-wins duplicates, whitespace fidelity, }} escaping, and empty/separator edge cases. 112 passed for the crate; cargo fmt --check and cargo clippy -- -D warnings are clean.

Docs

Adds mssql-odbc/docs/connection_string_parser.md documenting the algorithm, the divergence table, the state diagram, and diagnostic mapping.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

saurabh500 and others added 7 commits July 13, 2026 16:41
Replace the two-phase tokenize+interpret parser with a single-pass,
character-by-character state machine that reproduces msodbcsql's
ParseAttrStr semantics, including its quirks: the key scan reads through
';', a missing '=' stops parsing, keys and values are never trimmed, '}}'
escapes a literal '}' in braced values, junk after a braced value stops
parsing, and unknown keywords warn (01S00) but never fail. Only an invalid
value on a recognized validated key is a hard error (E_FAIL).

Expand the recognized-but-ignored key set to msodbcsql's x_rgLookup, and
drop the ADO.NET-only aliases (Initial Catalog, User Id, Password) which
the ODBC parser does not recognize. Add exhaustive unit tests and a design
doc.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub's mermaid parser rejects transition labels containing quotes,
parentheses, slashes, and semicolons. Replace them with plain-word labels.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An unknown keyword raises 01S00 but parsing continues; only the four
structural malformations (missing '=', missing value, unterminated brace,
junk after brace) stop the scan.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract the parser out of connection/mod.rs into connection/connection_string_parser.rs, leaving mod.rs as a thin module root that declares submodules and re-exports parse_connection_string. No behavior change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Enrich the connection-string parser with example-driven comments on each phase of the state machine, add a classify_key growth TODO, document that '{' has no escape (only '}}' does), and cover it with open-brace unit tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
msodbcsql validates connection-string values verbatim: IsAttrStrValid requires an exact length match plus case-insensitive content, so whitespace-padded values fail. Verified empirically against ODBC Driver 18 (whitespace -> 08001 invalid-value rejection before connect; clean value -> reaches login). Covers TrustServerCertificate, Encrypt, and Trusted_Connection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

100%

🎯 Overall Coverage

90.9%

📦 Project: mssql-tds + mssql-py-core
ℹ️ Note: diff coverage is reported, not enforced.


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.


🔗 Quick Links

View Azure DevOps Build · Coverage Report

…tr-parser

# Conflicts:
#	mssql-odbc/src/connection/mod.rs
@saurabh500
saurabh500 marked this pull request as ready for review July 15, 2026 13:43
@saurabh500
saurabh500 requested a review from a team as a code owner July 15, 2026 13:43
Copilot AI review requested due to automatic review settings July 15, 2026 13:43

Copilot AI 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.

Pull request overview

This pull request rewrites the mssql-odbc connection-string parser to a single-pass state machine intended to mirror msodbcsql’s ParseAttrStr behavior (including documented quirks), while keeping the external parse_connection_string(&str) -> Result<(ConnectionParams, bool), InvalidAttrValue> API shape intact for the rest of the ODBC driver.

Changes:

  • Replaces the previous tokenizer/interpret parser with a new state-machine implementation in a dedicated connection_string_parser module.
  • Refactors connection/mod.rs into a small module hub that re-exports parse_connection_string.
  • Adds documentation describing the algorithm and its msodbcsql compatibility goals.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
mssql-odbc/src/connection/mod.rs Re-scopes the connection module and re-exports the parser from the new module.
mssql-odbc/src/connection/connection_string_parser.rs Introduces the new single-pass, msodbcsql-compatible connection-string parser plus exhaustive unit tests.
mssql-odbc/docs/connection_string_parser.md Adds design/behavior documentation for the parser and its msodbcsql fidelity rules.

Comment thread mssql-odbc/src/connection/mod.rs Outdated
Comment thread mssql-odbc/docs/connection_string_parser.md Outdated
Comment thread mssql-odbc/docs/connection_string_parser.md
Comment thread mssql-odbc/src/connection/connection_string_parser.rs
- Replace all chars[i] indexing in parse_connection_string with a bounds-checked peek() over chars.get(), per the mssql-odbc no-panic guideline (this crate is dlopen'd via FFI where panics are fatal).
- Fix the unresolved [ConnectionParams] intra-doc link in connection/mod.rs.
- Update docs/connection_string_parser.md to point at connection_string_parser.rs instead of the old mod.rs for both the implementation and the tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread mssql-odbc/src/connection/connection_string_parser.rs
Comment thread mssql-odbc/docs/connection_string_parser.md
saurabh500 and others added 2 commits July 15, 2026 11:10
Remove the two KNOWN DIVERGENCE blocks from the malformed-token e2e test.
Direct probing of ODBC Driver 18 confirms the rewritten single-pass parser
now matches msodbcsql byte-for-byte:
- buried malformed token -> SQL_ERROR + 28000 (was accept-either)
- leading/middle/trailing separators -> no 01S00 (divergence #2 does not exist)

Add HasDiagState helper that scans all diagnostic records, since a successful
login interleaves the server's 01000 messages and drivers order the 01S00
parse warning differently (msodbcsql posts it last, mssql-odbc first).

Add ConnectionStringParserParityBehaviors e2e test (braced values, first-wins
duplicates, verbatim key matching) and a unit test locking in separator parity.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a1a34e64-f34b-4792-991d-7ba87971423d
A trailing run of 2+ separators (or a trailing ';' followed by whitespace)
makes msodbcsql's ParseAttrStr start a fresh iteration that finds a
degenerate empty key at end-of-input and posts 01S00; a single trailing
';' is clean. Our parser previously swallowed all trailing separators
silently. Move the clean end-of-input guard to before the leading-skip
loop (mirroring sqlcconn.cpp line 4299) so trailing runs fall into the
empty-key warning path.

Confirmed against ODBC Driver 18 with a unixODBC ctypes probe and the
e2e parity harness (run_e2e.sh --compare-with-msodbcsql): 13/13 tests now
pass identically on both drivers. Update the trailing-separator unit
tests, the e2e MalformedTokenReturnsSuccessWithInfo case, and the parser
docs (algorithm, state diagram, stop-conditions).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: a1a34e64-f34b-4792-991d-7ba87971423d
Comment thread mssql-odbc/src/connection/connection_string_parser.rs
@David-Engel

Copy link
Copy Markdown
Contributor

Suggestion: the new parser-parity e2e tests build their own connection strings from cfg.Server(), cfg.Uid(), and cfg.Pwd(), but the DriverConnectLiveTest fixture only checks HasConnection(). That also allows valid e2e configurations that use ODBC_TEST_CONNSTR, DSN, or integrated auth, where those individual SQL-auth fields can be empty. In those environments these tests can fail even though the suite is configured correctly.

Could we either skip the parser-parity tests unless Server, UID, and PWD are all present, or add a small helper/fixture for tests that explicitly require mutable SQL-auth field configuration?

@saurabh500

Copy link
Copy Markdown
Contributor Author

@ttk what's your advise on @David-Engel 's comment about e2e testing ?

The parser-parity tests build and corrupt SQL-auth login strings from
Server/UID/PWD, but DriverConnectLiveTest only gates on HasConnection(),
which is true for ODBC_TEST_CONNSTR, DSN, and integrated-auth configs
where UID/PWD are legitimately empty. Add ODBCTestConfig::HasSqlAuth()
and GTEST_SKIP those two tests when it is false so they no longer fail
spuriously in non-SQL-auth environments.

Tactical guard; a capability-based test framework is tracked separately.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a1a34e64-f34b-4792-991d-7ba87971423d
@saurabh500

saurabh500 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Two things:

Tactical fix (in this PR, commit f457cff): added ODBCTestConfig::HasSqlAuth() (Server && UID && PWD all present) and GTEST_SKIP() at the top of the two parser-parity tests (MalformedTokenReturnsSuccessWithInfo, ConnectionStringParserParityBehaviors). They now skip cleanly in ODBC_TEST_CONNSTR, DSN, and integrated-auth environments instead of failing on an empty-credential string. Verified in WSL/unixODBC: with SQL auth the full suite is 13/13; without UID/PWD the two tests report SKIPPED (0 failures).

Proper design (follow-up #117): a capability model on the test config (enum class ODBCCap { SqlAuth, MutableDatabase, IntegratedAuth, Dsn, Encrypt } + HasCapability() + a one-line ODBC_REQUIRE_CAP(...) skip helper), plus a narrow BuildConnectionStringNarrow() so auth-neutral parser cases append to the already-configured connection rather than rebuilding it. That lets "any connection that authenticates" tests run everywhere, while tests that need mutable SQL-auth fields declare it explicitly.

@ttk and @David-Engel since I have to automate the runs of these tests in the pipeline, I have created an issue to track the management of configurations needed for various tests. For this PR I have incorporated the fix that David suggested.

@Theekshna
Theekshna merged commit 484a6c2 into main Jul 16, 2026
17 of 18 checks passed
David-Engel added a commit that referenced this pull request Jul 16, 2026
Resolve conflict in mssql-odbc/tests/e2e/tests/driver_connect_test.cpp.
PR #107 rewrote the ODBC connection-string parser to mirror msodbcsql's
ParseAttrStr and reworked this test to use a record-scanning HasDiagState
helper (r.has01S00). That supersedes this branch's local fix for the
extra-semicolons case: the rewritten parser now emits 01S00 for a trailing
';;;' run (matching msodbcsql), and has01S00 scans every diagnostic record
so it observes the warning even though it is appended after the server's
5701/5703 login context-change info. Took main's version of both hunks.

Validated on the merged tree: mssql-odbc unit tests pass (256), and the
full C++ e2e suite passes 13/13 against a live SQL Server 2022 container
(driver_connect_test green with both the INFO-token feature and the new
parser).
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.

4 participants