Skip to content

Merge | DbConnectionString.Common #3256

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

Merged
merged 4 commits into from
Apr 16, 2025

Conversation

benrr101
Copy link
Contributor

@benrr101 benrr101 commented Apr 3, 2025

Description: This is a bit more heavyweight change. DbConnectionString.Common.cs contains several classes and a large collection of utility methods. I have made the following changes:

  • Created a new namespace Microsoft.Data.Common.ConnectionString to house these connection string utilities
  • Separated DbConnectionStringDefaults
  • Separated DbConnectionStringKeywords
  • Separated DbConnectionStringSynonyms
  • Renamed DbConnectionStringBuilderUtil to DbConnectionStringUtilities
  • Extracted AttestationProtocolUtilities, IpAddressPreferenceUtilities, and PoolBlockingUtilities from DbConnectionStringUtilities (these were especially called out with regions in the previous).
  • Moved all aforementioned files into the common project
  • Wired up the name changes

This also identified a lot of pointless code that just forwards method calls or duplicates enums. I will be cleaning these up in a future PR.

Testing: Stuff moved but wasn't functionally changed, no changes to public APIs. Projects still build.

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Apr 3, 2025
@benrr101 benrr101 requested review from a team and Copilot April 3, 2025 17:57
Copy link
Contributor

@Copilot 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 PR reorganizes connection string utilities by moving them into the new namespace Microsoft.Data.Common.ConnectionString and splitting out the defaults, keywords, and synonyms into dedicated files. Key changes include renaming DbConnectionStringBuilderUtil to DbConnectionStringUtilities, extracting protocol/IP address/pool blocking utilities into separate classes, and updating references across both netfx and netcore implementations.

Reviewed Changes

Copilot reviewed 20 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
TdsEnums.cs Updated usage of DbConnectionStringDefaults in SQL provider constants.
SqlUtil.cs Changed reference from DbConnectionStringBuilderUtil to DbConnectionStringUtilities.
SqlConnectionStringBuilder.cs Updated conversion methods to use the new utilities and added TODO comments regarding unnecessary methods.
SqlConnectionString.cs Replaced references from BuilderUtil to the new Utilities and added TODO comments for planned removals.
ActiveDirectoryAuthenticationProvider.cs Updated client name usage to new DbConnectionStringDefaults reference.
DbConnectionOptions.Common.cs, AdapterUtil.cs, and other common files Added new namespace usage and updated references accordingly.
New utility files (PoolBlockingUtilities.cs, IpAddressPreferenceUtilities.cs, DbConnectionStringSynonyms.cs, DbConnectionStringKeywords.cs, DbConnectionStringDefaults.cs, AttestationProtocolUtilities.cs) Introduced separated implementations for connection string defaults, keywords, synonyms, and specialized utilities.
Netfx and Netcore SqlConnection.cs files Updated error throw statements to reference the new Utilities classes.
Other minor files Added necessary adjustments for the namespace change in various files.
Files not reviewed (2)
  • src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj: Language not supported
  • src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj: Language not supported

{
SqlConnectionAttestationProtocol.AAS => nameof(SqlConnectionAttestationProtocol.AAS),
SqlConnectionAttestationProtocol.HGS => nameof(SqlConnectionAttestationProtocol.HGS),
SqlConnectionAttestationProtocol.None => nameof(SqlConnectionAttestationProtocol.None),
Copy link
Preview

Copilot AI Apr 3, 2025

Choose a reason for hiding this comment

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

The default case in the switch for AttestationProtocolToString returns null when the value is not AAS, HGS, or None. Since NotSpecified is a valid value per the IsValidAttestationProtocol check, consider returning a meaningful string (e.g., 'NotSpecified') instead of null.

Suggested change
SqlConnectionAttestationProtocol.None => nameof(SqlConnectionAttestationProtocol.None),
SqlConnectionAttestationProtocol.None => nameof(SqlConnectionAttestationProtocol.None),
SqlConnectionAttestationProtocol.NotSpecified => nameof(SqlConnectionAttestationProtocol.NotSpecified),

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@edwardneal edwardneal left a comment

Choose a reason for hiding this comment

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

There's one #if NETFRAMEWORK conditional compilation block which we can pare down - and in future, remove completely.

Besides that, I agree with your TODO comments about removing the redundant code.

Copy link

codecov bot commented Apr 7, 2025

Codecov Report

Attention: Patch coverage is 69.60000% with 114 lines in your changes missing coverage. Please review.

Project coverage is 72.80%. Comparing base (2bb0dc7) to head (74e2231).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...on/ConnectionString/DbConnectionStringUtilities.cs 66.83% 65 Missing ⚠️
...a/Common/ConnectionString/PoolBlockingUtilities.cs 52.77% 17 Missing ⚠️
...n/ConnectionString/IpAddressPreferenceUtilities.cs 60.00% 12 Missing ⚠️
...n/ConnectionString/AttestationProtocolUtilities.cs 79.59% 10 Missing ⚠️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs 66.66% 4 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs 66.66% 4 Missing ⚠️
...SqlClient/ActiveDirectoryAuthenticationProvider.cs 50.00% 1 Missing ⚠️
...rc/Microsoft/Data/SqlClient/SqlConnectionString.cs 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3256      +/-   ##
==========================================
- Coverage   72.81%   72.80%   -0.01%     
==========================================
  Files         297      301       +4     
  Lines       59661    59665       +4     
==========================================
- Hits        43444    43442       -2     
- Misses      16217    16223       +6     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.22% <69.80%> (-0.02%) ⬇️
netfx 71.45% <69.69%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paulmedynski paulmedynski self-assigned this Apr 8, 2025
@paulmedynski paulmedynski removed their assignment Apr 8, 2025
@benrr101 benrr101 merged commit 6695ecd into main Apr 16, 2025
251 checks passed
@benrr101 benrr101 deleted the dev/russellben/merge/dbconnectionstring branch April 16, 2025 16:30
@benrr101 benrr101 added this to the 6.1-preview1 milestone Apr 29, 2025
This was referenced Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants