Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes the handling of enhanced routing acknowledgment in SQL Server connections. The PR ensures that enhanced routing information (which includes a database name) is only used when the server explicitly acknowledges support for the FEATUREEXT_ENHANCEDROUTINGSUPPORT feature extension. If the server doesn't acknowledge the feature or explicitly disables it, the routing information is ignored and the connection stays with the current server.
Changes:
- Added validation logic to ignore enhanced routing info when
DatabaseNameis present butIsEnhancedRoutingSupportEnabledis false - Updated
OnFeatureExtAckto process enhanced routing feature acknowledgment even during routing - Refactored unit tests to use a helper class pattern and cover both sync and async code paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | Added enhanced routing acknowledgment validation in LoginNoFailover and LoginWithFailover methods; updated OnFeatureExtAck to allow processing of enhanced routing feature |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs | Refactored tests to use TestRoutingServers helper class; converted to Theory-based tests covering sync/async paths and multiple server behaviors |
| $"SqlInternalConnectionTds.LoginWithFailover | " + | ||
| $"Ignoring enhanced routing info because the server did not acknowledge the feature."); | ||
| RoutingInfo = null; | ||
| continue; |
There was a problem hiding this comment.
The use of continue here is inconsistent with the equivalent logic in LoginNoFailover (line 3299), which uses break. Both are functionally equivalent since RoutingInfo is set to null immediately before, but break is more explicit and clearer about the intent to exit the routing loop. Consider changing continue to break for consistency.
| continue; | |
| break; |
94e7051 to
0db8b2b
Compare
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
The base branch was changed.
96a5e18 to
28f7ef2
Compare
| { | ||
| RoutingTCPHost = "localhost", | ||
| RoutingTCPPort = (ushort)TargetServer.EndPoint.Port, | ||
| RoutingDatabaseName = RoutingDatabaseName, |
There was a problem hiding this comment.
RoutingTdsServers always send a routing or enhanced routing envchange token. They only acknowledge the feature extension based on the specific feature extension behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3973 +/- ##
==========================================
- Coverage 74.20% 64.58% -9.63%
==========================================
Files 287 282 -5
Lines 43881 66034 +22153
==========================================
+ Hits 32562 42645 +10083
- Misses 11319 23389 +12070
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:
|
Ports #3922 to main