[dotnet] [bidi] Additional json data for commands/results#17614
[dotnet] [bidi] Additional json data for commands/results#17614nvborisenko wants to merge 4 commits into
Conversation
Review Summary by QodoSupport additional JSON data serialization in BiDi commands and results
WalkthroughsDescription• 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 Diagramflowchart 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"]
File Changes1. dotnet/src/webdriver/BiDi/Broker.cs
|
Code Review by Qodo
Context used 1. Options AdditionalData dropped
|
| public ImmutableDictionary<string, JsonElement> AdditionalData { get; init; } | ||
| = ImmutableDictionary<string, JsonElement>.Empty; | ||
|
|
||
| public ImmutableDictionary<string, JsonElement> AdditionalMessageData { get; init; } | ||
| = ImmutableDictionary<string, JsonElement>.Empty; |
There was a problem hiding this comment.
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
| if (options is not null) | ||
| { | ||
| foreach (var kvp in options.AdditionalMessageData) | ||
| { | ||
| writer.WritePropertyName(kvp.Key); | ||
| kvp.Value.WriteTo(writer); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
|
@RenderMichael I used |
Supporting serialization of additional data (both on
paramsandenvelopelevels) for commands and results.Usage:
🔗 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, andEmptyResulttypes to manage this data in a type-safe way.Support for additional message data in BiDi commands:
Broker.csto serialize any extra key-value pairs fromCommandOptions.AdditionalMessageDatainto outgoing messages, allowing clients to send custom properties with commands.Broker.csto capture and store any unrecognized properties from incoming messages into anadditionalMessageDatadictionary, which is then attached to the result object. [1] [2] [3]Type and API enhancements for extensibility:
ParametersandEmptyResulttypes to support extension data via the[JsonExtensionData]attribute, and exposed this data as immutable dictionaries for safer access.AdditionalDataandAdditionalMessageDataproperties toCommandOptionsandEmptyResultfor consistent handling of extra properties throughout the command lifecycle.usingstatements for new types and attributes inCommand.cs.🔧 Implementation Notes
Key decisions:
externaljson serializers - this is one more reason forpublic setter(see previous point).AdditionalDataandAdditionalMessageData- hopefully this is good naming (I avoidExtensionword intentionally to not collide withWebExtension)🤖 AI assistance
💡 Additional Considerations
EventArgswill use the same approach, will slightly different code base.🔄 Types of changes