-
Notifications
You must be signed in to change notification settings - Fork 311
Enable User Agent Extension #3530
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.
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
base: main
Are you sure you want to change the base?
Conversation
…eat_jsonuagent
…ient into sam_test_uagent
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few problems to address.
@@ -3072,6 +3069,25 @@ internal void OnFeatureExtAck(int featureId, byte[] data) | |||
break; | |||
} | |||
|
|||
case TdsEnums.FEATUREEXT_USERAGENT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be no ack defined in the TDS spec, so we shouldn't do this. Just let the default case handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ignoring these UserAgent files. I assume they are identical to the previous PR. If not, please call out the differences, or rebase this PR off of the previous PR so the diffs are automatic.
@@ -191,8 +192,17 @@ internal void TdsLogin( | |||
} | |||
|
|||
int feOffset = length; | |||
// TODO: User Agent Json Payload will go here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this?
Assert.True(loginFound, "Expected UserAgent extension in LOGIN7"); | ||
|
||
// Verify server never acknowledged it | ||
Assert.False(responseFound, "Server should not acknowledge UserAgent"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just testing that our Test Server doesn't sent an ACK, which we already know - we wrote it. We need a test that verifies driver behaviour when the server does send an ACK. Maybe that's already covered elsewhere in general feature ext ack tests. Can you confirm?
@@ -93,5 +93,11 @@ public interface ITDSServerSession | |||
/// Indicates whether the client supports Vector column type | |||
/// </summary> | |||
bool IsVectorSupportEnabled { get; set; } | |||
|
|||
/// <summary> | |||
/// Indicates whether the client supports Vector column type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-pasta comment :)
/// <summary> | ||
/// Default feature extension version supported on the server for user agent. | ||
/// </summary> | ||
public const byte DefaultSupportedUserAgentFeatureExtVersion = 0x0F; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x0F is the feature identifier, not its version. Did you mean 0x01 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring this file as well.
What's a user agent and what can users do with them? |
@Wraith2 I think it is similar to the User Agent string submitted by a browser, and can be used for telemetry from the server side. |
We're using the term |
Description
This is the final PR(part 3) for User Agent Feature Extension work. Making this a draft since we want few changes on part 2 : #3489
This PR will be updated once part 2 is merged.
Majority of changes in this PR include:
Testing
Builds are passing. Added unit tests and functional tests