Skip to content

Conversation

@jahnvi480
Copy link
Contributor

@jahnvi480 jahnvi480 commented Dec 10, 2025

Work Item / Issue Reference

AB#40945

GitHub Issue: #362


Summary

This pull request updates the logic for filtering sensitive parameters in the authentication module. The main change is that the function now removes Trusted_Connection instead of Encrypt and TrustServerCertificate, and the corresponding test has been updated to reflect this new behavior.

Sensitive parameter filtering update:

  • In mssql_python/auth.py, the remove_sensitive_params function now excludes trusted_connection instead of encrypt and trustservercertificate when filtering parameters.

Test updates for new filtering logic:

  • In tests/test_008_auth.py, the test for remove_sensitive_params has been updated to expect that Encrypt and TrustServerCertificate are no longer removed, while Trusted_Connection is now excluded.

Copilot AI review requested due to automatic review settings December 10, 2025 11:31
@github-actions github-actions bot added the pr-size: small Minimal code update label Dec 10, 2025
Copy link
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

This pull request fixes the connection string parameter filtering logic for Azure AD token-based authentication. The change correctly identifies that Trusted_Connection (Windows integrated authentication) conflicts with token-based auth and should be removed, while Encrypt and TrustServerCertificate (connection security settings) do not conflict and should be retained in the connection string.

Key Changes:

  • Updated remove_sensitive_params function to remove Trusted_Connection instead of encryption-related parameters
  • Modified the test to verify the new filtering behavior

Reviewed changes

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

File Description
mssql_python/auth.py Updated remove_sensitive_params to exclude trusted_connection instead of encrypt and trustservercertificate from the authentication filter list
tests/test_008_auth.py Updated test to include Trusted_Connection parameter and verify it's filtered out while encryption parameters are retained

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

@github-actions
Copy link

github-actions bot commented Dec 10, 2025

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

75%


📈 Total Lines Covered: 5255 out of 7003
📁 Project: mssql-python


Diff Coverage

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

No lines with coverage information in this diff.


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 66.3%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.7%
mssql_python.cursor.py: 84.3%
mssql_python.logging.py: 85.3%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

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

LGTM. Quick change.

@jahnvi480 jahnvi480 merged commit fb7b5c3 into main Dec 10, 2025
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: small Minimal code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants