-
Notifications
You must be signed in to change notification settings - Fork 317
Cleanup | SqlInternalConnection properties #3743
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
Conversation
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
Show resolved
Hide resolved
...lient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs
Show resolved
Hide resolved
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 refactors the SqlInternalConnection class to improve code organization, maintainability, and consistency. The key changes include extracting the TransactionRequest enum to its own file, consolidating property definitions with better documentation, and standardizing naming conventions.
- Extracts
TransactionRequestenum from nested type to standalone file - Refactors
SqlInternalConnectionproperties with comprehensive XML documentation - Standardizes casing for property names (
PromotedDTCToken→PromotedDtcToken,IsAzureSQLConnection→IsAzureSqlConnection)
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| TransactionRequest.cs | New standalone enum file for transaction request types |
| SqlInternalTransaction.cs | Updated to use standalone TransactionRequest enum |
| SqlInternalConnection.cs | Major refactoring with improved property organization, XML docs, and simplified expressions |
| SqlDelegatedTransaction.cs | Updated references to TransactionRequest and PromotedDtcToken |
| WaitHandleDbConnectionPool.cs | Replaced IsNonPoolableTransactionRoot with explicit logic |
| DbConnectionInternal.cs | Removed IsNonPoolableTransactionRoot property |
| SqlInternalConnectionTds.cs (netfx & netcore) | Removed override of IsNonPoolableTransactionRoot, updated property references |
| Microsoft.Data.SqlClient.csproj (netfx & netcore) | Added TransactionRequest.cs to compilation |
| ChannelDbConnectionPoolTest.cs | Added ActiveIssue attribute to flaky test |
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs:1500
- These 'if' statements can be combined.
if (!IsAzureSqlConnection)
{
// If not a connection to Azure SQL, Readonly with FailoverPartner is not supported
if (ConnectionOptions.ApplicationIntent == ApplicationIntent.ReadOnly)
{
if (!string.IsNullOrEmpty(ConnectionOptions.FailoverPartner))
{
throw SQL.ROR_FailoverNotSupportedConnString();
}
if (ServerProvidedFailoverPartner != null)
{
throw SQL.ROR_FailoverNotSupportedServer(this);
}
}
}
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs:1509
- These 'if' statements can be combined.
if (!IsAzureSqlConnection)
{
// If not a connection to Azure SQL, Readonly with FailoverPartner is not supported
if (ConnectionOptions.ApplicationIntent == ApplicationIntent.ReadOnly)
{
if (!string.IsNullOrEmpty(ConnectionOptions.FailoverPartner))
{
throw SQL.ROR_FailoverNotSupportedConnString();
}
if (ServerProvidedFailoverPartner != null)
{
throw SQL.ROR_FailoverNotSupportedServer(this);
}
}
}
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TransactionRequest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3743 +/- ##
==========================================
- Coverage 76.78% 76.58% -0.20%
==========================================
Files 273 272 -1
Lines 44914 44177 -737
==========================================
- Hits 34487 33833 -654
+ Misses 10427 10344 -83
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:
|
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TransactionRequest.cs
Show resolved
Hide resolved
benrr101
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.
Looks good! I appreciate the changes!
Description
Cleaned up properties in SqlInternalConnection to make them auto-properties, add summary doc comments, and remove consideration for old SQL Server versions (where applicable).
Should be reviewed commit by commit.
Testing
No logic changes. Existing tests should cover everything.