- Notifications
You must be signed in to change notification settings - Fork311
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…al with utils in a bit)
…String namespace and renaming to sync class/file name.
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 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), |
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.
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.
| SqlConnectionAttestationProtocol.None=> nameof(SqlConnectionAttestationProtocol.None), | |
| SqlConnectionAttestationProtocol.None=> nameof(SqlConnectionAttestationProtocol.None), | |
| SqlConnectionAttestationProtocol.NotSpecified=> nameof(SqlConnectionAttestationProtocol.NotSpecified), |
Copilot uses AI. Check for mistakes.
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.
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.
...soft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringKeywords.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
codecovbot commentedApr 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6695ecd intomainUh oh!
There was an error while loading.Please reload this page.
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:
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.