[dotnet] [bidi] Migrate RemoteValue to separate types

[RenderMichael]

User description

Description

Migrate RemoteValue to separate types

Also adds the BigInt remote type

Motivation and Context

Contributes to #15407

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.
• All new and existing tests passed.

---

PR Type

Enhancement, Tests

---

Description

• Replaced nested DTO types with separate, standalone types.

• Updated serialization and deserialization logic for new types.

• Refactored tests to align with new type structure.

• Added support for BigInt type in remote values.

---

Changes walkthrough 📝

	Relevant files
Enhancement	6 files BiDiJsonSerializerContext.cs Updated JSON serialization for new RemoteValue types          +27/-26  LocateNodesResultConverter.cs Updated node deserialization to use new RemoteNodeValue    +1/-1      RemoteValueConverter.cs Refactored RemoteValue deserialization for new types          +27/-26  LocateNodesCommand.cs Updated LocateNodesResult to use new RemoteNodeValue          +5/-5      NodeProperties.cs Refactored NodeProperties to use new RemoteNodeValue          +2/-2      RemoteValue.cs Replaced nested RemoteValue types with standalone types    +207/-152
Tests	5 files LogTest.cs Updated tests to use RemoteStringValue                                      +1/-1      CallFunctionParameterTest.cs Refactored function call tests for new RemoteValue types  +18/-18  CallFunctionRemoteValueTest.cs Updated RemoteValue tests for new type structure                  +44/-44  EvaluateParametersTest.cs Refactored evaluation tests for new RemoteValue types        +7/-7      ScriptCommandsTest.cs Updated script command tests for new RemoteValue types      +1/-1

---

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:

🎫 Ticket compliance analysis ✅ 15407 - PR Code Verified Compliant requirements: Migrate from nested DTO types to separate standalone types Apply this pattern to all similar types Ensure types are discoverable Requires further human verification: Verify that the pattern like new Locator.Css("div") has been replaced with new CssLocator("div") for all relevant types (the PR only shows RemoteValue changes)

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Static Factory Methods The implementation adds static factory methods like RemoteValue.String() and RemoteValue.Number() which should be verified to ensure they correctly handle all edge cases and maintain backward compatibility. public static RemoteBigIntValue BigInt(BigInteger value) { return new RemoteBigIntValue(value.ToString()); } public static RemoteNumberValue Number(double value) { return new RemoteNumberValue(value); } public static RemoteBooleanValue Boolean(bool value) { return new RemoteBooleanValue(value); } public static RemoteValue String(string? value) { return value is null ? RemoteNullValue.Instance : new RemoteStringValue(value); } public static RemoteNullValue Null => RemoteNullValue.Instance; public static RemoteUndefinedValue Undefined => RemoteUndefinedValue.Instance; public static RemoteNumberValue NegativeInfinity => RemoteNumberValue.NegativeInfinity; public static RemoteNumberValue PositiveInfinity => RemoteNumberValue.PositiveInfinity; public static RemoteNumberValue NaN => RemoteNumberValue.NaN; public static RemoteNumberValue NegativeZero => RemoteNumberValue.NegativeZero; } String Handling The RemoteValueConverter has a change in string handling that needs verification. The method returns from the if block without setting a value, which could lead to unexpected behavior. if (jsonDocument.RootElement.ValueKind == JsonValueKind.String) { return RemoteValue.String(jsonDocument.RootElement.GetString()); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null and type checking The cast operation doesn't include null checking, which could cause a NullReferenceException if remoteValue is null or an InvalidCastException if it's not a RemoteNumberValue. Add proper null and type checking. dotnet/src/webdriver/BiDi/Modules/Script/RemoteValue.cs [58] -public static implicit operator double(RemoteValue remoteValue) => (double)((RemoteNumberValue)remoteValue).Value; +public static implicit operator double(RemoteValue remoteValue) => remoteValue is RemoteNumberValue numberValue ? numberValue.Value : throw new InvalidCastException($"Cannot convert {remoteValue?.GetType().Name ?? "null"} to double"); Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion significantly improves error handling by adding both null checking and proper type checking for the implicit operator. This prevents potential runtime exceptions and provides a more descriptive error message.	Medium
	✅ Handle potential null string Suggestion Impact: The commit addressed the null string issue, but used a different approach. Instead of using the null-coalescing operator, it used the null-forgiving operator (!) to indicate that the value won't be null or that nulls are handled by the constructor. It also changed the method call from RemoteValue.String() to directly instantiating a StringRemoteValue. code diff: - return RemoteValue.String(jsonDocument.RootElement.GetString()); + return new StringRemoteValue(jsonDocument.RootElement.GetString()!); The RemoteValue.String() method is called with a potentially null string value from GetString(), but the method doesn't appear to handle null values properly. This could lead to a NullReferenceException. dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/RemoteValueConverter.cs [36] -return RemoteValue.String(jsonDocument.RootElement.GetString()); +return RemoteValue.String(jsonDocument.RootElement.GetString() ?? string.Empty); [Suggestion has been applied] Suggestion importance[1-10]: 7 __ Why: The suggestion addresses a potential NullReferenceException by handling the case where GetString() returns null. This is a valid defensive programming practice that improves code robustness.	Medium
Learned best practice	Validate JSON property existence with TryGetProperty before accessing it to prevent exceptions when properties are missing The code directly accesses the "nodes" property without checking if it exists first. This could throw a JsonException if the property is missing. Use TryGetProperty to safely check for the property's existence before attempting to access it, and provide a meaningful error message if it's missing. dotnet/src/webdriver/BiDi/Communication/Json/Converters/Enumerable/LocateNodesResultConverter.cs [34-36] -var nodes = doc.RootElement.GetProperty("nodes").Deserialize<IReadOnlyList<RemoteNodeValue>>(options); +if (!doc.RootElement.TryGetProperty("nodes", out JsonElement nodesElement)) + throw new JsonException("Missing required 'nodes' property in LocateNodesResult"); +var nodes = nodesElement.Deserialize<IReadOnlyList<RemoteNodeValue>>(options); return new LocateNodesResult(nodes!); Apply this suggestion Suggestion importance[1-10]: 6	Low
Update

[RenderMichael]

PR is most easily reviewed with whitespace off: https://github.com/SeleniumHQ/selenium/pull/15426/files?w=1