Skip to content

Commit

Permalink
[dotnet] Move Response constructors towards immutability (#14998)
Browse files Browse the repository at this point in the history 
Moves us towards immutability of the Response type. Eventually, we may be able to remove the setters from some values, but that's not crucial.

This also moves us towards aspirational goals of storing a JsonNode for the Value alongside the de-serialized object, which would help with bugs such as #14846 and many others involving incorrect de-serialization, especially with .ToString() invocations on dictionaries/lists, and as casts when the type is not a match.

---------

Co-authored-by: Nikolay Borisenko <[email protected]>
  • Loading branch information
@RenderMichael @nvborisenko
RenderMichael and nvborisenko authored Jan 3, 2025
1 parent ec6e817 commit 1f1d6b9
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 51 deletions.
11 changes: 5 additions & 6 deletions dotnet/src/webdriver/Remote/HttpCommandExecutor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ private async Task<HttpResponseInfo> MakeHttpRequest(HttpRequestInfo requestInfo

private Response CreateResponse(HttpResponseInfo responseInfo)
{
Response response = new Response();
Response response;
string body = responseInfo.Body;
if ((int)responseInfo.StatusCode < 200 || (int)responseInfo.StatusCode > 299)
{
Expand All @@ -326,8 +326,7 @@ private Response CreateResponse(HttpResponseInfo responseInfo)
}
else
{
response.Status = WebDriverResult.UnknownError;
response.Value = body;
response = new Response(sessionId: null, body, WebDriverResult.UnknownError);
}
}
else if (responseInfo.ContentType != null && responseInfo.ContentType.StartsWith(JsonMimeType, StringComparison.OrdinalIgnoreCase))
Expand All @@ -336,12 +335,12 @@ private Response CreateResponse(HttpResponseInfo responseInfo)
}
else
{
response.Value = body;
response = new Response(sessionId: null, body, WebDriverResult.Success);
}

if (response.Value is string)
if (response.Value is string valueString)
{
response.Value = ((string)response.Value).Replace("\r\n", "\n").Replace("\n", Environment.NewLine);
response.Value = valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine);
}

return response;
Expand Down
99 changes: 54 additions & 45 deletions dotnet/src/webdriver/Response.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
using System.Text.Json;
using System.Text.Json.Serialization;

#nullable enable

namespace OpenQA.Selenium
{
/// <summary>
Expand All @@ -48,88 +50,98 @@ public Response()
/// Initializes a new instance of the <see cref="Response"/> class
/// </summary>
/// <param name="sessionId">Session ID in use</param>
public Response(SessionId sessionId)
public Response(SessionId? sessionId)
{
if (sessionId != null)
{
this.SessionId = sessionId.ToString();
}
this.SessionId = sessionId?.ToString();
}

/// <summary>
/// Initializes a new instance of the <see cref="Response"/> class
/// </summary>
/// <param name="sessionId">The Session ID in use, if any.</param>
/// <param name="value">The JSON payload of the response.</param>
/// <param name="status">The WebDriver result status of the response.</param>
public Response(string? sessionId, object? value, WebDriverResult status)
{
this.SessionId = sessionId;
this.Value = value;
this.Status = status;
}

/// <summary>
/// Returns a new <see cref="Response"/> from a JSON-encoded string.
/// </summary>
/// <param name="value">The JSON string to deserialize into a <see cref="Response"/>.</param>
/// <returns>A <see cref="Response"/> object described by the JSON string.</returns>
/// <exception cref="ArgumentNullException">If <paramref name="value"/> is <see langword="null"/>.</exception>
/// <exception cref="JsonException">If <paramref name="value"/> is not a valid JSON object.</exception>
public static Response FromJson(string value)
{
Dictionary<string, object> rawResponse = JsonSerializer.Deserialize<Dictionary<string, object>>(value, s_jsonSerializerOptions)
Dictionary<string, object?> rawResponse = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, s_jsonSerializerOptions)
?? throw new WebDriverException("JSON success response returned \"null\" value");

var response = new Response();
object? contents;
string? sessionId = null;

if (rawResponse.ContainsKey("sessionId"))
if (rawResponse.TryGetValue("sessionId", out object? s) && s is not null)
{
if (rawResponse["sessionId"] != null)
{
response.SessionId = rawResponse["sessionId"].ToString();
}
sessionId = s.ToString();
}

if (rawResponse.TryGetValue("value", out object valueObj))
if (rawResponse.TryGetValue("value", out object? valueObj))
{
response.Value = valueObj;
contents = valueObj;
}

// If the returned object does *not* have a "value" property
// the response value should be the entirety of the response.
// TODO: Remove this if statement altogether; there should
// never be a spec-compliant response that does not contain a
// value property.
if (!rawResponse.ContainsKey("value") && response.Value == null)
else
{
// If the returned object does *not* have a "value" property
// the response value should be the entirety of the response.
// TODO: Remove this if statement altogether; there should
// never be a spec-compliant response that does not contain a
// value property.

// Special-case for the new session command, where the "capabilities"
// property of the response is the actual value we're interested in.
if (rawResponse.ContainsKey("capabilities"))
if (rawResponse.TryGetValue("capabilities", out object? capabilities))
{
response.Value = rawResponse["capabilities"];
contents = capabilities;
}
else
{
response.Value = rawResponse;
contents = rawResponse;
}
}

if (response.Value is Dictionary<string, object> valueDictionary)
if (contents is Dictionary<string, object?> valueDictionary)
{
// Special case code for the new session command. If the response contains
// sessionId and capabilities properties, fix up the session ID and value members.
if (valueDictionary.ContainsKey("sessionId"))
if (valueDictionary.TryGetValue("sessionId", out object? session))
{
response.SessionId = valueDictionary["sessionId"].ToString();
if (valueDictionary.TryGetValue("capabilities", out object capabilities))
sessionId = session.ToString();
if (valueDictionary.TryGetValue("capabilities", out object? capabilities))
{
response.Value = capabilities;
contents = capabilities;
}
else
{
response.Value = valueDictionary["value"];
contents = valueDictionary["value"];
}
}
}

return response;
return new Response(sessionId, contents, WebDriverResult.Success);
}

/// <summary>
/// Gets or sets the value from JSON.
/// </summary>
public object Value { get; set; }
public object? Value { get; set; }

/// <summary>
/// Gets or sets the session ID.
/// </summary>
public string SessionId { get; set; }
public string? SessionId { get; set; }

/// <summary>
/// Gets or sets the status value of the response.
Expand All @@ -142,26 +154,25 @@ public static Response FromJson(string value)
/// </summary>
/// <param name="value">The JSON string to deserialize into a <see cref="Response"/>.</param>
/// <returns>A <see cref="Response"/> object described by the JSON string.</returns>
/// <exception cref="ArgumentNullException">If <paramref name="value"/> is <see langword="null"/>.</exception>
/// <exception cref="JsonException">If <paramref name="value"/> is not a valid JSON object.</exception>
/// <exception cref="WebDriverException">If the JSON dictionary is not in the expected state, per spec.</exception>
public static Response FromErrorJson(string value)
{
var deserializedResponse = JsonSerializer.Deserialize<Dictionary<string, object>>(value, s_jsonSerializerOptions)
Dictionary<string, object?> deserializedResponse = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, s_jsonSerializerOptions)
?? throw new WebDriverException("JSON error response returned \"null\" value");

var response = new Response();

if (!deserializedResponse.TryGetValue("value", out var valueObject))
if (!deserializedResponse.TryGetValue("value", out object? valueObject))
{
throw new WebDriverException($"The 'value' property was not found in the response:{Environment.NewLine}{value}");
}

if (valueObject is not Dictionary<string, object> valueDictionary)
if (valueObject is not Dictionary<string, object?> valueDictionary)
{
throw new WebDriverException($"The 'value' property is not a dictionary of <string, object>{Environment.NewLine}{value}");
}

response.Value = valueDictionary;

if (!valueDictionary.TryGetValue("error", out var errorObject))
if (!valueDictionary.TryGetValue("error", out object? errorObject))
{
throw new WebDriverException($"The 'value > error' property was not found in the response:{Environment.NewLine}{value}");
}
Expand All @@ -171,11 +182,9 @@ public static Response FromErrorJson(string value)
throw new WebDriverException($"The 'value > error' property is not a string{Environment.NewLine}{value}");
}

response.Value = deserializedResponse["value"];

response.Status = WebDriverError.ResultFromError(errorString);
WebDriverResult status = WebDriverError.ResultFromError(errorString);

return response;
return new Response(sessionId: null, valueDictionary, status);
}

/// <summary>
Expand Down

0 comments on commit 1f1d6b9

Please sign in to comment.