Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Dec 1, 2025

User description

💥 What does this PR do?

Now STJ doesn't generate metadata for BiDi type.

💡 Additional Considerations

It still doesn't resolve AOT trimming, but good step forward.

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

PR Type

Enhancement


Description

  • Remove BiDi parameter from EventArgs constructors to reduce metadata generation

  • Update all BiDi event classes to avoid unnecessary type references

  • Refactor InterceptedRequest, InterceptedResponse, and InterceptedAuth to use internal constructors

  • Improve AOT compatibility by eliminating BiDi type from serialization metadata


Diagram Walkthrough

flowchart LR
  A["EventArgs Base Classes"] -->|Remove BiDi param| B["BrowsingContextEventArgs"]
  A -->|Remove BiDi param| C["LogEntry"]
  B -->|Remove BiDi param| D["BrowsingContext Events"]
  B -->|Remove BiDi param| E["Network Events"]
  C -->|Remove BiDi param| F["Log Events"]
  G["Intercepted Classes"] -->|Refactor to internal ctor| H["InterceptedRequest/Response/Auth"]
  H -->|Set BiDi property| I["Improved AOT Compatibility"]
Loading

File Walkthrough

Relevant files
Enhancement
19 files
EventArgs.cs
Remove BiDi parameter from base EventArgs classes               
+4/-4     
BrowsingContextInfo.cs
Remove BiDi parameter from BrowsingContextInfo record       
+2/-2     
DownloadEndEventArgs.cs
Remove BiDi parameter from download event records               
+6/-6     
DownloadWillBeginEventArgs.cs
Remove BiDi parameter from download begin event                   
+2/-2     
HistoryUpdatedEventArgs.cs
Remove BiDi parameter from history event                                 
+2/-2     
NavigationInfo.cs
Remove BiDi parameter from navigation event                           
+2/-2     
UserPromptClosedEventArgs.cs
Remove BiDi parameter from user prompt event                         
+2/-2     
UserPromptOpenedEventArgs.cs
Remove BiDi parameter from user prompt opened event           
+2/-2     
LogEntry.cs
Remove BiDi parameter from log entry records                         
+8/-8     
AuthRequiredEventArgs.cs
Remove BiDi parameter from auth required event                     
+2/-2     
BaseParametersEventArgs.cs
Remove BiDi parameter from base network event                       
+3/-3     
BeforeRequestSentEventArgs.cs
Remove BiDi parameter from request event                                 
+2/-2     
FetchErrorEventArgs.cs
Remove BiDi parameter from fetch error event                         
+2/-2     
NetworkModule.HighLevel.cs
Refactor intercepted classes to use internal constructors
+21/-6   
ResponseCompletedEventArgs.cs
Remove BiDi parameter from response completed event           
+2/-2     
ResponseStartedEventArgs.cs
Remove BiDi parameter from response started event               
+2/-2     
MessageEventArgs.cs
Remove BiDi parameter from message event                                 
+1/-1     
RealmDestroyedEventArgs.cs
Remove BiDi parameter from realm destroyed event                 
+1/-1     
RealmInfo.cs
Remove BiDi parameter from realm info records                       
+9/-9     

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Dec 1, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 1, 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
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Logging Context: The refactor changes constructors and logging-related event records but the diff does not
show additions that ensure audit logs include user ID, timestamp, action description, and
outcome for critical actions.

Referred Code
public abstract record EventArgs
{
    [JsonIgnore]
    public BiDi BiDi { get; internal set; }
}

public abstract record BrowsingContextEventArgs(BrowsingContext.BrowsingContext Context)
    : EventArgs;

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Error Handling: New internal constructors and async continuation methods rely on external calls without
visible null/edge-case validation or contextual error handling in the added code.

Referred Code
public sealed record InterceptedRequest : BeforeRequestSentEventArgs
{
    internal InterceptedRequest(BiDi biDi, BrowsingContext.BrowsingContext? context, bool isBlocked, BrowsingContext.Navigation? navigation, long redirectCount, RequestData request, DateTimeOffset timestamp, Initiator initiator, IReadOnlyList<Intercept>? intercepts)
        : base(context, isBlocked, navigation, redirectCount, request, timestamp, initiator, intercepts)
    {
        BiDi = biDi;
    }

    public Task ContinueAsync(ContinueRequestOptions? options = null)
    {
        return BiDi.Network.ContinueRequestAsync(Request.Request, options);
    }

    public Task FailAsync()
    {
        return BiDi.Network.FailRequestAsync(Request.Request);
    }

    public Task ProvideResponseAsync(ProvideResponseOptions? options = null)
    {
        return BiDi.Network.ProvideResponseAsync(Request.Request, options);


 ... (clipped 28 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Validation: The new constructors accept externally-influenced fields (e.g., URLs, request/response
data) without visible validation or sanitization within the added code.

Referred Code
using System;
using System.Collections.Generic;

namespace OpenQA.Selenium.BiDi.Network;

public abstract record BaseParametersEventArgs(BrowsingContext.BrowsingContext? Context, bool IsBlocked, BrowsingContext.Navigation? Navigation, long RedirectCount, RequestData Request, DateTimeOffset Timestamp, IReadOnlyList<Intercept>? Intercepts)
    : BrowsingContextEventArgs(Context);

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 1, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Make BiDi property nullable to prevent exceptions

In EventArgs, change the BiDi property to be nullable (BiDi?) to reflect that it
is not initialized in the constructor and prevent potential null reference
exceptions.

dotnet/src/webdriver/BiDi/EventArgs.cs [24-28]

 public abstract record EventArgs
 {
     [JsonIgnore]
-    public BiDi BiDi { get; internal set; }
+    public BiDi? BiDi { get; internal set; }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the BiDi property is uninitialized, which can lead to NullReferenceException, and proposes making it nullable to improve type safety.

Medium
Learned
best practice
Validate non-null dependency

Guard against a null biDi by validating in the constructor and throwing
ArgumentNullException. This prevents null dereferences when calling
BiDi.Network.*.

dotnet/src/webdriver/BiDi/Network/NetworkModule.HighLevel.cs [62-84]

-public sealed record InterceptedRequest : BeforeRequestSentArgs
+public sealed record InterceptedRequest : BeforeRequestSentEventArgs
 {
     internal InterceptedRequest(BiDi biDi, BrowsingContext.BrowsingContext? context, bool isBlocked, BrowsingContext.Navigation? navigation, long redirectCount, RequestData request, DateTimeOffset timestamp, Initiator initiator, IReadOnlyList<Intercept>? intercepts)
         : base(context, isBlocked, navigation, redirectCount, request, timestamp, initiator, intercepts)
     {
-        BiDi = biDi;
+        BiDi = biDi ?? throw new ArgumentNullException(nameof(biDi));
     }
 
     public Task ContinueAsync(ContinueRequestOptions? options = null)
     {
         return BiDi.Network.ContinueRequestAsync(Request.Request, options);
     }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Pattern 1: Ensure external resources/contexts are properly managed; initialize required dependencies defensively to avoid null usage.

Low
  • Update

@nvborisenko nvborisenko merged commit 54fc9b4 into SeleniumHQ:trunk Dec 1, 2025
10 checks passed
@nvborisenko nvborisenko deleted the bidi-avoid-in-args branch December 1, 2025 18:06
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