Skip to content

[dotnet] [bidi] Remove IEnumerable of command results #16219

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Aug 20, 2025

User description

This contributes to #16095

💥 What does this PR do?

Remove syntax sugar and be more straightforward. Simplifying things as much possible. Any other sugar can be treated as "alternative API" via extensions.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement


Description

  • Remove IEnumerable implementation from BiDi command results

  • Simplify result classes to use primary constructors

  • Delete custom JSON converters for enumerable results

  • Update test code to access properties directly


Diagram Walkthrough

flowchart LR
  A["Command Result Classes"] --> B["Remove IEnumerable Interface"]
  B --> C["Use Primary Constructors"]
  C --> D["Delete JSON Converters"]
  D --> E["Update Test Access Patterns"]
Loading

File Walkthrough

Relevant files
Enhancement
14 files
GetClientWindowsCommand.cs
Simplify GetClientWindowsResult to primary constructor     
+1/-20   
GetUserContextsCommand.cs
Simplify GetUserContextsResult to primary constructor       
+1/-18   
BrowsingContextScriptModule.cs
Update GetRealms call to access Realms property                   
+1/-1     
GetTreeCommand.cs
Simplify GetTreeResult to primary constructor                       
+1/-17   
LocateNodesCommand.cs
Simplify LocateNodesResult to primary constructor               
+1/-18   
Broker.cs
Remove enumerable JSON converter registrations                     
+0/-6     
GetClientWindowsResultConverter.cs
Delete GetClientWindowsResult JSON converter                         
+0/-43   
GetCookiesResultConverter.cs
Delete GetCookiesResult JSON converter                                     
+0/-45   
GetRealmsResultConverter.cs
Delete GetRealmsResult JSON converter                                       
+0/-43   
GetTreeResultConverter.cs
Delete GetTreeResult JSON converter                                           
+0/-43   
GetUserContextsResultConverter.cs
Delete GetUserContextsResult JSON converter                           
+0/-43   
LocateNodesResultConverter.cs
Delete LocateNodesResult JSON converter                                   
+0/-44   
GetRealmsCommand.cs
Simplify GetRealmsResult to primary constructor                   
+1/-18   
GetCookiesCommand.cs
Simplify GetCookiesResult to primary constructor                 
+1/-21   
Tests
8 files
BiDiFixture.cs
Update test to access Contexts property                                   
+1/-1     
BrowserTest.cs
Update tests to access result properties                                 
+12/-12 
BrowsingContextTest.cs
Update tests to access Contexts property                                 
+12/-12 
SetFilesTest.cs
Update test to access Nodes property                                         
+2/-2     
CallFunctionParameterTest.cs
Update test to access Realms property                                       
+5/-5     
EvaluateParametersTest.cs
Update test to access Realms property                                       
+5/-5     
ScriptCommandsTest.cs
Update tests to access Realms property                                     
+14/-14 
StorageTest.cs
Update tests to access Cookies property                                   
+24/-24 

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 20, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

16095 - Partially compliant

Compliant requirements:

  • Remove overreaching syntax sugar like IReadOnlyList on Result classes and associated JSON converters; provide optional extensions instead.
  • Use EmptyResult as return type instead of void for all methods.

Non-compliant requirements:

  • Move/introduce driver.AsBiDiAsync() on AnyDriver/IWebDriver so BiDi lifetime is owned by the driver.
  • Revisit InterceptRequestAsync-style helpers; prefer extension-based convenience APIs atop public methods.
  • Use normal types as event args (inheritance topic).

Requires further human verification:

  • Validate end-to-end that removal of IEnumerable interfaces does not break downstream consumers or docs.
  • Confirm public API surface aligns with intended BiDi stability plan across packages (Selenium.WebDriver -> Selenium.WebDriver.BiDi).
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

GetRealmsAsync now returns Realms property from result. Confirm that null handling and default options remain correct and that previous callers expecting IEnumerable still behave via updated tests.

public async Task<IReadOnlyList<RealmInfo>> GetRealmsAsync(GetRealmsOptions? options = null)
{
    options ??= new();

    options.Context = context;

    return (await scriptModule.GetRealmsAsync(options).ConfigureAwait(false)).Realms;
}
Serialization Change

Removal of enumerable JSON converters may impact deserialization of older payload shapes. Verify all affected commands map correctly to new record shapes and no converters are still needed.

        // https://github.com/dotnet/runtime/issues/72604
        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(),
    }
};

_jsonSerializerContext = new BiDiJsonSerializerContext(jsonSerializerOptions);
API Break

GetCookiesResult now exposes Cookies and PartitionKey properties only; indexer and Count removed. Ensure all public usages and samples are updated and that ordering guarantees (if any) are documented.

public sealed class GetCookiesOptions : CommandOptions
{
    public CookieFilter? Filter { get; set; }

    public PartitionDescriptor? Partition { get; set; }
}

public sealed record GetCookiesResult(IReadOnlyList<Network.Cookie> Cookies, PartitionKey PartitionKey) : EmptyResult;

public sealed record CookieFilter
{
    public string? Name { get; set; }

Copy link
Contributor

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
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants