Skip to content

Enable User Agent Extension #3606

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Enable User Agent Extension #3606

wants to merge 13 commits into from

Conversation

samsharma2700
Copy link
Contributor

@samsharma2700 samsharma2700 commented Sep 10, 2025 •
edited
Loading

Description

Part 3 for User Agent Feature Extension work. Following are added in this PR:-

  • Enabling the pre-calculated JsonPayload to be added to the FE data.
  • Adding case handling logic in case of a server ack.
  • Changes to test server to force emit Ack for this feature(testing purposes).
  • Two new tests to verify both Acked and Unacked pathways work.

Part 2 : #3451
Part 1 : #3582

Unit tests and functional tests are passing

@Copilot Copilot AI review requested due to automatic review settings September 10, 2025 00:14
@samsharma2700 samsharma2700 requested a review from a team as a code owner September 10, 2025 00:14
Copilot
Copilot AI reviewed Sep 10, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a 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 implements Part 3 of the User Agent Feature Extension work, adding the ability to send User Agent information in TDS login packets and handle server acknowledgments. The implementation includes TDS protocol support for the new feature extension and test coverage to verify the functionality.

  • Adds UserAgentSupport feature ID to TDS protocol and enables sending User Agent JSON payload during login
  • Updates both .NET Framework and .NET Core implementations to include User Agent data in feature extension requests
  • Implements server acknowledgment handling with appropriate logging when unexpected ACKs are received

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS/TDSFeatureID.cs Adds UserAgentSupport feature ID enum value
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTDSServerSession.cs Adds IsUserAgentSupportEnabled property to server session
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTDSServer.cs Implements User Agent feature extension handling in test server
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/ITDSServerSession.cs Adds User Agent support interface definition
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs Adds tests for User Agent feature extension behavior with and without server ACK
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs Updates TdsLogin to include User Agent payload in feature extension data
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs Implements User Agent feature request writing for .NET Framework
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs Adds User Agent ACK handling and property name correction
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs Implements User Agent feature request writing for .NET Core
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs Adds User Agent ACK handling and property name correction

@paulmedynski paulmedynski changed the title Dev/samsharma2700/user agent fe Enable User Agent Extension Sep 10, 2025
@paulmedynski paulmedynski mentioned this pull request Sep 10, 2025
Closed
paulmedynski
paulmedynski requested changes Sep 10, 2025
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few unused/unnecessary fields to remove, and need to add some checks to the tests.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
int feOffset = length;
// calculate and reserve the required bytes for the featureEx
length = ApplyFeatureExData(requestedFeatures, recoverySessionData, fedAuthFeatureExtensionData, useFeatureExt, length);
length = ApplyFeatureExData(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an observation - nothing for you to address here right now.

This approach of pre-calculating the packet length is inefficient. We're making 2 passes over the data to be written. Instead, we should be writing everything to the buffer once, leaving a hole where the header length goes, calculating the length as we write the data, and then going back at the end to fill in the header's length field.

I wonder if we're similarly inefficient for the other TDS packets/messages we write?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave a comment here so that we can follow up and investigate a bit later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been addressed?

@paulmedynski paulmedynski added this to the 7.0-preview1 milestone Sep 10, 2025
@apoorvdeshmukh apoorvdeshmukh self-assigned this Sep 11, 2025
paulmedynski
paulmedynski requested changes Sep 11, 2025
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still some cleanup to be done - mostly in the tests.

Also, we need to change the USERAGENT feature extension value from 0x0F to 0x10. Please update that value.

@paulmedynski paulmedynski self-assigned this Sep 11, 2025
paulmedynski
paulmedynski approved these changes Sep 12, 2025
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay - all good!

cheenamalhotra
cheenamalhotra reviewed Sep 25, 2025
View reviewed changes
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTDSServer.cs
}

// If tests request it, force an ACK for UserAgentSupport with no negotiation
if (EmitUserAgentFeatureExtAck)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rewrite this large method to split these conditional blocks into individual methods?

cheenamalhotra
cheenamalhotra reviewed Sep 25, 2025
View reviewed changes
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTDSServer.cs
}

// If tests request it, force an ACK for UserAgentSupport with no negotiation
if (EmitUserAgentFeatureExtAck)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you not be reading session instance value here as done above for other feature ext tokens?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheenamalhotra cheenamalhotra cheenamalhotra left review comments

@paulmedynski paulmedynski paulmedynski approved these changes

Copilot code review Copilot Copilot left review comments

@apoorvdeshmukh apoorvdeshmukh Awaiting requested review from apoorvdeshmukh

At least 2 approving reviews are required to merge this pull request.

Assignees

@apoorvdeshmukh apoorvdeshmukh

@paulmedynski paulmedynski

Labels
None yet
Projects
None yet
Milestone
7.0-preview2
Development

Successfully merging this pull request may close these issues.
4 participants
@samsharma2700 @apoorvdeshmukh @cheenamalhotra @paulmedynski