Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Dec 11, 2025

User description

🔗 Related Issues

Fixes #15621

💥 What does this PR do?

Adds missing type.

🔧 Implementation Notes

Moving polymorphic serialization discriminator for LocalValue abstract type into derived types, which is explicitly null for RemoteReference types.

🔄 Types of changes

  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Bug fix, Enhancement


Description

  • Removed polymorphic serialization from LocalValue abstract type

  • Added RemoteReferenceLocalValue abstract type with implementations

  • Added explicit Type properties to all LocalValue derived classes

  • Refactored serialization to use individual type discriminators


Diagram Walkthrough

flowchart LR
  A["LocalValue<br/>abstract record"] --> B["RemoteReferenceLocalValue<br/>abstract record"]
  A --> C["PrimitiveProtocolLocalValue<br/>abstract record"]
  A --> D["ChannelLocalValue<br/>sealed record"]
  A --> E["ArrayLocalValue<br/>sealed record"]
  B --> F["SharedReferenceLocalValue<br/>sealed record"]
  B --> G["RemoteObjectReferenceLocalValue<br/>sealed record"]
  C --> H["NumberLocalValue<br/>with Type property"]
  C --> I["StringLocalValue<br/>with Type property"]
  J["Removed<br/>JsonPolymorphic"] -.-> K["Added<br/>Type properties"]
Loading

File Walkthrough

Relevant files
Enhancement
LocalValue.cs
Refactor LocalValue serialization and add RemoteReference types

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

  • Removed JsonPolymorphic attribute and all JsonDerivedType declarations
    from LocalValue base class
  • Added new RemoteReferenceLocalValue abstract record with two sealed
    implementations: SharedReferenceLocalValue and
    RemoteObjectReferenceLocalValue
  • Added explicit Type property with [JsonInclude] attribute to all 16
    derived LocalValue classes for individual type discrimination
  • Reorganized class hierarchy to include new RemoteReferenceLocalValue
    branch alongside existing PrimitiveProtocolLocalValue and other types
+72/-28 

@nvborisenko nvborisenko changed the title [dotnet] [bidi] Added missing Script.RemoteReference LocaclValue type [dotnet] [bidi] Added missing Script.RemoteReference LocalValue type Dec 11, 2025
@qodo-code-review qodo-code-review bot changed the title [dotnet] [bidi] Added missing Script.RemoteReference LocalValue type [dotnet] [bidi] Added missing Script.RemoteReference LocaclValue type Dec 11, 2025
@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Dec 11, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 11, 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
🟢
🎫 #15621
🟢 Add missing script.RemoteReference LocalValue type to BiDi .NET implementation as per the
WebDriver BiDi spec.
Ensure LocalValue hierarchy supports remote references (e.g., shared and remote object
references).
Adjust serialization to correctly discriminate LocalValue types without relying on
previous polymorphic attributes.
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:
No auditing scope: The changes add/modify type models and serialization details without introducing or
touching any critical actions or logging, so there is no evidence of missing required
audit trails in this diff but also nothing to assess audit coverage.

Referred Code
using OpenQA.Selenium.BiDi.Json.Converters;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Text.Json.Serialization;
using System.Text.RegularExpressions;

namespace OpenQA.Selenium.BiDi.Script;

public abstract record LocalValue
{
    public static implicit operator LocalValue(bool? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(int? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(double? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(string? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(DateTimeOffset? value) { return ConvertFrom(value); }

    // TODO: Extend converting from types
    public static LocalValue ConvertFrom(object? value)


 ... (clipped 322 lines)

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:
No error paths: The diff introduces/updates data records and type discriminators without adding operations
that can fail, so there are no new failure points to assess; robustness appears unchanged
but cannot be fully verified from this snippet.

Referred Code
            values.Add([property.Name, ConvertFrom(propertyValue)]);
        }

        return new ObjectLocalValue(values);
    }
}

public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference;

public sealed record SharedReferenceLocalValue(string SharedId) : RemoteReferenceLocalValue, ISharedReference
{
    public Handle? Handle { get; set; }
}

public sealed record RemoteObjectReferenceLocalValue(Handle Handle) : RemoteReferenceLocalValue, IRemoteObjectReference
{
    public string? SharedId { get; set; }
}

public abstract record PrimitiveProtocolLocalValue : LocalValue;


 ... (clipped 80 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:
Serialization surface: The added explicit type discriminators and new records change the serialization surface
but the diff does not show external input validation or security-sensitive handling, so
security impact cannot be fully assessed from this isolated model code.

Referred Code
public abstract record PrimitiveProtocolLocalValue : LocalValue;

public sealed record NumberLocalValue([property: JsonConverter(typeof(SpecialNumberConverter))] double Value) : PrimitiveProtocolLocalValue
{
    [JsonInclude]
    internal string Type { get; } = "number";

    public static explicit operator NumberLocalValue(double n) => new NumberLocalValue(n);
}

public sealed record StringLocalValue(string Value) : PrimitiveProtocolLocalValue
{
    [JsonInclude]
    internal string Type { get; } = "string";
}

public sealed record NullLocalValue : PrimitiveProtocolLocalValue
{
    [JsonInclude]
    internal string Type { get; } = "null";
}


 ... (clipped 60 lines)

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 11, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add missing JSON type property
Suggestion Impact:The commit added an abstract Type property to LocalValue with [JsonInclude], and implemented it in all subclasses. For RemoteReferenceLocalValue’s derived classes, Type is now overridden (though set to null! here), addressing the missing Type property for remote references and aligning with the serialization pattern.

code diff:

+    [JsonInclude]
+    internal abstract string Type { get; }
 }
 
 public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference;
@@ -272,91 +291,82 @@
 public sealed record SharedReferenceLocalValue(string SharedId) : RemoteReferenceLocalValue, ISharedReference
 {
     public Handle? Handle { get; set; }
+
+    internal override string Type { get; } = null!;
 }
 
 public sealed record RemoteObjectReferenceLocalValue(Handle Handle) : RemoteReferenceLocalValue, IRemoteObjectReference
 {
     public string? SharedId { get; set; }
+
+    internal override string Type { get; } = null!;
 }

Add the missing Type property to the RemoteReferenceLocalValue abstract record
to ensure correct JSON serialization for its derived classes.

dotnet/src/webdriver/BiDi/Script/LocalValue.cs [270-280]

-public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference;
+public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference
+{
+    [JsonInclude]
+    internal string Type { get; } = "remote";
+}
 
 public sealed record SharedReferenceLocalValue(string SharedId) : RemoteReferenceLocalValue, ISharedReference
 {
     public Handle? Handle { get; set; }
 }
 
 public sealed record RemoteObjectReferenceLocalValue(Handle Handle) : RemoteReferenceLocalValue, IRemoteObjectReference
 {
     public string? SharedId { get; set; }
 }

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that the new RemoteReferenceLocalValue and its derived classes are missing the Type property, which is essential for correct JSON serialization and is inconsistent with the pattern applied to all other types in this PR.

High
High-level
Re-evaluate the serialization strategy change

Reconsider replacing the standard JsonPolymorphic attribute with manual Type
properties in each subclass. This change increases complexity and likely
requires a custom deserializer, so reverting to the built-in polymorphism
support is recommended unless there's a strong reason against it.

Examples:

dotnet/src/webdriver/BiDi/Script/LocalValue.cs [31-45]
public abstract record LocalValue
{
    public static implicit operator LocalValue(bool? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(int? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(double? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(string? value) { return ConvertFrom(value); }
    public static implicit operator LocalValue(DateTimeOffset? value) { return ConvertFrom(value); }

    // TODO: Extend converting from types
    public static LocalValue ConvertFrom(object? value)

 ... (clipped 5 lines)

Solution Walkthrough:

Before:

// No JsonPolymorphic attribute
public abstract record LocalValue
{
    // ...
}

public sealed record NumberLocalValue(...) : PrimitiveProtocolLocalValue
{
    [JsonInclude]
    internal string Type { get; } = "number";
}

public sealed record StringLocalValue(...) : PrimitiveProtocolLocalValue
{
    [JsonInclude]
    internal string Type { get; } = "string";
}
// ... and so on for all derived types

After:

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(NumberLocalValue), "number")]
[JsonDerivedType(typeof(StringLocalValue), "string")]
// ... other existing types
[JsonDerivedType(typeof(SharedReferenceLocalValue), "sharedId")] // Example type name
[JsonDerivedType(typeof(RemoteObjectReferenceLocalValue), "handle")] // Example type name
public abstract record LocalValue
{
    // ...
}

public sealed record NumberLocalValue(...) : PrimitiveProtocolLocalValue;
public sealed record StringLocalValue(...) : PrimitiveProtocolLocalValue;
// ... derived types do not need an explicit 'Type' property
Suggestion importance[1-10]: 8

__

Why: This is a significant architectural suggestion that correctly questions the removal of the standard JsonPolymorphic attribute in favor of a more complex and manual approach, which increases maintenance overhead.

Medium
General
Use init-only properties for consistency

Change the mutable properties Handle and SharedId in SharedReferenceLocalValue
and RemoteObjectReferenceLocalValue to be init-only for consistency with record
type immutability.

dotnet/src/webdriver/BiDi/Script/LocalValue.cs [272-280]

 public sealed record SharedReferenceLocalValue(string SharedId) : RemoteReferenceLocalValue, ISharedReference
 {
-    public Handle? Handle { get; set; }
+    public Handle? Handle { get; init; }
 }
 
 public sealed record RemoteObjectReferenceLocalValue(Handle Handle) : RemoteReferenceLocalValue, IRemoteObjectReference
 {
-    public string? SharedId { get; set; }
+    public string? SharedId { get; init; }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out an inconsistency in property mutability and proposes using init setters to align with the immutable nature of record types, which improves code quality and maintainability.

Low
Learned
best practice
Centralize type discriminator logic
Suggestion Impact:The commit moved the Type property to an abstract member on the base LocalValue and overrode it in each derived record, eliminating repeated [JsonInclude] properties per record. While it did not introduce the exact TypedLocalValue/TypedPrimitiveLocalValue helper types suggested, it centralized the discriminator logic as proposed.

code diff:

@@ -265,6 +281,9 @@
 
         return new ObjectLocalValue(values);
     }
+
+    [JsonInclude]
+    internal abstract string Type { get; }
 }
 
 public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference;
@@ -272,91 +291,82 @@
 public sealed record SharedReferenceLocalValue(string SharedId) : RemoteReferenceLocalValue, ISharedReference
 {
     public Handle? Handle { get; set; }
+
+    internal override string Type { get; } = null!;
 }
 
 public sealed record RemoteObjectReferenceLocalValue(Handle Handle) : RemoteReferenceLocalValue, IRemoteObjectReference
 {
     public string? SharedId { get; set; }
+
+    internal override string Type { get; } = null!;
 }
 
 public abstract record PrimitiveProtocolLocalValue : LocalValue;
 
 public sealed record NumberLocalValue([property: JsonConverter(typeof(SpecialNumberConverter))] double Value) : PrimitiveProtocolLocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "number";
+    internal override string Type { get; } = "number";
 
     public static explicit operator NumberLocalValue(double n) => new NumberLocalValue(n);
 }
 
 public sealed record StringLocalValue(string Value) : PrimitiveProtocolLocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "string";
+    internal override string Type { get; } = "string";
 }
 
 public sealed record NullLocalValue : PrimitiveProtocolLocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "null";
+    internal override string Type { get; } = "null";
 }
 
 public sealed record UndefinedLocalValue : PrimitiveProtocolLocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "undefined";
+    internal override string Type { get; } = "undefined";
 }
 
 public sealed record BooleanLocalValue(bool Value) : PrimitiveProtocolLocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "boolean";
+    internal override string Type { get; } = "boolean";
 }
 
 public sealed record BigIntLocalValue(string Value) : PrimitiveProtocolLocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "bigint";
+    internal override string Type { get; } = "bigint";
 }
 
 public sealed record ChannelLocalValue(ChannelProperties Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "channel";
+    internal override string Type { get; } = "channel";
 }
 
 public sealed record ArrayLocalValue(IEnumerable<LocalValue> Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "array";
+    internal override string Type { get; } = "array";
 }
 
 public sealed record DateLocalValue(string Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "date";
+    internal override string Type { get; } = "date";
 }
 
 public sealed record MapLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "map";
+    internal override string Type { get; } = "map";
 }
 
 public sealed record ObjectLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "object";
+    internal override string Type { get; } = "object";
 }
 
 public sealed record RegExpLocalValue(RegExpValue Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "regexp";
+    internal override string Type { get; } = "regexp";
 }
 
 public sealed record SetLocalValue(IEnumerable<LocalValue> Value) : LocalValue
 {
-    [JsonInclude]
-    internal string Type { get; } = "set";
-}
+    internal override string Type { get; } = "set";
+}

Avoid repeating the discriminator property across all records; centralize it in
a shared base/helper to prevent drift and reduce boilerplate.

dotnet/src/webdriver/BiDi/Script/LocalValue.cs [284-362]

-public sealed record NumberLocalValue([property: JsonConverter(typeof(SpecialNumberConverter))] double Value) : PrimitiveProtocolLocalValue
+public abstract record TypedLocalValue(string Discriminator) : LocalValue
 {
     [JsonInclude]
-    internal string Type { get; } = "number";
+    internal string Type { get; } = Discriminator;
+}
 
+public abstract record TypedPrimitiveLocalValue(string Discriminator) : PrimitiveProtocolLocalValue
+{
+    [JsonInclude]
+    internal string Type { get; } = Discriminator;
+}
+
+public sealed record NumberLocalValue([property: JsonConverter(typeof(SpecialNumberConverter))] double Value)
+    : TypedPrimitiveLocalValue("number")
+{
     public static explicit operator NumberLocalValue(double n) => new NumberLocalValue(n);
 }
 
-public sealed record StringLocalValue(string Value) : PrimitiveProtocolLocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "string";
-}
+public sealed record StringLocalValue(string Value) : TypedPrimitiveLocalValue("string");
+public sealed record NullLocalValue() : TypedPrimitiveLocalValue("null");
+public sealed record UndefinedLocalValue() : TypedPrimitiveLocalValue("undefined");
+public sealed record BooleanLocalValue(bool Value) : TypedPrimitiveLocalValue("boolean");
+public sealed record BigIntLocalValue(string Value) : TypedPrimitiveLocalValue("bigint");
 
-public sealed record NullLocalValue : PrimitiveProtocolLocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "null";
-}
+public sealed record ChannelLocalValue(ChannelProperties Value) : TypedLocalValue("channel");
+public sealed record ArrayLocalValue(IEnumerable<LocalValue> Value) : TypedLocalValue("array");
+public sealed record DateLocalValue(string Value) : TypedLocalValue("date");
+public sealed record MapLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : TypedLocalValue("map");
+public sealed record ObjectLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : TypedLocalValue("object");
+public sealed record RegExpLocalValue(RegExpValue Value) : TypedLocalValue("regexp");
+public sealed record SetLocalValue(IEnumerable<LocalValue> Value) : TypedLocalValue("set");
 
-public sealed record UndefinedLocalValue : PrimitiveProtocolLocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "undefined";
-}
-
-public sealed record BooleanLocalValue(bool Value) : PrimitiveProtocolLocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "boolean";
-}
-
-public sealed record BigIntLocalValue(string Value) : PrimitiveProtocolLocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "bigint";
-}
-
-public sealed record ChannelLocalValue(ChannelProperties Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "channel";
-}
-
-public sealed record ArrayLocalValue(IEnumerable<LocalValue> Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "array";
-}
-
-public sealed record DateLocalValue(string Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "date";
-}
-
-public sealed record MapLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "map";
-}
-
-public sealed record ObjectLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "object";
-}
-
-public sealed record RegExpLocalValue(RegExpValue Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "regexp";
-}
-
-public sealed record SetLocalValue(IEnumerable<LocalValue> Value) : LocalValue
-{
-    [JsonInclude]
-    internal string Type { get; } = "set";
-}
-

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities to centralize logic and reduce repetition.

Low
  • Update

Copy link
Contributor

@RenderMichael RenderMichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing holding this change back is some tests. Unlike other changes, we have good test coverage here, so no major overhaul is needed.

@nvborisenko
Copy link
Member Author

@RenderMichael should be fine now, please review again.

This was referenced Jan 19, 2026
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.

[dotnet] [bidi] We are missing script.RemoteReference type (https://w3c.github.io/webdriver-bidi/#type-script-LocalValue)

3 participants