Add Json Payload Functionality for User Agent Feature Extension #3489
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the JSON payload functionality for the User Agent feature extension in Microsoft.Data.SqlClient, adding the core infrastructure for generating and managing user agent information that will be sent to SQL Server during connection establishment.
- Implements UserAgentInfo class with OS detection, architecture detection, and runtime information gathering
- Adds UserAgentInfoDto as a serializable data transfer object with size constraints and field-dropping logic
- Includes comprehensive unit tests covering field truncation, payload sizing, and JSON serialization
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| UserAgentInfo.cs (netfx/netcore) | Core implementation with OS/runtime detection and JSON payload generation with size constraints |
| UserAgentInfoDto.cs (netfx/netcore) | Data transfer object for JSON serialization with property name constants |
| UserAgentInfoTests.cs | Unit tests covering truncation logic, payload sizing, and JSON contract validation |
| Microsoft.Data.SqlClient.csproj (netfx/netcore) | Project file updates to include the new UserAgent classes |
| Microsoft.Data.SqlClient.UnitTests.csproj | Test project configuration with empty folder reference |
Comments suppressed due to low confidence (2)
| <ItemGroup> | ||
| <Folder Include="microsoft.data.sqlclient\tests\unittests\microsoft\data\sqlclient\" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
This empty folder reference appears to be unnecessary and may have been added by mistake. Empty folder references in project files don't serve any functional purpose and should be removed to keep the project file clean.
| <ItemGroup> | |
| <Folder Include="microsoft.data.sqlclient\tests\unittests\microsoft\data\sqlclient\" /> | |
| </ItemGroup> |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The UnitTests project doesn't explicitly specify any of the source files to compile. We let the SDK figure it out. This ItemGroup shouldn't be necessary.
There was a problem hiding this comment.
Sounds good.Will update.
| // Last check to ensure we are within the limits(in case remaining fields are still too large) | ||
| return payload.Length > UserAgentPayloadMaxBytes |
There was a problem hiding this comment.
This condition check is inconsistent with the logic above. The method drops fields when payload exceeds UserAgentPayloadMaxBytes, but then checks the same condition again. This should check JsonPayloadMaxBytesSpec (2047) instead to ensure the final payload meets the server specification.
| // Last check to ensure we are within the limits(in case remaining fields are still too large) | |
| return payload.Length > UserAgentPayloadMaxBytes | |
| // Last check to ensure we are within the limits (in case remaining fields are still too large) | |
| return payload.Length > JsonPayloadMaxBytesSpec |
Copilot uses AI. Check for mistakes.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/UserAgentInfo.cs
Outdated
Show resolved
Hide resolved
|
@samsharma2700 @cheenamalhotra great to see this work happening. I've taken a look at the spec doc linked above, and could you clarify how we'd go about injecting EF information into the user agent? We'd need to identify EF Core (as opposed to direct users of SqlClient), it's version, etc. |
|
Hi @roji - There is a V2 of the design that adds a public API for middleware like EF to pass version information, but we need to go through a security and privacy review before we can implement it. This V1 phase only includes values we can pull from the runtime that don't have any such concerns. |
| <ItemGroup> | ||
| <Folder Include="microsoft.data.sqlclient\tests\unittests\microsoft\data\sqlclient\" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
The UnitTests project doesn't explicitly specify any of the source files to compile. We let the SDK figure it out. This ItemGroup shouldn't be necessary.
There was a problem hiding this comment.
These new files should live in the src/ project, and shouldn't be duplicated in netcore and netfx.
There was a problem hiding this comment.
Good point! moved to src/
| /// Gathers driver + environment info, enforces size constraints, | ||
| /// and serializes into a UTF-8 JSON payload. | ||
| /// </summary> | ||
| public static class UserAgentInfo |
There was a problem hiding this comment.
This isn't part of our public API, so should be internal.
The UnitTests project has access to internals, so shouldn't have any effect on testing.
|
@paulmedynski ok - can you clarify what's planned for the 6.1.0 (v1, v2?) |
|
The V1 work will arrive in 7.0.0. The V2 work to support EF and other middleware may also make it into that timeframe, but depends on which reviews we need and how long they take. There is also ongoing SQL Server support that we need to align with to make any of this driver side work useful. @samsharma2700 will be completing the V1 work over several PRs. We have (and will) put them into the 7.0.0-preview1 milestone. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3489 +/- ##
==========================================
- Coverage 68.85% 66.89% -1.97%
==========================================
Files 277 279 +2
Lines 62237 62316 +79
==========================================
- Hits 42854 41684 -1170
- Misses 19383 20632 +1249
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:
|
|
@paulmedynski thanks for the clarifications! |
| #if WINDOWS | ||
| using System.Management; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
#nullable enable
Add this to all new files, and we can remove it later when we enable nullable everywhere.
| using System.Management; | ||
| #endif | ||
|
|
||
| namespace Microsoft.Data.SqlClient.UserAgent |
There was a problem hiding this comment.
Prefer namespace declaration:
namespace Microsoft.Data.SqlClient.UserAgent;
Then the entire definition isn't unnecessarily indented.
There was a problem hiding this comment.
Yes agreed, the file-scoped namespace declarations do make the files cleaner. I assumed our project convention is to use traditional namespace declaration with a block, so continued with that.
| /// Maximum number of bytes allowed for the user agent json payload. | ||
| /// payloads larger than this may be rejected by the server. | ||
| /// </summary> | ||
| public const int JsonPayloadMaxBytesSpec = 2047; |
There was a problem hiding this comment.
What is the significance of the Spec suffix here? I think JsonPayloadMaxBytes is accurate enough.
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
So Spec marks the specification defined limit for the payload, but we still send the payload if it is higher than that. We drop a few fields if payload is larger than UserAgentPayloadMaxBytes. Added a suffix to distinguish them easily. Made it public for easier testing but I'll hard code this value for now.
| { | ||
| /// <summary> | ||
| /// Gathers driver + environment info, enforces size constraints, | ||
| /// and serializes into a UTF-8 JSON payload. |
There was a problem hiding this comment.
It's probably worthwhile adding a link to the spec document here.
|
|
||
| /// <summary> | ||
| /// Maximum number of bytes allowed for the user agent json payload. | ||
| /// payloads larger than this may be rejected by the server. |
| /// <param name="jsonStringVal">The string value to truncate</param> | ||
| /// <param name="maxChars">Maximum number of characters allowed</param> | ||
| /// <returns>Truncated string or default value if input is invalid</returns> | ||
| private static string TruncateOrDefault(string jsonStringVal, int maxChars) |
There was a problem hiding this comment.
private -> internal will make it easier to test.
| [InlineData(null, 5, "Unknown")] // null returns default | ||
| [InlineData("", 5, "Unknown")] // empty returns default | ||
| [InlineData("abc", 5, "abc")] // within limit unchanged | ||
| [InlineData("abcdef", 5, "abcde")] // overflow truncated |
There was a problem hiding this comment.
Test exactly max chars in the input.
| // Final payload must satisfy limits | ||
| Assert.InRange(payload.Length, 1, UserAgentInfo.UserAgentPayloadMaxBytes); | ||
|
|
||
| // Convert to string for field presence checks |
There was a problem hiding this comment.
Probably better to parse as a JsonDocument and then explictily test the fields.
There was a problem hiding this comment.
It would be nice to also see tests that prove your trunctate, adjust, and serialize all works together. You could put all of that into an internal function that takes in the field values, and then you can check the output is as expected.
| Assert.DoesNotContain(UserAgentInfoDto.OsInfo.DetailsJsonKey, json); | ||
| } | ||
|
|
||
| // 4. DTO serializes with expected JSON property names |
There was a problem hiding this comment.
What about testing that the values are serialized as expected as well?
Description
Part 2 of UserAgent work. Previous PR: #3451
Testing
Builds are running and added unit tests to verify changes.