-
Notifications
You must be signed in to change notification settings - Fork 30
FIX: Update escaping rules in connection string parser and builder #364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 67.1%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.pybind.ddbc_bindings.h: 76.9%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 82.5%
mssql_python.cursor.py: 83.8%🔗 Quick Links
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the ODBC connection string escaping rules in the parser and builder to correctly align with the MS-ODBCSTR specification. The key issue was that the code was treating {{ as an escape sequence for {, when in fact only closing braces } need escaping (as }}). Opening braces { should be kept as literal characters within braced values.
Key Changes:
- Updated parser to not treat
{{as an escape sequence, keeping it as literal{{ - Updated builder to only escape closing braces
}→}}, removed escaping of opening braces - Updated all affected tests to reflect the corrected behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mssql_python/connection_string_parser.py |
Removed logic that treated {{ as an escape sequence in _parse_braced_value(), updated comments and docstrings to clarify only } needs escaping |
mssql_python/connection_string_builder.py |
Removed .replace("{", "{{") from _escape_value(), updated docstrings and examples to reflect only closing braces need escaping |
tests/test_010_connection_string_parser.py |
Updated test assertions and docstrings to expect {{ as literal characters rather than escaped {, covering multiple test cases including edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jahnvi480
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bewithgaurav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Waiting for alpine build issues to be solved which is a blocker for merge. Coordination with odbc team in progress |
Work Item / Issue Reference
Summary
FIX: The escaping rules for the parser and the builder
The parser was treating {{ as an escaped string and was parsing it as { and then the builder was escaping { to {{
However the ODBC driver doesn't require escaping of the opening brace, if its already wrapped in {}. Only closing brace values inside curlies require escaping. Hence a literal pw}d needs to be sent as {pw}}d}
Fixed the parser and the builder to take care of this nuance.
Fixes #363