Skip to content

[dotnet] [bidi] Remove obsolete unsubscribing by attributes #16205

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

[dotnet] [bidi] Remove obsolete unsubscribing by attributes #16205

wants to merge 2 commits into from
+1 −44

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Aug 18, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Other

Description

  • Remove obsolete unsubscribe by attributes functionality

  • Simplify broker unsubscribe logic to use subscription IDs only

  • Clean up unused JSON serialization attributes

  • Remove deprecated context parameter handling

Diagram Walkthrough

flowchart LR
  A["Broker.UnsubscribeAsync"] --> B["Remove complex attribute logic"]
  B --> C["Use subscription ID only"]
  D["SessionModule"] --> E["Remove UnsubscribeByAttributes method"]
  F["UnsubscribeCommand"] --> G["Remove attributes command class"]
  H["JsonSerializerContext"] --> I["Remove serialization attribute"]
Loading

File Walkthrough
Relevant files
Cleanup
Broker.cs
Simplify unsubscribe logic to subscription ID only             

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

  • Remove complex conditional logic for unsubscribing by attributes
  • Simplify to use subscription ID only approach
  • Remove context-based unsubscribe handling
 +1/-21   
BiDiJsonSerializerContext.cs
Remove obsolete JSON serialization attribute                         

dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs

  • Remove JsonSerializable attribute for UnsubscribeByAttributesCommand
 +0/-1     
SessionModule.cs
Remove unsubscribe by attributes method                                   

dotnet/src/webdriver/BiDi/Session/SessionModule.cs

  • Remove UnsubscribeAsync method that accepts event names
  • Remove support for unsubscribing by attributes
 +0/-7     
UnsubscribeCommand.cs
Remove attributes-based unsubscribe command classes           

dotnet/src/webdriver/BiDi/Session/UnsubscribeCommand.cs

  • Remove UnsubscribeByAttributesCommand class
  • Remove UnsubscribeByAttributesParameters record
  • Remove deprecated contexts parameter handling
 +0/-10   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 18, 2025
@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1234 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Investigate regression for JavaScript-in-href click behavior.
  • Provide a fix ensuring click() triggers JavaScript in href.
  • Validate behavior in Firefox context.

Requires further human verification:

  • None

5678 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Diagnose and resolve ConnectFailure on multiple ChromeDriver instances.
  • Provide preventive code/config changes.

Requires further human verification:

  • None
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Null Handling Regression

Previously, unsubscribe skipped BiDi call when subscription was null; now it unconditionally calls UnsubscribeAsync with the possibly null subscription. Confirm that subscription is guaranteed non-null here or add guard to avoid sending invalid payloads.

 
public async Task UnsubscribeAsync(Session.Subscription subscription, EventHandler eventHandler)
{
    var eventHandlers = _eventHandlers[eventHandler.EventName];

    eventHandlers.Remove(eventHandler);

    await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false);
}
Dead Option Type

UnsubscribeByAttributesOptions remains but there is no command or API using it after removing attributes-based unsubscribe. Consider removing this options class to avoid confusion and unused public API surface.

 
public sealed class UnsubscribeByIdOptions : CommandOptions;

public sealed class UnsubscribeByAttributesOptions : CommandOptions
{
    public IEnumerable<BrowsingContext.BrowsingContext>? Contexts { get; set; }
}
Serializer Coverage

After removing the JsonSerializable entry for UnsubscribeByAttributesCommand, ensure no runtime serialization is expected for attributes-based options or events. Validate source generator output and trim warnings.

 
[JsonSerializable(typeof(Session.StatusCommand))]
[JsonSerializable(typeof(Session.StatusResult))]
[JsonSerializable(typeof(Session.NewCommand))]
[JsonSerializable(typeof(Session.NewResult))]
[JsonSerializable(typeof(Session.EndCommand))]
[JsonSerializable(typeof(Session.SubscribeCommand))]
[JsonSerializable(typeof(Session.SubscribeResult))]
[JsonSerializable(typeof(Session.UnsubscribeByIdCommand))]

[JsonSerializable(typeof(Browser.CloseCommand), TypeInfoPropertyName = "Browser_CloseCommand")]
[JsonSerializable(typeof(Browser.CreateUserContextCommand))]
[JsonSerializable(typeof(Browser.GetUserContextsCommand))]

@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 18, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against null subscription

Add a null check for subscription before calling unsubscribe to avoid a
potential ArgumentNullException or protocol error when it is null. If null,
return early without invoking the command.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [278]

+if (subscription is null)
+{
+    return;
+}
 await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The PR removes a complex conditional block that previously handled null subscription values, but the new code does not account for this case, which could lead to a runtime error.

Medium
General
✅ Mark removed API as obsolete error
Suggestion Impact:The commit removed UnsubscribeByAttributesOptions from the public API, which aligns with the suggestion's intent to prevent its usage.

code diff:

-public sealed class UnsubscribeByAttributesOptions : CommandOptions
-{
-    public IEnumerable<BrowsingContext.BrowsingContext>? Contexts { get; set; }
-}

Remove UnsubscribeByAttributesOptions since attribute-based unsubscribe is no
longer supported, preventing accidental usage and API drift. If kept for binary
compatibility, mark it [Obsolete(true)] to produce compile-time errors.

dotnet/src/webdriver/BiDi/Session/UnsubscribeCommand.cs [32-35]

+[Obsolete("Unsubscribe by attributes is no longer supported. Use unsubscribe by subscription id instead.", true)]
 public sealed class UnsubscribeByAttributesOptions : CommandOptions
 {
     public IEnumerable<BrowsingContext.BrowsingContext>? Contexts { get; set; }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The PR removes the functionality associated with UnsubscribeByAttributesOptions but leaves the class in the public API, so marking it as obsolete improves API consistency and prevents misuse.

Medium
  • Update

@nvborisenko
Copy link
Member Author

Try to recall where in code base it was used, subscription without id.

@nvborisenko nvborisenko changed the title [dotnet] [biti] Remove obsolete unsubscribing by attributes [dotnet] [bidi] Remove obsolete unsubscribing by attributes Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C-dotnet .NET Bindings Review effort 2/5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nvborisenko @selenium-ci