Skip to content

[dotnet] [bidi] Support network collectors and get response body #16192

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

Draft
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Aug 16, 2025

User description

https://w3c.github.io/webdriver-bidi/#command-network-addDataCollector
https://w3c.github.io/webdriver-bidi/#command-network-getData
https://w3c.github.io/webdriver-bidi/#command-network-removeDataCollector

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Add BiDi network data collectors for response body capture

  • Implement AddDataCollectorAsync, RemoveDataCollectorAsync, and GetDataAsync methods

  • Add Collector class with automatic disposal support

  • Extend BytesValue with string conversion operators


Diagram Walkthrough

flowchart LR
  A["NetworkModule"] --> B["AddDataCollectorAsync"]
  B --> C["Collector"]
  C --> D["GetDataAsync"]
  D --> E["BytesValue"]
  C --> F["RemoveDataCollectorAsync"]
  G["CollectorConverter"] --> C
  H["JSON Serialization"] --> G
Loading

File Walkthrough

Relevant files
Configuration changes
2 files
Broker.cs
Add CollectorConverter to JSON serialization                         
+1/-0     
BiDiJsonSerializerContext.cs
Register data collector command types                                       
+5/-0     
Enhancement
7 files
CollectorConverter.cs
Create JSON converter for Collector objects                           
+47/-0   
AddDataCollectorCommand.cs
Implement add data collector command                                         
+49/-0   
BytesValue.cs
Add explicit string conversion operator                                   
+7/-0     
Collector.cs
Create Collector class with disposal                                         
+58/-0   
GetDataCommand.cs
Implement get data command                                                             
+36/-0   
NetworkModule.cs
Add data collector management methods                                       
+25/-0   
RemoveDataCollectorCommand.cs
Implement remove data collector command                                   
+29/-0   
Tests
1 files
NetworkTest.cs
Add tests for data collector functionality                             
+31/-0   

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

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Casting Behavior

The explicit cast to string decodes Base64 using UTF-8. Verify this is correct for all expected response bodies (binary data or non-UTF-8 encodings may throw or corrupt data). Consider exposing a byte[] accessor or encoding-parameterized decode to avoid misuse.

public static explicit operator string(BytesValue value) => value switch
{
    StringBytesValue stringBytesValue => stringBytesValue.Value,
    Base64BytesValue base64BytesValue => System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(base64BytesValue.Value)),
    _ => throw new InvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.")
};
Dispose Exceptions

DisposeAsync calls RemoveAsync without try/catch and without ConfigureAwait. If removal fails (e.g., session closed), disposal could throw during using/await using. Consider swallowing/logging expected disposal-time errors to keep disposal idempotent and resilient.

public async ValueTask DisposeAsync()
{
    await RemoveAsync();
}
Null Handling

Converter assumes JSON token is a non-null string and uses null-forgiving operator. Add validation to guard against null/invalid token types to avoid NullReferenceException at runtime.

public override Collector? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    var id = reader.GetString();

    return new Collector(_bidi, id!);
}

@nvborisenko nvborisenko marked this pull request as draft August 16, 2025 13:48
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate JSON token for id

Validate the JSON token type before reading to prevent invalid JSON from causing
unexpected behavior. Throw a JsonException when the token is not a string or
when the id is null/empty.

dotnet/src/webdriver/BiDi/Communication/Json/Converters/CollectorConverter.cs [36-41]

 public override Collector? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 {
+    if (reader.TokenType != JsonTokenType.String)
+    {
+        throw new JsonException("Expected string token for Collector id.");
+    }
+
     var id = reader.GetString();
+    if (string.IsNullOrEmpty(id))
+    {
+        throw new JsonException("Collector id cannot be null or empty.");
+    }
 
-    return new Collector(_bidi, id!);
+    return new Collector(_bidi, id);
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion significantly improves the robustness of the JSON converter by adding validation for the token type and for a null or empty id, preventing potential exceptions from malformed JSON.

Medium
Make dispose safe and idempotent

Dispose should be resilient and idempotent. Wrap the removal in a try/catch to
avoid throwing during dispose paths and guard against double-dispose to prevent
multiple removal attempts.

dotnet/src/webdriver/BiDi/Network/Collector.cs [42-45]

+private bool _disposed;
+
 public async ValueTask DisposeAsync()
 {
-    await RemoveAsync();
+    if (_disposed) return;
+    _disposed = true;
+    try
+    {
+        await RemoveAsync().ConfigureAwait(false);
+    }
+    catch
+    {
+        // Suppress exceptions on dispose
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly implements the standard disposable pattern, making DisposeAsync idempotent and preventing exceptions from being thrown, which is a crucial best practice for resource management.

Medium
General
Add null-safe explicit cast

Handle null inputs defensively to avoid a NullReferenceException during explicit
cast. Also use the Encoding property directly and consider malformed base64 data
to avoid hard crashes.

dotnet/src/webdriver/BiDi/Network/BytesValue.cs [33-38]

-public static explicit operator string(BytesValue value) => value switch
+public static explicit operator string?(BytesValue? value) => value switch
 {
+    null => null,
     StringBytesValue stringBytesValue => stringBytesValue.Value,
     Base64BytesValue base64BytesValue => System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(base64BytesValue.Value)),
     _ => throw new InvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.")
 };
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly improves the robustness of the explicit cast operator by handling null input, which aligns with standard C# casting behavior and prevents potential NullReferenceException.

Low
Learned
best practice
Validate constructor parameters

Validate constructor arguments to prevent invalid state and clearer errors.
Check for null bidi and null/empty id and throw ArgumentNullException or
ArgumentException.

dotnet/src/webdriver/BiDi/Network/Collector.cs [29-33]

 internal Collector(BiDi bidi, string id)
 {
-    _bidi = bidi;
-    Id = id;
+    _bidi = bidi ?? throw new ArgumentNullException(nameof(bidi));
+    Id = !string.IsNullOrEmpty(id) ? id : throw new ArgumentException("Collector id cannot be null or empty.", nameof(id));
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add null checks and validation for parameters before using them to prevent runtime errors.

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.

3 participants