Skip to content

FIX: Avoid parsing connection string multiple times in auth path (#580)#590

Open
jahnvi480 wants to merge 1 commit into
mainfrom
jahnvi/auth_conn_string_parsing
Open

FIX: Avoid parsing connection string multiple times in auth path (#580)#590
jahnvi480 wants to merge 1 commit into
mainfrom
jahnvi/auth_conn_string_parsing

Conversation

@jahnvi480
Copy link
Copy Markdown
Contributor

@jahnvi480 jahnvi480 commented May 19, 2026

Work Item / Issue Reference

AB#45135

GitHub Issue: #580


Summary

This pull request significantly refactors the authentication handling and connection string construction in the mssql_python package. The main improvements include migrating from string-based to dictionary-based parameter handling, consolidating authentication logic, and simplifying sensitive parameter removal. These changes modernize the codebase, reduce redundant parsing, and make it easier to extend or maintain authentication features.

Authentication and connection string processing improvements:

  • Refactored authentication parameter extraction and processing to operate directly on parsed parameter dictionaries instead of raw connection string fragments, reducing multiple parsing passes and improving clarity (mssql_python/auth.py, mssql_python/connection.py).
  • Consolidated and simplified the removal of sensitive parameters (such as UID, PWD, Trusted_Connection, and Authentication) by using a canonical set and dictionary operations, ensuring robust sanitization before ODBC handoff (mssql_python/auth.py, mssql_python/connection.py).

API and codebase modernization:

  • Changed Connection._construct_connection_string to return both the constructed string and the normalized parameter dictionary, enabling downstream logic to avoid reparsing and to use normalized parameters directly (mssql_python/connection.py).
  • Updated tests to accommodate the new return type of _construct_connection_string, ensuring continued correctness and coverage (tests/test_003_connection.py).

Cleanup and removal of legacy code:

  • Removed old, string-based authentication helpers and replaced them with dictionary-based logic, reducing code duplication and improving maintainability (mssql_python/auth.py, mssql_python/connection.py, tests/test_008_auth.py).

Overall, these changes streamline authentication handling, reduce parsing complexity, and lay a stronger foundation for future enhancements.

Copilot AI review requested due to automatic review settings May 19, 2026 08:42
@github-actions github-actions Bot added the pr-size: medium Moderate update size label May 19, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the Entra ID auth handling path so the connection string is parsed only once. Connection._construct_connection_string now returns both the built string and the normalized parameter dict, which is then passed directly to the auth helpers; process_connection_string (and the duplicate auth parsing it contained) is removed.

Changes:

  • _construct_connection_string returns (conn_str, normalized_params); Connection.__init__ reuses the parsed dict for auth handling instead of regex-checking and re-parsing the string.
  • auth.py is reworked to operate on dicts: process_auth_parameters and extract_auth_type take a parsed dict, remove_sensitive_params filters a dict via the new _SENSITIVE_KEYS, and _AUTH_TYPE_MAP centralizes the auth value→short-name mapping. process_connection_string is removed.
  • Tests updated to the new dict-based signatures; the obsolete TestProcessConnectionString* suites and the empty/no-equals-sign edge cases are dropped in favor of dict-input edge cases.

Reviewed changes

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

File Description
mssql_python/auth.py Replaces string-list APIs with dict-based ones; removes process_connection_string; adds _SENSITIVE_KEYS and _AUTH_TYPE_MAP.
mssql_python/connection.py Uses the parsed param dict for auth handling; switches _construct_connection_string to return a tuple and builds the sanitized string via _ConnectionStringBuilder.
tests/test_003_connection.py Updates _construct_connection_string call sites to unpack the new tuple.
tests/test_008_auth.py Migrates tests to dict-based APIs; removes process_connection_string tests and obsolete edge cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 19, 2026

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

25%


📈 Total Lines Covered: 6998 out of 27114
📁 Project: mssql-python


Diff Coverage

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

  • mssql_python/auth.py (100%)
  • mssql_python/connection.py (100%)

Summary

  • Total: 37 lines
  • Missing: 0 lines
  • Coverage: 100%

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.build._deps.simdutf-src.src.haswell.implementation.cpp: 0.4%
mssql_python.pybind.build._deps.simdutf-src.src.implementation.cpp: 6.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.implementation.h: 10.4%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.scalar.utf16_to_utf8.utf16_to_utf8.h: 25.3%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.internal.isadetection.h: 65.3%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 74.2%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@jahnvi480 jahnvi480 force-pushed the jahnvi/auth_conn_string_parsing branch from a18953c to b476518 Compare May 22, 2026 04:49
@github-actions github-actions Bot added pr-size: large Substantial code update and removed pr-size: medium Moderate update size labels May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants