Skip to content

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

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

Open
nvborisenko wants to merge 4 commits into
base: trunk
Choose a base branch
from
+53 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dea5420
Commands/Results
nvborisenko Jun 2, 2026
c7f16d8
Envelope result
nvborisenko Jun 2, 2026
ed62ded
Fix null
nvborisenko Jun 2, 2026
5a87e46
Update Command.cs
nvborisenko Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions dotnet/src/webdriver/BiDi/Broker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ public async Task<TResult> ExecuteAsync<TParameters, TResult>(Command<TParameter
writer.WriteString("method"u8, descriptor.Method);
writer.WritePropertyName("params"u8);
JsonSerializer.Serialize(writer, @params, descriptor.ParamsTypeInfo);
if (options is not null)
{
foreach (var kvp in options.AdditionalMessageData)
{
writer.WritePropertyName(kvp.Key);
kvp.Value.WriteTo(writer);
}
}
Comment on lines +100 to +107
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Jun 2, 2026

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

writer.WriteEndObject();
}
}
Expand Down Expand Up @@ -189,6 +197,7 @@ private void ProcessReceivedMessage(ReadOnlySpan<byte> data)
string? message = default;
Utf8JsonReader resultReader = default;
Utf8JsonReader paramsReader = default;
ImmutableDictionary<string, JsonElement>.Builder? additionalMessageData = null;

Utf8JsonReader reader = new(data);
reader.Read(); // "{"
Expand Down Expand Up @@ -236,7 +245,10 @@ private void ProcessReceivedMessage(ReadOnlySpan<byte> data)
}
else
{
var propName = reader.GetString()!;
reader.Read();
additionalMessageData ??= ImmutableDictionary.CreateBuilder<string, JsonElement>();
additionalMessageData[propName] = JsonSerializer.Deserialize<JsonElement>(ref reader);
}

reader.Skip();
Expand All @@ -255,6 +267,11 @@ private void ProcessReceivedMessage(ReadOnlySpan<byte> data)
var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo)
?? throw new BiDiException("Remote end returned null command result in the 'result' property.");

if (additionalMessageData is not null)
{
((EmptyResult)commandResult).AdditionalMessageData = additionalMessageData.ToImmutable();
}

command.TaskCompletionSource.TrySetResult((EmptyResult)commandResult);
}
catch (Exception ex)
Expand Down
37 changes: 36 additions & 1 deletion dotnet/src/webdriver/BiDi/Command.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
// under the License.
// </copyright>

using System.ComponentModel;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;

namespace OpenQA.Selenium.BiDi;
Expand All @@ -31,11 +34,43 @@ public readonly record struct Command<TParameters, TResult>(
public record Parameters
{
public static Parameters Empty { get; } = new Parameters();

[JsonExtensionData]
[EditorBrowsable(EditorBrowsableState.Never)]
public Dictionary<string, JsonElement>? RawAdditionalData { get; set; }

[JsonIgnore]
public ImmutableDictionary<string, JsonElement> AdditionalData
{
get => RawAdditionalData?.ToImmutableDictionary() ?? ImmutableDictionary<string, JsonElement>.Empty;
init => RawAdditionalData = value is null || value.IsEmpty ? null : new(value);
}
}

public abstract record CommandOptions
{
public TimeSpan? Timeout { get; init; }

public ImmutableDictionary<string, JsonElement> AdditionalData { get; init; }
= ImmutableDictionary<string, JsonElement>.Empty;

public ImmutableDictionary<string, JsonElement> AdditionalMessageData { get; init; }
= ImmutableDictionary<string, JsonElement>.Empty;
Comment on lines +54 to +58
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Jun 2, 2026

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

}

public abstract record EmptyResult;
public abstract record EmptyResult
{
[JsonExtensionData]
[EditorBrowsable(EditorBrowsableState.Never)]
public Dictionary<string, JsonElement>? RawAdditionalData { get; set; }

[JsonIgnore]
public ImmutableDictionary<string, JsonElement> AdditionalData
{
get => RawAdditionalData?.ToImmutableDictionary() ?? ImmutableDictionary<string, JsonElement>.Empty;
init => RawAdditionalData = value is null || value.IsEmpty ? null : new(value);
}

public ImmutableDictionary<string, JsonElement> AdditionalMessageData { get; internal set; }
= ImmutableDictionary<string, JsonElement>.Empty;
}
Loading