[dotnet] Parse response before deserialization

[RenderMichael]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

This will parse the WebDriver response into its constituent parts before deserializing into an object

Motivation and Context

This simplifies the handling, and prepares us to potentially expose the response as a JsonNode/JsonElement in the future

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• [x ] All new and existing tests passed.

---

PR Type

Enhancement

---

Description

• Refactored Response class to use JsonNode for parsing.

• Simplified JSON deserialization logic for better maintainability.

• Improved error handling for JSON parsing in FromJson and FromErrorJson.

• Updated ResponseJsonSerializerContext to include new serialization options.

---

Changes walkthrough 📝

	Relevant files
Enhancement	Response.cs Refactor JSON parsing and deserialization in Response class dotnet/src/webdriver/Response.cs Replaced Dictionary with JsonNode for JSON parsing. Updated FromJson and FromErrorJson methods for better error handling. Added JsonSourceGenerationOptions to ResponseJsonSerializerContext. Simplified deserialization logic with JsonSerializer.Deserialize. +26/-28

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling in FromJson() could be improved. If parsing fails or contents is null after the capabilities check, no explicit error is thrown, which could lead to unclear errors downstream. JsonObject rawResponse = JsonNode.Parse(value) as JsonObject ?? throw new WebDriverException($"JSON success response did not return a dictionary{Environment.NewLine}{value}"); JsonNode? contents; string? sessionId = null; if (rawResponse.TryGetPropertyValue("sessionId", out JsonNode? s) && s is not null) { sessionId = s.ToString(); } if (rawResponse.TryGetPropertyValue("value", out JsonNode? valueObj)) { contents = valueObj; } 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.TryGetPropertyValue("capabilities", out JsonNode? capabilities)) { contents = capabilities; } else { contents = rawResponse; } } Type Safety The deserialization of contents into contentsDictionary does not have explicit type checking or error handling, which could cause runtime errors if the JSON structure is unexpected. var contentsDictionary = JsonSerializer.Deserialize(contents, ResponseJsonSerializerContext.Default.Object);

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for safety Add null check for valueNode before attempting to use it as JsonObject to prevent potential NullReferenceException. dotnet/src/webdriver/Response.cs [188-191] -if (valueNode is not JsonObject valueObject) +if (valueNode is null || valueNode is not JsonObject valueObject) { - throw new WebDriverException($"The 'value' property is not a dictionary{Environment.NewLine}{value}"); + throw new WebDriverException($"The 'value' property is null or not a dictionary{Environment.NewLine}{value}"); } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion adds an important null check that could prevent a potential NullReferenceException in production. This is a valid defensive programming practice for improving error handling.	Medium
	Validate contents before deserialization Add null check for contents before attempting deserialization to prevent potential NullReferenceException. dotnet/src/webdriver/Response.cs [132] +if (contents is null) +{ + throw new WebDriverException("Response contents cannot be null"); +} var contentsDictionary = JsonSerializer.Deserialize(contents, ResponseJsonSerializerContext.Default.Object); Apply this suggestion Suggestion importance[1-10]: 7 __ Why: Adding a null check before deserialization is a good defensive programming practice that would prevent NullReferenceException and provide a more descriptive error message.	Medium
Learned best practice	Add null validation check at the start of methods that accept nullable parameters to prevent NullReferenceException The FromJson and FromErrorJson methods accept a nullable string parameter but don't validate it for null at the start. Add ArgumentNullException.ThrowIfNull() check to prevent potential NullReferenceException when parsing the JSON. dotnet/src/webdriver/Response.cs [77-80] public static Response FromJson(string value) { + ArgumentNullException.ThrowIfNull(value, nameof(value)); JsonObject rawResponse = JsonNode.Parse(value) as JsonObject ?? throw new WebDriverException($"JSON success response did not return a dictionary{Environment.NewLine}{value}"); Apply this suggestion	Low

[RenderMichael]

@nvborisenko Now that Response is immutable, this brings us one step forward.

[nvborisenko]

Is it binary breaking change?

[RenderMichael]

No breaks in this PR, this is all internal changes. We still return a Value deserializes to .NET types, like before.

[nvborisenko]

Thanks!