Skip to content

[dotnet] [bidi] Address BiDi's JSON converter AOT warnings #15390

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.

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

Merged
merged 19 commits into from
Apr 7, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Mar 8, 2025

User description

Description

Addresses warnings users face when trying to publish BiDi code to AOT.

Motivation and Context

These warnings are actually user-facing. Users will see this when running dotnet publish with AOT enabled; we should do our best from surfacing them.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Bug fix


Description

  • Added AOT-safe annotations to JSON converters for BiDi modules.

  • Improved handling of JSON serialization in Command class.

  • Enhanced error handling and compatibility in ChromiumDriver and utilities.

  • Reduced unnecessary warnings during AOT compilation.


Changes walkthrough 📝

Relevant files
Enhancement
13 files
GetClientWindowsResultConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
GetCookiesResultConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
GetRealmsResultConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
GetUserContextsResultConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
InputSourceActionsConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
LocateNodesResultConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
InputOriginConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
EvaluateResultConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
LogEntryConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
MessageConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
RealmInfoConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
RemoteValueConverter.cs
Added AOT-safe annotations to JSON converter                         
+3/-0     
ChromiumDriver.cs
Added AOT compatibility annotations to DevTools session handling
+2/-0     
Bug fix
2 files
Command.cs
Improved JSON serialization and deserialization logic       
+5/-9     
FileUtilities.cs
Enhanced null-checking for assembly location handling       
+3/-2     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 8, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 9491f5a)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incomplete Fix

    The ConvertParametersFromJson method now uses CommandJsonSerializerContext.Default but doesn't handle potential null values or exceptions. The method should include error handling for JSON deserialization failures.

    Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
    return parameters;
    Serialization Options

    The ParametersAsJsonString property uses s_jsonSerializerOptions while ConvertParametersFromJson now uses CommandJsonSerializerContext.Default. This inconsistency could lead to serialization/deserialization mismatches.

    return JsonSerializer.Serialize(this.Parameters, s_jsonSerializerOptions);

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 8, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 9491f5a
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use source-generated serializer

    Use the source-generated serializer context instead of runtime serialization
    options to improve AOT compatibility. The code already has a fix for this
    pattern in the next line, but this instance was missed.

    dotnet/src/webdriver/Command.cs [116]

    -Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, s_jsonSerializerOptions);
    +Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject);

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 9

    __

    Why: This suggestion correctly identifies a missed opportunity to use the source-generated serializer context instead of runtime serialization options. This change is critical for AOT compatibility, which is the main focus of this PR. The fix aligns with the pattern being established throughout the codebase.

    High
    • Update

    Previous suggestions

    Suggestions up to commit 27cdb89
    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Initialize variables before returning them to avoid null reference exceptions and ensure proper functionality

    The ConvertParametersFromJson method is missing the actual deserialization code.
    The method assigns the result to the parameters variable but doesn't initialize
    it. Add the deserialization code using the CommandJsonSerializerContext to
    properly initialize the parameters variable before returning it.

    dotnet/src/webdriver/Command.cs [121-122]

     private static Dictionary<string, object?>? ConvertParametersFromJson(string value)
     {
    +    Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
         return parameters;
     }
    Suggestion importance[1-10]: 6
    Low
    General
    Remove unnecessary variable assignment

    The parameters variable is assigned but the value is never used before being
    returned. The method should directly return the result of the deserialization.

    dotnet/src/webdriver/Command.cs [121-122]

     private static Dictionary<string, object?>? ConvertParametersFromJson(string value)
     {
    -    Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
    -    return parameters;
    +    return JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
     }
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion correctly identifies an unnecessary intermediate variable. The code can be simplified by directly returning the deserialization result, which improves readability and reduces a line of code. This is a minor code style improvement with no functional impact.

    Low

    Sorry, something went wrong.

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
    …r handling
    …into bidi-AOT
    @RenderMichael
    Copy link
    Contributor Author

    With this PR, there are only 4 AOT warnings remaining (really two locations, coming from one single issue: the best-effort AOT support for Command.ParametersAsJsonString

    image

    @nvborisenko
    Copy link
    Member

    I see 2 options:

    • Mark all enums with [CamelCaseJsonEnumStringConverter] attribute
    • Add all enums (one by one) explicitly for BiDiJsonSerializerContext

    I am not sure which way is better. I propose to hold on it until #15329 is resolved. Then we will understand how to "extend" serializer context, and will apply more accurate decision here.

    @RenderMichael
    Copy link
    Contributor Author

    OK, I will break off the enum string changes until a decision is made there. Everything else should still be valid.

    @RenderMichael RenderMichael changed the title [dotnet] Address AOT warnings in BiDi [dotnet] Address and minimize AOT warnings Mar 8, 2025
    @RenderMichael RenderMichael marked this pull request as draft March 11, 2025 17:53
    @RenderMichael RenderMichael marked this pull request as ready for review March 21, 2025 15:20
    @RenderMichael
    Copy link
    Contributor Author

    Only question this PR brought up: JsonStringEnumConverter<TEnum> on each enum, or BiDi-wide JsonStringEnumConverter on the options?

    I removed those changes from this PR. Everything else should be straightforward.

    If we merge this PR, we can enable AOT warnings on the project as a whole, making AOT questions much simpler.

    @RenderMichael RenderMichael changed the title [dotnet] Address and minimize AOT warnings [dotnet] [bidi] Address BiDi AOT warnings Mar 28, 2025
    @RenderMichael RenderMichael changed the title [dotnet] [bidi] Address BiDi AOT warnings [dotnet] [bidi] Address BiDi's JSON converter AOT warnings Mar 28, 2025
    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

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

    Let me mark this PR with changes required, seems there is better way how to resolve this warning (please see inline comment, I didn't verify it). Thanks for understanding, we should research it before introducing new rule. I hope we just miss something, time to learn.

    @nvborisenko
    Copy link
    Member

    Using the overload with JsonTypeInfo helped. Thanks all..

    @nvborisenko nvborisenko merged commit 9935e51 into SeleniumHQ:trunk Apr 7, 2025
    10 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants