Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 8, 2025

User description

Improves how we deserialize event json data, providing JsonTypeInfo directly instead of context.

💥 What does this PR do?

Improves how we deserialize event json data, providing JsonTypeInfo directly instead of context.

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Replace JsonSerializerContext with specific JsonTypeInfo for event deserialization

  • Update all BiDi module event subscription methods

  • Simplify Broker event type mapping implementation

  • Remove unused Type property from CommandInfo class


Diagram Walkthrough

flowchart LR
  A["JsonSerializerContext"] --> B["JsonTypeInfo"]
  C["Event Subscription"] --> D["Specific Type Info"]
  E["Broker Mapping"] --> F["Simplified Implementation"]
Loading

File Walkthrough

Relevant files
Enhancement
BrowsingContextModule.cs
Update event subscriptions with specific JsonTypeInfo       

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs

  • Replace JsonContext with specific JsonContext.EventType for all event
    subscriptions
  • Update 20 event subscription methods across navigation, download,
    context, and prompt events
+28/-28 
Broker.cs
Refactor Broker to use JsonTypeInfo directly                         

dotnet/src/webdriver/BiDi/Communication/Broker.cs

  • Change event type mapping from tuple to direct JsonTypeInfo storage
  • Update SubscribeAsync methods to accept JsonTypeInfo instead of
    JsonSerializerContext
  • Remove unused Type property from CommandInfo class
  • Simplify event deserialization logic
+6/-9     
LogModule.cs
Update log event subscriptions                                                     

dotnet/src/webdriver/BiDi/Log/LogModule.cs

  • Update log entry event subscriptions to use JsonContext.LogEntry
+2/-2     
NetworkModule.cs
Update network event subscriptions                                             

dotnet/src/webdriver/BiDi/Network/NetworkModule.cs

  • Update all network event subscriptions with specific JsonTypeInfo
  • Modify beforeRequestSent, responseStarted, responseCompleted,
    fetchError, and authRequired events
+10/-10 
ScriptModule.cs
Update script event subscriptions                                               

dotnet/src/webdriver/BiDi/Script/ScriptModule.cs

  • Update script event subscriptions to use specific JsonTypeInfo
  • Modify message, realmCreated, and realmDestroyed event handlers
+6/-6     

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

qodo-merge-pro bot commented Oct 8, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Enforce type safety in subscriptions

To enforce type safety, change the jsonTypeInfo parameter in the SubscribeAsync
methods from the non-generic JsonTypeInfo to the generic JsonTypeInfo. This
prevents potential runtime casting errors by ensuring the JSON type info matches
the event handler's argument type at compile time.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [159-215]

-    public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo jsonTypeInfo)
+    public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo)
         where TEventArgs : EventArgs
     {
         _eventTypesMap[eventName] = jsonTypeInfo;
 ...
     }
 
-    public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo jsonTypeInfo)
+    public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo)
         where TEventArgs : EventArgs
     {
         _eventTypesMap[eventName] = jsonTypeInfo;
 ...
     }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential type-safety issue in the SubscribeAsync method signatures. Using the generic JsonTypeInfo<TEventArgs> instead of the non-generic JsonTypeInfo enforces a compile-time check, preventing runtime errors from mismatched event handlers and deserialization types, thus making the API more robust.

Medium
Learned
best practice
Harden unsubscribe collection handling

Safely handle absent keys and empty handler lists to avoid KeyNotFound or race
conditions; use TryGetValue and clean up empty lists.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [217-224]

 public async Task UnsubscribeAsync(Session.Subscription subscription, EventHandler eventHandler)
 {
-    var eventHandlers = _eventHandlers[eventHandler.EventName];
+    if (eventHandler is null) throw new ArgumentNullException(nameof(eventHandler));
+    if (string.IsNullOrEmpty(eventHandler.EventName)) throw new ArgumentException("EventName is required", nameof(eventHandler));
 
-    eventHandlers.Remove(eventHandler);
+    if (_eventHandlers.TryGetValue(eventHandler.EventName, out var handlers) && handlers is not null)
+    {
+        handlers.Remove(eventHandler);
+        if (handlers.Count == 0)
+        {
+            _eventHandlers.TryRemove(eventHandler.EventName, out _);
+        }
+    }
 
-    await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false);
+    await _bidi.SessionModule.UnsubscribeAsync(new[] { subscription }).ConfigureAwait(false);
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Make concurrency and lifecycle code robust by using concurrent-safe operations and guarding collection access.

Low
Add input argument validation

Add argument validation to guard against null eventName, action/func, and
jsonTypeInfo to avoid runtime exceptions and inconsistent handler maps.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [159-186]

 public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo jsonTypeInfo)
     where TEventArgs : EventArgs
 {
+    if (string.IsNullOrWhiteSpace(eventName)) throw new ArgumentException("eventName is required", nameof(eventName));
+    if (action is null) throw new ArgumentNullException(nameof(action));
+    if (jsonTypeInfo is null) throw new ArgumentNullException(nameof(jsonTypeInfo));
+
     _eventTypesMap[eventName] = jsonTypeInfo;
 
-    var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []);
+    var handlers = _eventHandlers.GetOrAdd(eventName, _ => new List<EventHandler>());
 
     if (options is BrowsingContextsSubscriptionOptions browsingContextsOptions)
     {
-        var subscribeResult = await _bidi.SessionModule.SubscribeAsync([eventName], new() { Contexts = browsingContextsOptions.Contexts }).ConfigureAwait(false);
+        var subscribeResult = await _bidi.SessionModule.SubscribeAsync(new[] { eventName }, new() { Contexts = browsingContextsOptions.Contexts }).ConfigureAwait(false);
 
         ...
     }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Validate inputs and states early to prevent null dereferences and logic errors.

Low
  • More

@nvborisenko nvborisenko merged commit f243aee into SeleniumHQ:trunk Oct 8, 2025
13 checks passed
This was referenced Oct 19, 2025
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