-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[dotnet] Serialize command parameters directly to UTF-8 #15151
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,13 +101,29 @@ public string ParametersAsJsonString | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Serializes the parameters of the comand to JSON as UTF-8 bytes. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public byte[] GetParametersAsUtf8Bytes() | ||
| { | ||
| if (this.Parameters != null && this.Parameters.Count > 0) | ||
| { | ||
| return JsonSerializer.SerializeToUtf8Bytes(this.Parameters, s_jsonSerializerOptions); | ||
| } | ||
| else | ||
| { | ||
| return "{}"u8.ToArray(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns a string of the Command object | ||
| /// </summary> | ||
| /// <returns>A string representation of the Command Object</returns> | ||
| public override string ToString() | ||
| { | ||
| return string.Concat("[", this.SessionId, "]: ", this.Name, " ", this.ParametersAsJsonString); | ||
| return $"[{this.SessionId}]: {this.Name} {this.ParametersAsJsonString}"; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sidenote: |
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
|
|
||
| #nullable enable | ||
|
|
||
|
|
@@ -29,6 +30,8 @@ namespace OpenQA.Selenium.Remote | |
| /// </summary> | ||
| public class SendingRemoteHttpRequestEventArgs : EventArgs | ||
| { | ||
| private string? _requestBody; | ||
| private readonly byte[]? _utf8RequestBody; | ||
| private readonly Dictionary<string, string> headers = new Dictionary<string, string>(); | ||
|
|
||
| /// <summary> | ||
|
|
@@ -42,7 +45,21 @@ public SendingRemoteHttpRequestEventArgs(string method, string fullUrl, string? | |
| { | ||
| this.Method = method ?? throw new ArgumentNullException(nameof(method)); | ||
| this.FullUrl = fullUrl ?? throw new ArgumentNullException(nameof(fullUrl)); | ||
| this.RequestBody = requestBody; | ||
| _utf8RequestBody = requestBody is null ? null : Encoding.UTF8.GetBytes(requestBody); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old constructor kept around for backwards compatibility, but unused by our code. |
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="SendingRemoteHttpRequestEventArgs"/> class. | ||
| /// </summary> | ||
| /// <param name="method">The HTTP method of the request being sent.</param> | ||
| /// <param name="fullUrl">The full URL of the request being sent.</param> | ||
| /// <param name="requestBody">The body of the request.</param> | ||
| /// <exception cref="ArgumentNullException">If <paramref name="method"/>, <paramref name="fullUrl"/> are null.</exception> | ||
| public SendingRemoteHttpRequestEventArgs(string method, string fullUrl, byte[]? requestBody) | ||
| { | ||
| this.Method = method ?? throw new ArgumentNullException(nameof(method)); | ||
| this.FullUrl = fullUrl ?? throw new ArgumentNullException(nameof(fullUrl)); | ||
| _utf8RequestBody = requestBody; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -58,7 +75,18 @@ public SendingRemoteHttpRequestEventArgs(string method, string fullUrl, string? | |
| /// <summary> | ||
| /// Gets the body of the HTTP request as a string. | ||
| /// </summary> | ||
| public string? RequestBody { get; } | ||
| public string? RequestBody | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This event args seems to be an obscure mechanism to add headers to requests. I don't know how many people are accessing the For this reason, I feel that the added complexity is warranted: We will not decode our data into a |
||
| { | ||
| get | ||
| { | ||
| if (_requestBody is not null) | ||
| { | ||
| return _requestBody; | ||
| } | ||
|
|
||
| return _requestBody = _utf8RequestBody is null ? null : Encoding.UTF8.GetString(_utf8RequestBody); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a read-only dictionary of the headers of the HTTP request. | ||
|
|
||
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.
Commandis an abstraction. Command to bytes... we should mentionjsonin method name. LikeGetParametersAsJsoonUtf8Bytes()