Add Feature Extension for User Agent#3451
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds initial support for the User Agent feature extension by defining new constants, extending the TDS writer with a method to serialize the user-agent payload, and toggling the feature flag in the login flow under debug builds.
- Introduce
FEATUREEXT_USERAGENTandSUPPORTED_USER_AGENT_VERSIONconstants and enum entry - Add
WriteUserAgentFeatureRequestin both netfx and netcore TdsParser implementations - Add
IsUserAgentEnabledflag and debug-only enabling logic in SqlInternalConnectionTds
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs | Added user-agent feature ID constant and enum mapping |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs | Implemented WriteUserAgentFeatureRequest |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs | Implemented WriteUserAgentFeatureRequest |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | Added IsUserAgentEnabled flag and debug-only gating |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | Added IsUserAgentEnabled flag and debug-only gating |
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs:8715
- The new
WriteUserAgentFeatureRequestmethod lacks accompanying unit tests; please add tests to verify both length calculation and correct byte output in write and length-only modes.
internal int WriteUserAgentFeatureRequest(byte[] userAgentJsonPayload,
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
Show resolved
Hide resolved
apoorvdeshmukh
left a comment
There was a problem hiding this comment.
We should add a test case validating the login request. You can refer to TestConnWithVectorFeatExtVersionNegotiation to check how to inspect login packet.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3451 +/- ##
==========================================
+ Coverage 66.91% 67.59% +0.68%
==========================================
Files 280 280
Lines 62386 62417 +31
==========================================
+ Hits 41745 42191 +446
+ Misses 20641 20226 -415
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/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Show resolved
Hide resolved
paulmedynski
left a comment
There was a problem hiding this comment.
Let's get either some unit tests added (that don't require the test TDS server), or some functional tets that use TDS server. Either way, the tests should verify that we produce the correct Login payload given various byte arrays, and that we handle a variety of Login Response payloads.
paulmedynski
left a comment
There was a problem hiding this comment.
Looks good, with some minor tweaks.
Description
Adding support for User Agent Feature Extension(part 1) according to design doc here(https://microsoft.sharepoint-df.com/:w:/t/sqldevx/ERIWTt0zlCxLroNHyaPlKYwBI_LNSff6iy_wXZ8xX6nctQ?e=iP8q75):
Part 2 will include the
userAgentJsonPayloadrelated implementations followed by functional tests.Testing
Builds are passing.
Unit tests are passing.