Skip to content
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

[dotnet] Make classic WebDriver commands/responses AOT compatible #14574

Merged
merged 29 commits into from
Oct 28, 2024

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 7, 2024

User description

Description

Road to AOT compatible.

Motivation and Context

Related to #14480, fixes #13363

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


Description

  • Enhanced the serialization process for commands and responses to improve Ahead-Of-Time (AOT) compatibility.
  • Introduced SerializableCommand and DeserializableResponse classes with JsonExtensionData to handle additional JSON data.
  • Implemented CommandSerializerContext and ResponseSerializerContext to manage JSON serialization contexts.

Changes walkthrough 📝

Relevant files
Enhancement
Command.cs
Enhance command serialization for AOT compatibility           

dotnet/src/webdriver/Command.cs

  • Modified serialization of command parameters to use
    SerializableCommand.
  • Introduced SerializableCommand class with JsonExtensionData.
  • Added CommandSerializerContext for JSON serialization context.
  • +16/-1   
    Response.cs
    Improve response serialization for AOT compatibility         

    dotnet/src/webdriver/Response.cs

  • Updated JSON serializer options to use ResponseSerializerContext.
  • Added DeserializableResponse class with JsonExtensionData.
  • Introduced ResponseSerializerContext for JSON serialization context.
  • +15/-1   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    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

    Potential Performance Impact
    The new serialization approach using SerializableCommand might have performance implications compared to the previous direct serialization of commandParameters.

    Backwards Compatibility
    The changes in the Response class, particularly the new DeserializableResponse and ResponseSerializerContext, might affect backwards compatibility with existing code that relies on the previous serialization behavior.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 7, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Initialize the dictionary property to prevent potential null reference exceptions

    Consider initializing the Data property in the SerializableCommand class constructor
    to avoid potential null reference exceptions.

    dotnet/src/webdriver/Command.cs [137-141]

     internal class SerializableCommand
     {
         [JsonExtensionData]
    -    public Dictionary<string, object> Data { get; set; }
    +    public Dictionary<string, object> Data { get; set; } = new Dictionary<string, object>();
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Initializing the Data property in the SerializableCommand class constructor is a good practice to prevent null reference exceptions, which can lead to runtime errors. This suggestion enhances the robustness of the code.

    8
    Best practice
    Seal the class to prevent unintended inheritance and potential serialization issues

    Consider making the SerializableCommand class sealed to prevent inheritance, as it's
    designed for serialization purposes.

    dotnet/src/webdriver/Command.cs [137-141]

    -internal class SerializableCommand
    +internal sealed class SerializableCommand
     {
         [JsonExtensionData]
         public Dictionary<string, object> Data { get; set; }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Making the SerializableCommand class sealed is a reasonable suggestion to prevent inheritance, which is typically unnecessary for classes designed solely for serialization. This can help avoid potential misuse and maintain the intended design.

    7
    Seal the class to prevent unintended inheritance and potential deserialization issues

    Consider making the DeserializableResponse class sealed to prevent inheritance, as
    it's designed for deserialization purposes.

    dotnet/src/webdriver/Response.cs [214-218]

    -internal class DeserializableResponse
    +internal sealed class DeserializableResponse
     {
         [JsonExtensionData]
         public Dictionary<string, object> Data { get; set; }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Similar to the previous suggestion, sealing the DeserializableResponse class is a good practice to prevent unintended inheritance. This ensures the class is used as intended for deserialization purposes, enhancing code maintainability.

    7

    💡 Need additional feedback ? start a PR chat

    @nvborisenko nvborisenko changed the title [dotnet] Make classic WebDriver commands/responses AOT comapatible [dotnet] Make classic WebDriver commands/responses AOT compatible Oct 7, 2024
    @nvborisenko
    Copy link
    Member Author

    Current state of this PR:

    • Our unit tests pass
    • I verified manually simple scenario within published AOT
    • I looked though examples, provided by SauceLabs, BrowserStack, Appium (even source code) - should work

    But there is a risk I missed some primitive type (like ulong) and didn't declare it as supported type. I broadcasted community asking help. Our goal here is identify what primitive types community uses before merging it. If community is silent - then merge it.

    @nvborisenko
    Copy link
    Member Author

    I have added all built-in primitive types. So waiting community feedback (or not), and then just merge.

    @nvborisenko
    Copy link
    Member Author

    Appium is friendly to test it, already identified some issues - waiting new round of testing.

    @nvborisenko
    Copy link
    Member Author

    Appium tests on Android passed.

    @nvborisenko
    Copy link
    Member Author

    Thanks all, merging it to be included into upcoming v4.26

    @nvborisenko nvborisenko merged commit 5d3414d into SeleniumHQ:trunk Oct 28, 2024
    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.

    [🐛 Bug]: driver.Manage().Cookies.AddCookie(); with Native aot(c#)
    1 participant