Merge | SqlInternalConnectionTds #3760

benrr101 · 2025-11-12T18:43:30Z

Description

This is a big PR! If you want me to split it up, I have a couple branches I could split it into! Just reject the PR and I'll do that!

At long last, the last (non-trivial) class merge! This mainly merges the SqlInternalConnectionTds class, but because there are multiple classes in the file, they have also been split up. Each commit is a single (or small collection) of members, so it can be reviewed one-commit-at-a-time. Each commit also has notes about what changes were made to the member.

The only non-trivial change made as part of this merge is to move the smaller classes in the file to a new Microsoft.Data.SqlClient.Connection namespace. It's unclear what they're used for unless you put them in a new namespace.

Issues

More or less finishes up #1261

Testing

Projects still build. I kinda expect a handful of test failures, but we'll sort that out as they happen.

…InBytes, _accessTokenCallback, _cleanSQLDNSCaching, _currentSessionData, _sessionRecoveryAcknowledged, _fedAuthRequired, _federatedAuthenticationAcknowledged, _federatedAuthenticationInfoReceived, _federatedAuthenticationInfoRequested, _federatedAuthenticationRequested, _sspiContextProvider, _activeDirectoryAuthTimeoutRetryHelper, _credential, _fedAuthFeatureExtensionData, _fedAuthToken, _loginAck, _parser, _poolGroupProviderInfo, _recoverySessionData, _serverSupportsDNSCaching, _sessionRecoveryRequested

…Supported, IsDnsCachingBeforeRedirectSupported, IsSQLDNSRetryEnabled

…JsonSupportEnabled, IsVectorSupportEnabled, pendingSQLDNSObject, _tceVersionSupported, _dbConnectionPool, _dbConnectionPoolAuthenticationContextKey, _newDbConnectionPoolAuthenticationContext

…nContextUnLockedRefreshTimeSpan, s_transientErrors

…tPacketSize, _identity, _instanceName, _originalDatabase, _originalLanguage, _fResetConnection, CurrentSessionData

…nation, _threadIdOwningParserLock, _timeout, _timeoutErrorInternal, TimeoutErrorInternal

…ction, Identity, OriginalClientConnectionId, PendingTransaction, RoutingDestination, RoutingInfo, IsTransientError()

…e, Is2008OrNewer, IsLockedForBulkCopy, PacketSize, Parser, PoolGroupProviderInfo, ServerProcessId (made internal), ServerProvidedFailoverPartner, ReadyToPrepareTransaction # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs # src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

Move IsSqlDNSCachingSupported

(split long lines, added parameter labels to literals, replaced ad-hoc SQL with string interpolation)

(Split long lines, adopt netcore style trave event formatting, use null propagation for parser.disconnect, move comments around)

(remove redundant parantheses, split long lines, moved a couple comments)

(split long lines, flip enum comparison operands, add parameter labels for constant)

(diff denoted by #if NETFRAMEWORK, rebalance comments)

(rebalance comments, flip comparison with constant operands)

(split long lines, rebalanced comments)

(rewrote disconnectransaction into a single line)

(use `is Enum or Enum` pattern, chop argument list)

(make private, split long lines, rebalance comments)

…actionEnded (redundant override) (split long lines)

(labels for constant parameters, split long lines, adopt netcore style trace event strings)

(split long lines, rebalanced comments)

(split long lines, introduce temp variable for source type b/c the ternary was was too long to use in-line, reorder temp variable declaration order to group similar variables)

(extract exception number checks to variable, comment to xmldocs)

(resolve conflicts with #if, split long lines, rebalance comments, introduce some ternaries, nothing too crazy)

(Split long lines, flip constant comparisons, standardize the indenting)

(rebalanced comments, use typed provider info)

…ad, SyncAsyncLock.ThreadMayHaveLock (moved to property)

Copilot

Pull Request Overview

This PR completes a major refactoring by merging the platform-specific SqlInternalConnectionTds implementations into a single shared file. The main accomplishment is consolidating the last non-trivial class merge for the SqlClient codebase.

Key Changes

Merged SqlInternalConnectionTds from netfx and netcore into a single common implementation
Introduced new Microsoft.Data.SqlClient.Connection namespace for helper classes (ServerInfo, SessionData, SessionStateRecord)
Changed ThreadMayHaveLock() from method to property for consistency
Fixed spelling error in SqlException.cs
Removed stub files that were used during the merge process
Updated project files to reference the new shared implementation

Reviewed Changes

Copilot reviewed 13 out of 16 changed files in this pull request and generated 22 comments.

Show a summary per file

File	Description
SqlInternalConnectionTds.cs	New merged implementation of the core TDS connection class with 3688 lines, consolidating netfx and netcore versions
Connection/SessionData.cs	Moved to new namespace, contains session state management logic
Connection/SessionStateRecord.cs	Moved to new namespace, represents individual session state records
Connection/ServerInfo.cs	Moved to new namespace, contains server connection information
SqlException.cs	Added new overload for creating exceptions with single SqlError, fixed spelling error
TdsParser.cs	Updated using statements and property access to match API changes
TdsParserStateObject.cs	Updated property access from method call to property
SqlConnection.cs, SqlConnectionFactory.cs, SspiContextProvider.cs	Added using statement for new Connection namespace
netfx/netcore project files	Updated to reference shared files instead of platform-specific ones

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SessionData.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlException.cs

paulmedynski

Since the new SqlInternalConnectionTds.cs is 3688 lines long, most of which I assume are identical to the two previous files, it would be helpful to have the functional differences called out. There are too many commits for reviewers to walk through and try to keep track of it all IMO.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SessionData.cs

benrr101 · 2025-11-13T22:42:06Z

@paulmedynski since I called out in bold at the beginning of the PR description that the PR is big and can be split up if necessary and that if you want it smaller you should reject it, I will trust you read that and decided not to reject because you are ok with it being a very big PR. I called out code changes in the commit comments themselves, but I can scrub them again and check for "functional changes" to call out.

benrr101

Adding comments that call out less-trivial changes. There are no true "functional" changes here, just tweaks to formatting.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SessionStateRecord.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SessionData.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs

@mdaigle

* Use string.Format again for UserServerName assignment in ServerInfo.cs * Make fields readonly that were called out by copilot * Fixed typo in comment for _dbConnectionPoolAuthenticationContextKey * Restored TODO comment from @mdaigle * Added `using` for cancellation token source in the middle of active directory junk

paulmedynski

Thanks for the comments - very helpful! A couple of suggestions, but not blocking.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs

benrr101 added 30 commits November 7, 2025 11:59

Introduce partial for SqlInternalConnectionTds

67c3819

Merge accessTokenExpirationBufferTime

67848e4

Merge _dnsCachingBeforeRedirect, _SQLDNSRetryEnabled, IsSqlDnsCaching…

da2c73f

…Supported, IsDnsCachingBeforeRedirectSupported, IsSQLDNSRetryEnabled

Merging _forceMsalRetry, _forceExpiryLocked, _forceExpiryUnLocked, Is…

161f254

…JsonSupportEnabled, IsVectorSupportEnabled, pendingSQLDNSObject, _tceVersionSupported, _dbConnectionPool, _dbConnectionPoolAuthenticationContextKey, _newDbConnectionPoolAuthenticationContext

Merge _dbAuthenticationContextLockedRefreshTimeSpan, _dbAuthenticatio…

1ea8e58

…nContextUnLockedRefreshTimeSpan, s_transientErrors

Merge _asyncCommandCount, _fConnectionOpen, _currentLanguage, _curren…

73d8cb2

…tPacketSize, _identity, _instanceName, _originalDatabase, _originalLanguage, _fResetConnection, CurrentSessionData

Merge _clientConnectionId, _originalClientConnectionId, _routingDesti…

93debea

…nation, _threadIdOwningParserLock, _timeout, _timeoutErrorInternal, TimeoutErrorInternal

Merge AvailableInternalTransaction, ClientConnectionid, CurrentTransa…

d46aee8

…ction, Identity, OriginalClientConnectionId, PendingTransaction, RoutingDestination, RoutingInfo, IsTransientError()

Merge IsAccesTokenExpired, UnbindOnTransactionCompletion

5d74b87

Move IsSqlDNSCachingSupported

Merge IgnoreEnvChange, ThreadHasParserLockForClose

144a9cd

Merge ChangeDatabaseInternal

282ecee

(split long lines, added parameter labels to literals, replaced ad-hoc SQL with string interpolation)

Merge Dispose

8f9dab8

(Split long lines, adopt netcore style trave event formatting, use null propagation for parser.disconnect, move comments around)

Merge ValidateConnectionForExecute

3e0649a

(remove redundant parantheses, split long lines, moved a couple comments)

Merge CheckEnlistedTransactionBinding

89fa5ea

(split long lines, flip enum comparison operands, add parameter labels for constant)

Merge IsConnectionAlive, Activate

7deca1b

(diff denoted by #if NETFRAMEWORK, rebalance comments)

Merge InternalDeactivate

ae0f909

(rebalance comments, flip comparison with constant operands)

Merge ResetConnection

8cec5c7

(split long lines, rebalanced comments)

Merge DecrementAsyncCount, DisconnectTransaction, IncrementAsyncCount

0e089f0

(rewrote disconnectransaction into a single line)

Merge ExecuteTransaction, Remove unused ExecuteTransaction overload

f809dce

(use `is Enum or Enum` pattern, chop argument list)

Merge ExecuteTransaction2005

5f9143c

(make private, split long lines, rebalance comments)

Merge GetDTCAddress, PropagateTransactionCookie; Remove DelegateTrans…

5a73c92

…actionEnded (redundant override) (split long lines)

Merge CompleteLogin

05baf5c

(labels for constant parameters, split long lines, adopt netcore style trace event strings)

Merge Login

cea5a8b

(split long lines, rebalanced comments)

Merge LoginFailure

d94c9f2

Merge OpenLoginEnlist

842d13e

(split long lines, introduce temp variable for source type b/c the ternary was was too long to use in-line, reorder temp variable declaration order to group similar variables)

Merge IsDoNotRetryConnectError

3aeebc3

(extract exception number checks to variable, comment to xmldocs)

Merge LoginNoFailure

26d4599

(resolve conflicts with #if, split long lines, rebalance comments, introduce some ternaries, nothing too crazy)

Merge ShouldDisableTnir from netframework

8e8f760

benrr101 added 12 commits November 11, 2025 12:17

Merge OnFeatureExtAck

ee8f95e

(Split long lines, flip constant comparisons, standardize the indenting)

Merge constructor and TryReplaceConnection

484e96f

(rebalanced comments, use typed provider info)

Start merging the SyncAsyncLock

07e4ece

Merge SyncAsyncLock._semaphor, SyncAsyncLock.CanBeReleasedFromAnyThre…

55aeb40

…ad, SyncAsyncLock.ThreadMayHaveLock (moved to property)

Merge SyncAsyncLock.Release, SyncAsyncLock.Wait

ff352f0

Finishing merge of SqlInternalConnectionTds.cs, removing stub

7d887d6

Move SessionStateRecord into new Connection namespace

11f9839

Introduce partial for SessionData in Connection folder

8ac4724

Merge the SessionData class

11e1b71

Merge ServerInfo class

ef412fc

Remove (now empty) SqlInternalConnectionTds files

3f49af6

Put the little classes in the connection namespace

bef6043

benrr101 added this to the 7.0.0-preview3 milestone Nov 12, 2025

benrr101 requested a review from a team as a code owner November 12, 2025 18:43

Copilot AI review requested due to automatic review settings November 12, 2025 18:43

benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 12, 2025

Copilot started reviewing on behalf of benrr101 November 12, 2025 18:43 View session

Copilot finished reviewing on behalf of benrr101 November 12, 2025 18:44

Copilot AI reviewed Nov 12, 2025

View reviewed changes

edwardneal reviewed Nov 12, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs Outdated Show resolved Hide resolved

mdaigle requested changes Nov 12, 2025

View reviewed changes

paulmedynski reviewed Nov 13, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SessionData.cs Outdated Show resolved Hide resolved

paulmedynski self-assigned this Nov 13, 2025

benrr101 commented Nov 13, 2025

View reviewed changes

paulmedynski approved these changes Nov 14, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs Show resolved Hide resolved

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ServerInfo.cs Show resolved Hide resolved

mdaigle approved these changes Nov 14, 2025

View reviewed changes

benrr101 merged commit ad5687e into main Nov 14, 2025
265 checks passed

benrr101 deleted the dev/russellben/merge/sqlconnectiontds branch November 14, 2025 19:04

Merge | SqlInternalConnectionTds #3760

Merge | SqlInternalConnectionTds #3760

Uh oh!

Conversation

benrr101 commented Nov 12, 2025

Description

Issues

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benrr101 commented Nov 13, 2025

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants