Skip to content

[dotnet] [bidi] Fix reflection based deserialization of additional data#17648

Merged
nvborisenko merged 1 commit into
SeleniumHQ:trunkfrom
nvborisenko:bidi-fix-additionaldata-reflection
Jun 6, 2026
Merged

[dotnet] [bidi] Fix reflection based deserialization of additional data#17648
nvborisenko merged 1 commit into
SeleniumHQ:trunkfrom
nvborisenko:bidi-fix-additionaldata-reflection

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

Fixes:

System.InvalidOperationException: Reflection-based serialization has been disabled for this application. Either use the source generator APIs or explicitly configure the 'JsonSerializerOptions.TypeInfoResolver' property.

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

🤖 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

🔄 Types of changes

  • Bug fix (backwards compatible)

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix BiDi reflection-based deserialization of additional data

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Replace JsonSerializer.Deserialize with JsonElement.ParseValue
• Fixes reflection-based deserialization error in BiDi message processing
• Resolves disabled reflection serialization exception for additional data
Diagram
flowchart LR
  A["JsonSerializer.Deserialize<JsonElement>"] -->|"Replace with"| B["JsonElement.ParseValue"]
  B -->|"Result"| C["Reflection disabled error fixed"]

Loading

Grey Divider

File Changes

1. dotnet/src/webdriver/BiDi/Broker.cs 🐞 Bug fix +1/-1

Replace JsonSerializer with JsonElement.ParseValue

• Changed deserialization method in ProcessReceivedMessage method
• Replaced JsonSerializer.Deserialize(ref reader) with JsonElement.ParseValue(ref reader)
• Fixes InvalidOperationException related to disabled reflection-based serialization
• Maintains same functionality while using reflection-safe API

dotnet/src/webdriver/BiDi/Broker.cs


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 6, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 6, 2026

Code Review by Qodo

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

Context used

Grey Divider


Remediation recommended

1. ProcessReceivedMessage lacks regression test 📘 Rule violation ☼ Reliability
Description
ProcessReceivedMessage now parses additional top-level fields via `JsonElement.ParseValue(ref
reader)`, changing the message parsing behavior to address reflection-disabled serialization
scenarios. The PR does not add/update a test that exercises this reflection-disabled configuration,
increasing regression risk for the reported failure mode.
Code

dotnet/src/webdriver/BiDi/Broker.cs[266]

+                additionalMessageData[propName] = JsonElement.ParseValue(ref reader);
Evidence
PR Compliance ID 6 requires adding/updating tests for implemented fixes. The change at
Broker.cs:266 alters JSON parsing for additional message fields to address reflection-disabled
deserialization, but no corresponding test is added/updated to validate that this scenario works and
remains stable.

AGENTS.md: Add or Update Tests for Implemented Fixes and Prefer Small Unit Tests
dotnet/src/webdriver/BiDi/Broker.cs[263-267]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[119-127]

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

## Issue description
The fix changes additional-data parsing to avoid reflection-based `JsonSerializer.Deserialize<JsonElement>(...)`, but there is no unit test that fails under `System.Text.Json` reflection-disabled mode and passes with the new parsing logic.

## Issue Context
The reported exception only happens when reflection-based serialization is disabled (common in AOT/trimming scenarios). Add a targeted unit test that toggles the relevant `AppContext` switch (and restores the prior value/state) and verifies that receiving a message with top-level additional fields does not throw and still populates `AdditionalMessageData`.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[263-267]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[119-127]

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


Grey Divider

Qodo Logo

@nvborisenko nvborisenko merged commit e7a36e2 into SeleniumHQ:trunk Jun 6, 2026
23 checks passed
@nvborisenko nvborisenko deleted the bidi-fix-additionaldata-reflection branch June 6, 2026 11:02
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