Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 5, 2025

User description

Contributes to #14480

💥 What does this PR do?

Generated json serialization now treats enums? Meaning AOT safe?

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Enable AOT-safe enum serialization in BiDi JSON context

  • Add UseStringEnumConverter option for string-based enum handling


Diagram Walkthrough

flowchart LR
  A["BiDi JSON Context"] --> B["Add UseStringEnumConverter"] --> C["AOT Safe Enums"]
Loading

File Walkthrough

Relevant files
Bug fix
BiDiJsonSerializerContext.cs
Add AOT-safe enum serialization configuration                       

dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs

  • Added JsonSourceGenerationOptions attribute with
    UseStringEnumConverter = true
  • Enables AOT-compatible enum serialization for BiDi communication
+2/-0     

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Oct 5, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 5, 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
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables AOT-safe enum serialization in the BiDi JSON serialization context by adding the UseStringEnumConverter option to the JSON source generation configuration.

  • Adds JsonSourceGenerationOptions attribute with UseStringEnumConverter = true to enable string-based enum serialization
  • Ensures compatibility with .NET's Native AOT compilation for BiDi communication

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@nvborisenko
Copy link
Member Author

@RenderMichael do you know? (hard to debug)

Copy link
Contributor

qodo-merge-pro bot commented Oct 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect enum serialization casing
Suggestion Impact:The line [JsonSourceGenerationOptions(UseStringEnumConverter = true)] was removed from the serializer context, aligning with the suggestion to drop this option.

code diff:

-[JsonSourceGenerationOptions(UseStringEnumConverter = true)]
-

The UseStringEnumConverter option serializes enums to PascalCase, but the BiDi
specification requires lowercase. Remove this option and instead implement
custom JsonConverters for each enum to ensure correct serialization.

dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs [189]

-[JsonSourceGenerationOptions(UseStringEnumConverter = true)]
+// This line should be removed.
+// [JsonSourceGenerationOptions(UseStringEnumConverter = true)]

[Suggestion processed]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical issue where using UseStringEnumConverter would cause enum serialization to violate the WebDriver BiDi specification (PascalCase vs. lowercase), leading to communication failures.

High
  • Update

@nvborisenko
Copy link
Member Author

Oh, it is still not correct. We have added global converter:

new JsonStringEnumConverter(JsonNamingPolicy.CamelCase)

As soon as I remove this converter, then enums are converted as PascalCase. Will do CamelCase custom converter.

@nvborisenko
Copy link
Member Author

I added CamelCaseEnumConverter<TEnum> - looks good. Now I need to add attribute for all known enums, will do. But the question is: will it be AOT safe? Or I still need to add all known enums to SerializerContext?

@RenderMichael
Copy link
Contributor

will it be AOT safe?

Yes, this is AOT-safe and the preferred way of enabling this behavior.

I still need to add all known enums to SerializerContext?

These enums are already known to the serializer context because they are properties of types which are known to the serializer context.

@RenderMichael
Copy link
Contributor

Now I need to add attribute for all known enums, will do

I much prefer this approach for all converter attributes, because we are safer to use multiple serializer contexts, and let users serialize the types with their own serializer. What do you think @nvborisenko ? Not in this PR, either way, but want to know your opinion.

@nvborisenko
Copy link
Member Author

Absolutely, we already discussed that DTO objects might be reused across modules, even across extension modules (like #15329). Seems we are on the way to apply converters via attributes. Not in this PR, but yes: design approach is approved. Thanks for your help.

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.

Nice update!

@nvborisenko nvborisenko merged commit 3ce647f into SeleniumHQ:trunk Oct 5, 2025
10 checks passed
@nvborisenko nvborisenko deleted the bidi-enum-aot branch October 5, 2025 19:10
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.

3 participants