[dotnet][bidi] Use subscription id for events to unsubscribe

[nvborisenko]

User description

Motivation and Context

Finally subscription has id managed on remote-end (will be available in chrome/edge/ff v134). I means we don't need to manage a state on client side.

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

---

Description

• Introduced Subscription ID for managing event subscriptions.

• Updated Subscribe and Unsubscribe methods to use Subscription ID.

• Added new classes and converters for handling Subscription serialization.

• Enhanced unsubscribe functionality with attribute-based and ID-based commands.

---

Changes walkthrough 📝

Relevant files

Enhancement

Broker.cs Refactored subscription and unsubscription logic                  dotnet/src/webdriver/BiDi/Communication/Broker.cs Updated SubscribeAsync to return Subscription with ID. Modified UnsubscribeAsync to use Subscription ID. Adjusted logic for context-based and ID-based unsubscription. +24/-16  BiDiJsonSerializerContext.cs Added serialization for new subscription commands                dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs Added serialization support for SubscribeResult. Added new commands for attribute-based unsubscription. +3/-1      SubscriptionConverter.cs Added JSON converter for `Subscription`                                    dotnet/src/webdriver/BiDi/Communication/Json/Converters/SubscriptionConverter.cs Introduced SubscriptionConverter for JSON serialization. Handles conversion of Subscription objects to/from JSON. +41/-0    SessionModule.cs Enhanced session module with new subscription methods        dotnet/src/webdriver/BiDi/Modules/Session/SessionModule.cs Updated SubscribeAsync to return SubscribeResult. Added ID-based and attribute-based unsubscription methods. +13/-9    SubscribeCommand.cs Introduced `SubscribeResult` for subscription responses    dotnet/src/webdriver/BiDi/Modules/Session/SubscribeCommand.cs Added SubscribeResult to encapsulate Subscription ID. +2/-0      Subscription.cs Introduced `Subscription` class for event management          dotnet/src/webdriver/BiDi/Modules/Session/Subscription.cs Added Subscription class to encapsulate subscription ID. Provides a structured way to manage subscriptions. +32/-0    UnsubscribeCommand.cs Added new commands for enhanced unsubscription                      dotnet/src/webdriver/BiDi/Modules/Session/UnsubscribeCommand.cs Added UnsubscribeByIdCommand for ID-based unsubscription. Added UnsubscribeByAttributesCommand for attribute-based unsubscription. Deprecated context-based unsubscription in attributes. +22/-3    Subscription.cs Enhanced `Subscription` class with ID-based logic                dotnet/src/webdriver/BiDi/Subscription.cs Updated Subscription class to include Subscription ID. Adjusted UnsubscribeAsync to use the new ID-based logic. +4/-2

---

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 🔍

(Review updated until commit b67ebbc)

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Null Reference The UnsubscribeAsync method checks if subscription is null but doesn't handle the case where eventHandler.EventName could throw NullReferenceException when accessing _eventHandlers dictionary public async Task UnsubscribeAsync(Modules.Session.Subscription subscription, EventHandler eventHandler) { var eventHandlers = _eventHandlers[eventHandler.EventName]; eventHandlers.Remove(eventHandler); if (subscription is not null) { await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false); } else { if (eventHandler.Contexts is not null) { if (!eventHandlers.Any(h => eventHandler.Contexts.Equals(h.Contexts)) && !eventHandlers.Any(h => h.Contexts is null)) { await _bidi.SessionModule.UnsubscribeAsync([eventHandler.EventName], new() { Contexts = eventHandler.Contexts }).ConfigureAwait(false); } } else { if (!eventHandlers.Any(h => h.Contexts is not null) && !eventHandlers.Any(h => h.Contexts is null)) { await _bidi.SessionModule.UnsubscribeAsync([eventHandler.EventName]).ConfigureAwait(false); } } } } Deprecation Warning The Contexts parameter is marked as obsolete but still used in the code. Should consider removing or updating the implementation to match the deprecation notice [Obsolete("Contexts param is deprecated and will be removed in the future versions")] // https://w3c.github.io/webdriver-bidi/#type-session-UnsubscribeByAttributesRequest public IEnumerable<BrowsingContext.BrowsingContext>? Contexts { get; set; }

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to b67ebbc
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add parameter validation Add null check for subscription parameter in UnsubscribeAsync to prevent potential NullReferenceException. The current code assumes subscription is not null when accessing it. dotnet/src/webdriver/BiDi/Communication/Broker.cs [274-283] public async Task UnsubscribeAsync(Modules.Session.Subscription subscription, EventHandler eventHandler) { + ArgumentNullException.ThrowIfNull(eventHandler); var eventHandlers = _eventHandlers[eventHandler.EventName]; eventHandlers.Remove(eventHandler); if (subscription is not null) { await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false); } Apply this suggestion Suggestion importance[1-10]: 8 __ Why: Adding null parameter validation is crucial for preventing NullReferenceException and providing clear error messages. This is particularly important in a public API method.	Medium
	Add dictionary key validation Add dictionary key existence check before accessing _eventHandlers to prevent KeyNotFoundException if the event name doesn't exist in the dictionary. dotnet/src/webdriver/BiDi/Communication/Broker.cs [276] -var eventHandlers = _eventHandlers[eventHandler.EventName]; +if (!_eventHandlers.TryGetValue(eventHandler.EventName, out var eventHandlers)) +{ + throw new InvalidOperationException($"No handlers found for event {eventHandler.EventName}"); +} Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion prevents potential KeyNotFoundException by safely checking dictionary key existence, which is essential for robust error handling in event management.	Medium

---

Previous suggestions

✅ Suggestions up to commit 4ecab12

Category	Suggestion	Impact
Possible issue	✅ Add parameter validation Suggestion Impact: The commit implements null checking for the subscription parameter, though in a different way than suggested. Instead of throwing an exception, it handles the null case with specific logic. code diff: + if (subscription is not null) + { + await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false); + } + else + { + if (eventHandler.Contexts is not null) + { + if (!eventHandlers.Any(h => eventHandler.Contexts.Equals(h.Contexts)) && !eventHandlers.Any(h => h.Contexts is null)) + { + await _bidi.SessionModule.UnsubscribeAsync([eventHandler.EventName], new() { Contexts = eventHandler.Contexts }).ConfigureAwait(false); + } + } + else + { + if (!eventHandlers.Any(h => h.Contexts is not null) && !eventHandlers.Any(h => h.Contexts is null)) + { + await _bidi.SessionModule.UnsubscribeAsync([eventHandler.EventName]).ConfigureAwait(false); + } + } + } Add null checks for the subscription ID and event handler parameters to prevent potential NullReferenceException when unsubscribing. dotnet/src/webdriver/BiDi/Communication/Broker.cs [274-281] public async Task UnsubscribeAsync(Modules.Session.Subscription subscription, EventHandler eventHandler) { + ArgumentNullException.ThrowIfNull(subscription); + ArgumentNullException.ThrowIfNull(eventHandler); + var eventHandlers = _eventHandlers[eventHandler.EventName]; eventHandlers.Remove(eventHandler); await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false); } Suggestion importance[1-10]: 8 __ Why: Adding null checks for parameters is crucial for preventing runtime NullReferenceExceptions, especially in an async method that handles critical subscription management.	Medium
	Handle missing event handlers Add error handling for the case when event handlers collection is empty or event name doesn't exist in the dictionary to prevent potential KeyNotFoundException. dotnet/src/webdriver/BiDi/Communication/Broker.cs [274-281] public async Task UnsubscribeAsync(Modules.Session.Subscription subscription, EventHandler eventHandler) { - var eventHandlers = _eventHandlers[eventHandler.EventName]; + if (!_eventHandlers.TryGetValue(eventHandler.EventName, out var eventHandlers)) + { + return; + } eventHandlers.Remove(eventHandler); await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false); } Suggestion importance[1-10]: 7 __ Why: Using TryGetValue instead of direct dictionary access prevents KeyNotFoundException and gracefully handles the case when event handlers don't exist, improving the robustness of the unsubscribe operation.	Medium

[nvborisenko]

Now I should to recall everything in events mechanism to understand whether it can be even more simplified.

[RenderMichael]

Per slack channel:

As I understand FF Beta starts to implement it.

Can we add tests on FF? Can we even run this code yet?

[nvborisenko]

PR is draft, so just waiting when v134 is released.

[nvborisenko]

Negotiated, works with both v133 and v134.