Home Original page

[dotnet] [bidi] Remove IEnumerable of command results by nvborisenko · Pull Request #16219 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Ensure JSON deserialization works

You removed custom JSON converters for enumerable results and changed result
types to primary-constructor records with collection properties, but did not
show how System.Text.Json will now bind nested arrays (e.g., "realms",
"contexts", "cookies") into those properties. Verify the default serializer
configuration maps the response payloads to the new record shapes, or add
minimal converters/JsonPropertyName attributes to prevent runtime
deserialization failures across all affected commands.

Examples:

dotnet/src/webdriver/BiDi/BrowsingContext/GetTreeCommand.cs [50] 
public sealed record GetTreeResult(IReadOnlyList<BrowsingContextInfo> Contexts) : EmptyResult;
dotnet/src/webdriver/BiDi/Communication/Broker.cs [95-103] 
                new Json.Converters.Polymorphic.EvaluateResultConverter(),
                new Json.Converters.Polymorphic.RemoteValueConverter(),
                new Json.Converters.Polymorphic.RealmInfoConverter(),
                new Json.Converters.Polymorphic.LogEntryConverter(),
                //

                // Enumerable
                new Json.Converters.Enumerable.InputSourceActionsConverter(),
            }

Solution Walkthrough:

Before:

// In Broker.cs
new JsonSerializerOptions
{
    Converters = 
    {
        // ...
        new Json.Converters.Enumerable.GetTreeResultConverter(),
        // ...
    }
};

// In GetTreeCommand.cs
public sealed record GetTreeResult : EmptyResult, IReadOnlyList<BrowsingContextInfo>
{
    // ... manual implementation ...
}

After:

// In Broker.cs
new JsonSerializerOptions
{
    Converters = 
    {
        // GetTreeResultConverter is removed
    }
};

// In GetTreeCommand.cs, to be robust against serializer settings:
using System.Text.Json.Serialization;
public sealed record GetTreeResult(
    [property: JsonPropertyName("contexts")] IReadOnlyList<BrowsingContextInfo> Contexts
) : EmptyResult;
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical potential failure point in JSON deserialization after removing custom converters, which could break multiple BiDi commands at runtime if the implicit mapping fails.

High
Possible issue
Null-safe realms access

Guard against a null Realms collection to avoid a potential
NullReferenceException if the deserializer or backend omits the property.
Provide an empty list fallback to keep the public API stable.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextScriptModule.cs [44]

-return (await scriptModule.GetRealmsAsync(options).ConfigureAwait(false)).Realms;
+var result = await scriptModule.GetRealmsAsync(options).ConfigureAwait(false);
+return result.Realms ?? Array.Empty<RealmInfo>();
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that Realms could be null after deserialization and proposes a robust fix using ?? Array.Empty<RealmInfo>(), which is good practice for methods returning collections.

Low
General
Null-safe empty assertion

Accessing Cookies.Count directly can throw if Cookies is null after
deserialization changes. Adjust the assertion to handle null safely to prevent
test failures masking real issues.

dotnet/src/webdriver/BiDi/Storage/StorageTest.cs [144]

-Assert.That(cookiesResult.Cookies.Count, Is.EqualTo(0));
+Assert.That(cookiesResult.Cookies, Is.Empty);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out a potential null reference and suggests a more robust and idiomatic assertion Assert.That(cookiesResult.Cookies, Is.Empty), improving test code quality.

Low
  • Update