Skip to content

[dotnet] [bidi] Additional json data for commands/results#17614

Open
nvborisenko wants to merge 4 commits into
SeleniumHQ:trunkfrom
nvborisenko:bidi-additional-data
Open

[dotnet] [bidi] Additional json data for commands/results#17614
nvborisenko wants to merge 4 commits into
SeleniumHQ:trunkfrom
nvborisenko:bidi-additional-data

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

Supporting serialization of additional data (both on params and envelope levels) for commands and results.

Usage:

await context.NavigateAsync("https://example.com", new NavigateOptions
{
    Timeout = TimeSpan.FromSeconds(30),
    AdditionalData = ImmutableDictionary<string, JsonElement>.Empty
        .Add("myExtension", JsonSerializer.SerializeToElement("value"))
});
var tree = await bidi.BrowsingContext.GetTreeAsync();
if (tree.AdditionalData.TryGetValue("experimentalField", out var val))
{
    Console.WriteLine(val.GetString());
}

🔗 Related Issues

Contributes to #16095

💥 What does this PR do?

This pull request enhances how additional, non-standard data is handled in BiDi command messages and results. It introduces support for capturing and serializing extra properties in both outgoing and incoming messages, ensuring that any data not explicitly modeled in the API can be preserved and accessed. The changes also add new properties to the Parameters, CommandOptions, and EmptyResult types to manage this data in a type-safe way.

Support for additional message data in BiDi commands:

  • Added logic in Broker.cs to serialize any extra key-value pairs from CommandOptions.AdditionalMessageData into outgoing messages, allowing clients to send custom properties with commands.
  • Updated message processing in Broker.cs to capture and store any unrecognized properties from incoming messages into an additionalMessageData dictionary, which is then attached to the result object. [1] [2] [3]

Type and API enhancements for extensibility:

  • Updated the Parameters and EmptyResult types to support extension data via the [JsonExtensionData] attribute, and exposed this data as immutable dictionaries for safer access.
  • Added AdditionalData and AdditionalMessageData properties to CommandOptions and EmptyResult for consistent handling of extra properties throughout the command lifecycle.
  • Added necessary using statements for new types and attributes in Command.cs.

🔧 Implementation Notes

Key decisions:

  • We mix serializable types and public surface - so [JsonAdditionalData] should be public (with setter). Not big deal - just hide this member and provide immutable accessor.
  • Serializable types... should be recognized by external json serializers - this is one more reason for public setter (see previous point).
  • AdditionalData and AdditionalMessageData - hopefully this is good naming (I avoid Extension word intentionally to not collide with WebExtension)

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

EventArgs will use the same approach, will slightly different code base.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Support additional JSON data serialization in BiDi commands and results

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add support for serializing additional JSON data in BiDi commands
• Capture unrecognized properties from incoming messages into dictionaries
• Expose additional data via immutable dictionary accessors in Parameters and EmptyResult
• Support both command-level and message-level extension data handling
Diagram
flowchart LR
  A["Outgoing Command"] -->|"AdditionalMessageData"| B["Broker.ExecuteAsync"]
  B -->|"Serialize extra properties"| C["JSON Message"]
  D["Incoming JSON Message"] -->|"Unrecognized properties"| E["Broker.ProcessReceivedMessage"]
  E -->|"Capture into dictionary"| F["EmptyResult.AdditionalMessageData"]
  G["Parameters/EmptyResult"] -->|"RawAdditionalData + JsonExtensionData"| H["Immutable AdditionalData accessor"]

Loading

Grey Divider

File Changes

1. dotnet/src/webdriver/BiDi/Broker.cs ✨ Enhancement +17/-0

Add additional data serialization and capture logic

• Serialize AdditionalMessageData from CommandOptions into outgoing JSON messages
• Capture unrecognized properties during message parsing into additionalMessageData dictionary
• Attach captured additional data to EmptyResult.AdditionalMessageData after deserialization
• Preserve extension data throughout the command lifecycle

dotnet/src/webdriver/BiDi/Broker.cs


2. dotnet/src/webdriver/BiDi/Command.cs ✨ Enhancement +36/-1

Add extension data properties to command types

• Add using statements for System.ComponentModel, System.Text.Json, and
 System.Text.Json.Serialization
• Add RawAdditionalData property with [JsonExtensionData] attribute to Parameters class
• Add immutable AdditionalData accessor property to Parameters for safe access
• Add AdditionalData and AdditionalMessageData properties to CommandOptions abstract record
• Add RawAdditionalData and AdditionalData properties to EmptyResult abstract record
• Add AdditionalMessageData property to EmptyResult for storing captured extension data

dotnet/src/webdriver/BiDi/Command.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 2, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

Grey Divider


Action required

1. Options AdditionalData dropped 🐞 Bug ≡ Correctness
Description
CommandOptions.AdditionalData is never merged into the serialized "params" object, so callers
setting options.AdditionalData will silently lose those fields in outgoing commands. Since command
parameter objects are internal and constructed without copying options.AdditionalData, public APIs
effectively cannot send params-level extension fields.
Code

dotnet/src/webdriver/BiDi/Command.cs[R54-58]

Evidence
The outgoing message writer only serializes @params into the params property and then appends
options.AdditionalMessageData at the envelope level; options.AdditionalData is never read.
Module code constructs internal *Parameters instances from selected option fields (e.g., Wait)
without copying options.AdditionalData, and because the *Parameters types are internal, callers
cannot set Parameters.AdditionalData directly.

dotnet/src/webdriver/BiDi/Broker.cs[68-109]
dotnet/src/webdriver/BiDi/Command.cs[50-59]
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs[70-75]
dotnet/src/webdriver/BiDi/BrowsingContext/Navigate.cs[25-30]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CommandOptions.AdditionalData` is defined as part of the public options surface, but it is never applied to the serialized command `params`. As a result, options-based additional params data is silently dropped, and callers cannot practically use params-level extension data because the concrete `*Parameters` types are internal and created inside modules.

## Issue Context
`Broker.ExecuteAsync` serializes only `@params` as `"params"`, and only uses `options.AdditionalMessageData` for envelope-level additions.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[68-109]
- dotnet/src/webdriver/BiDi/Command.cs[50-59]

## Implementation guidance
- In `Broker.ExecuteAsync` (centralized fix): if `options?.AdditionalData` is non-empty, merge it into `@params.RawAdditionalData` (or set `@params.AdditionalData` via the backing `RawAdditionalData`) before calling `JsonSerializer.Serialize`.
- Define merge behavior for key collisions (preferably throw a clear exception if the same key already exists in `@params.RawAdditionalData`).
- Add/adjust tests to assert that `options.AdditionalData` appears inside the serialized `params` object.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Reserved keys overridable 🐞 Bug ☼ Reliability
Description
Broker.ExecuteAsync writes options.AdditionalMessageData properties at the message root without
filtering reserved keys, allowing duplicates of "id", "method", or "params" in the emitted JSON. If
the remote end honors the last duplicate name, command correlation can break (timeouts or completing
the wrong command).
Code

dotnet/src/webdriver/BiDi/Broker.cs[R100-107]

Evidence
The writer emits reserved envelope fields (id, method, params) and then blindly writes
additional properties using user-provided keys. Nothing prevents AdditionalMessageData from
containing the same names, and Utf8JsonWriter will emit duplicates rather than error.

dotnet/src/webdriver/BiDi/Broker.cs[95-109]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`AdditionalMessageData` is appended to the outgoing command envelope as arbitrary root-level JSON properties. Without validation, callers can provide keys like `id`, `method`, or `params`, producing duplicate JSON property names and potentially breaking protocol semantics and command correlation.

## Issue Context
Outgoing envelope already writes `id`, `method`, and `params` before iterating `AdditionalMessageData`.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[95-109]

## Implementation guidance
- Add a reserved-name check before writing each `AdditionalMessageData` entry.
 - At minimum reject: `id`, `method`, `params`.
 - Consider also rejecting other envelope keys that appear in incoming messages: `type`, `result`, `error`, `message`.
- On collision, throw an `ArgumentException` (or similar) with a clear message so failures are deterministic and debuggable.
- Add unit/serialization tests ensuring reserved keys are rejected and non-reserved keys serialize correctly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 2, 2026
@nvborisenko nvborisenko requested a review from RenderMichael June 2, 2026 18:56
Comment on lines +54 to +58
public ImmutableDictionary<string, JsonElement> AdditionalData { get; init; }
= ImmutableDictionary<string, JsonElement>.Empty;

public ImmutableDictionary<string, JsonElement> AdditionalMessageData { get; init; }
= ImmutableDictionary<string, JsonElement>.Empty;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. Options additionaldata dropped 🐞 Bug ≡ Correctness

CommandOptions.AdditionalData is never merged into the serialized "params" object, so callers
setting options.AdditionalData will silently lose those fields in outgoing commands. Since command
parameter objects are internal and constructed without copying options.AdditionalData, public APIs
effectively cannot send params-level extension fields.
Agent Prompt
## Issue description
`CommandOptions.AdditionalData` is defined as part of the public options surface, but it is never applied to the serialized command `params`. As a result, options-based additional params data is silently dropped, and callers cannot practically use params-level extension data because the concrete `*Parameters` types are internal and created inside modules.

## Issue Context
`Broker.ExecuteAsync` serializes only `@params` as `"params"`, and only uses `options.AdditionalMessageData` for envelope-level additions.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[68-109]
- dotnet/src/webdriver/BiDi/Command.cs[50-59]

## Implementation guidance
- In `Broker.ExecuteAsync` (centralized fix): if `options?.AdditionalData` is non-empty, merge it into `@params.RawAdditionalData` (or set `@params.AdditionalData` via the backing `RawAdditionalData`) before calling `JsonSerializer.Serialize`.
- Define merge behavior for key collisions (preferably throw a clear exception if the same key already exists in `@params.RawAdditionalData`).
- Add/adjust tests to assert that `options.AdditionalData` appears inside the serialized `params` object.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +100 to +107
if (options is not null)
{
foreach (var kvp in options.AdditionalMessageData)
{
writer.WritePropertyName(kvp.Key);
kvp.Value.WriteTo(writer);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

2. Reserved keys overridable 🐞 Bug ☼ Reliability

Broker.ExecuteAsync writes options.AdditionalMessageData properties at the message root without
filtering reserved keys, allowing duplicates of "id", "method", or "params" in the emitted JSON. If
the remote end honors the last duplicate name, command correlation can break (timeouts or completing
the wrong command).
Agent Prompt
## Issue description
`AdditionalMessageData` is appended to the outgoing command envelope as arbitrary root-level JSON properties. Without validation, callers can provide keys like `id`, `method`, or `params`, producing duplicate JSON property names and potentially breaking protocol semantics and command correlation.

## Issue Context
Outgoing envelope already writes `id`, `method`, and `params` before iterating `AdditionalMessageData`.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[95-109]

## Implementation guidance
- Add a reserved-name check before writing each `AdditionalMessageData` entry.
  - At minimum reject: `id`, `method`, `params`.
  - Consider also rejecting other envelope keys that appear in incoming messages: `type`, `result`, `error`, `message`.
- On collision, throw an `ArgumentException` (or similar) with a clear message so failures are deterministic and debuggable.
- Add unit/serialization tests ensuring reserved keys are rejected and non-reserved keys serialize correctly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@nvborisenko
Copy link
Copy Markdown
Member Author

@RenderMichael I used ImmutableDictionary<string, JsonElement>. Since it is fully controlled by us, we can use any another more convenient json type (immutable only!). Will explore potential nice candidates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-dotnet .NET Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants