Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 11, 2025

User description

💥 What does this PR do?

Simplifies DTO definitions. JsonInclude attribute leads to messing.

🔧 Implementation Notes

I am applied it everywhere is possible.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Other


Description

  • Remove JsonInclude attributes from BiDi DTO classes

  • Convert properties to constructor parameters in records

  • Simplify serialization by using primary constructors

  • Update network event handlers with new signatures


Diagram Walkthrough

flowchart LR
  A["JsonInclude Properties"] --> B["Primary Constructor Parameters"]
  B --> C["Simplified DTOs"]
  C --> D["Updated Event Handlers"]
Loading

File Walkthrough

Relevant files
Enhancement
14 files
BrowsingContextInfo.cs
Convert Parent property to constructor parameter                 
+2/-7     
BrowsingContextNetworkModule.cs
Add Intercepts parameter to event handlers                             
+3/-3     
UserPromptClosedEventArgs.cs
Convert UserText property to constructor parameter             
+2/-8     
UserPromptOpenedEventArgs.cs
Convert DefaultValue property to constructor parameter     
+2/-6     
AuthRequiredEventArgs.cs
Add Intercepts parameter to constructor                                   
+3/-2     
BaseParametersEventArgs.cs
Convert Intercepts property to constructor parameter         
+2/-8     
BeforeRequestSentEventArgs.cs
Add Intercepts parameter to constructor                                   
+3/-2     
FetchErrorEventArgs.cs
Add Intercepts parameter to constructor                                   
+3/-2     
NetworkModule.HighLevel.cs
Update intercepted event constructors with Intercepts       
+10/-9   
ResponseCompletedEventArgs.cs
Add Intercepts parameter to constructor                                   
+4/-2     
ResponseData.cs
Convert AuthChallenges property to constructor parameter 
+2/-6     
ResponseStartedEventArgs.cs
Add Intercepts parameter to constructor                                   
+4/-2     
NodeProperties.cs
Convert all properties to constructor parameters                 
+1/-24   
RemoteValue.cs
Convert SharedId and Value to constructor parameters         
+1/-7     

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

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Potential null handling

Description: The new primary-constructor record NodeRemoteValue(string? SharedId, NodeProperties?
Value) makes SharedId and Value optional, which can lead to null dereference or logic
errors if downstream code assumes these were always populated by deserialization when
previously guarded by JsonInclude.
RemoteValue.cs [255-260]

Referred Code
public sealed record NodeRemoteValue(string? SharedId, NodeProperties? Value) : RemoteValue, ISharedReference
{
    public Handle? Handle { get; set; }

    public InternalId? InternalId { get; set; }
}
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

@nvborisenko nvborisenko changed the title [dotnet] [biid] Avoid using JsonInclude attribute to include optional property for DTO [dotnet] [bidi] Avoid using JsonInclude attribute to include optional property for DTO Oct 11, 2025
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Make record properties immutable

Move the mutable Handle and InternalId properties of the NodeRemoteValue record
to its primary constructor to ensure the object is immutable.

dotnet/src/webdriver/BiDi/Script/RemoteValue.cs [255-260]

-public sealed record NodeRemoteValue(string? SharedId, NodeProperties? Value) : RemoteValue, ISharedReference
-{
-    public Handle? Handle { get; set; }
+public sealed record NodeRemoteValue(string? SharedId, NodeProperties? Value, Handle? Handle, InternalId? InternalId) : RemoteValue, ISharedReference;
 
-    public InternalId? InternalId { get; set; }
-}
-
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that Handle and InternalId are mutable properties and proposes moving them to the primary constructor, which improves immutability and consistency with the record's design.

Low
  • More

@nvborisenko nvborisenko merged commit 0c96b6f into SeleniumHQ:trunk Oct 18, 2025
13 checks passed
@nvborisenko nvborisenko deleted the bidi-ignore-json-for-read branch October 18, 2025 18:27
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