Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Enhance TDS Login ClientInterfaceName field with client info - #3149

#3149
Merged
paulmedynski merged 6 commits into
maindotnet/SqlClient:mainfrom
dev/paul/driver-name-telemetrydotnet/SqlClient:dev/paul/driver-name-telemetryCopy head branch name to clipboard
Mar 4, 2025
Merged

Enhance TDS Login ClientInterfaceName field with client info#3149
paulmedynski merged 6 commits into
maindotnet/SqlClient:mainfrom
dev/paul/driver-name-telemetrydotnet/SqlClient:dev/paul/driver-name-telemetryCopy head branch name to clipboard

Conversation

@paulmedynski

@paulmedynski paulmedynski commented Feb 10, 2025

Copy link
Copy Markdown
Contributor

Spec is here: https://microsoft-my.sharepoint.com/:w:/p/davidengel/EVybTpCmdQZGixv9r-qzXYQBl-Jp4HH1GGAE6oWxh0DqhQ?e=d0b0uP

  • Added ClientInterface class that knows how to build a suitable TDS Login ClientInterfaceName value.
  • Added tests for ClientInterface.
  • Removed unnecessary SQL_PROVIDER_NAME constant.

@paulmedynski

Copy link
Copy Markdown
Contributor Author

@dotnet-policy-service agree company="Microsoft"

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated
@codecov

codecov Bot commented Feb 11, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 89.47368% with 8 lines in your changes missing coverage. Please review.

Project coverage is 72.80%. Comparing base (d73bc16) to head (5e60e0f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nt/src/Microsoft/Data/SqlClient/ClientInterface.cs 89.04% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3149      +/-   ##
==========================================
- Coverage   72.93%   72.80%   -0.14%     
==========================================
  Files         287      288       +1     
  Lines       59173    59246      +73     
==========================================
- Hits        43160    43136      -24     
- Misses      16013    16110      +97     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.51% <89.47%> (-0.07%) ⬇️
netfx 71.13% <86.48%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulmedynski
paulmedynski marked this pull request as ready for review February 11, 2025 20:06
@paulmedynski
paulmedynski requested a review from a team February 11, 2025 20:06
@paulmedynski paulmedynski added this to the 6.0.2 milestone Feb 13, 2025

@mdaigle mdaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated

@saurabh500 saurabh500 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments. Functionality looks good.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ClientInterface.cs Outdated
@paulmedynski

Copy link
Copy Markdown
Contributor Author

The spec has moved here:

https://microsoft.sharepoint-df.com/:w:/t/sqldevx/EZYUHqdnoXhKoQv0kxcM1QUB2V_uTCWN9GHfrDQY2tojmA?e=tf4xco

The OS Info and RuntimeInfo fields have been simplified to have a max length of 44 Unicode characters each, with no efforts to share the remaining space. The latest commit matches that.

…face class.

- Added tests for ClientInterface.
- Reverted changes to Application Name.
- Removed unnecessary SQP_PROVIDER_NAME constant.
- Addressed review comments:
  - Converted type/member documentation to C# XML style.
  - Added constants for OS types and max lengths.
  - Renamed Trunc() to Truncate().
  - Added copyright headers.
- Updated Driver Name and Arch max lengths per updated spec.
- Avoided Architecture test that doesn't compile in .NET Framework.
- Removed length sharing logic for OS and Runtime Info.
- Replaced with simple max len of 44 for each field.
@paulmedynski
paulmedynski force-pushed the dev/paul/driver-name-telemetry branch from 921f70b to cf328d1 Compare March 4, 2025 13:40
- Added a unit test to check the Client Interface Name the driver sends in the LOGIN7 packet.
@paulmedynski
paulmedynski merged commit 3f4e486 into main Mar 4, 2025
@paulmedynski
paulmedynski deleted the dev/paul/driver-name-telemetry branch March 4, 2025 20:33
paulmedynski added a commit that referenced this pull request Mar 5, 2025
- Moved logic to build client interface name into its own ClientInterface class.
- Added tests for ClientInterface.
- Reverted changes to Application Name.
- Removed unnecessary SQP_PROVIDER_NAME constant.
cheenamalhotra pushed a commit that referenced this pull request Mar 5, 2025
…ent info (#3200)

* Enhance TDS Login ClientInterfaceName field with client info (#3149)

- Moved logic to build client interface name into its own ClientInterface class.
- Added tests for ClientInterface.
- Reverted changes to Application Name.
- Removed unnecessary SQP_PROVIDER_NAME constant.

* User Story 33291: Enhance client interface value sent with login

- Fixed vulnerable transitive dependencies.

* User Story 33291: Enhance client interface value sent with login

- Removed duplicate System.Text.Json version.
@cheenamalhotra cheenamalhotra removed this from the 7.0-preview1 milestone Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.