Skip to content

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

Merged
nvborisenko merged 29 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-aot-classic
Oct 28, 2024
Merged

[dotnet] Make classic WebDriver commands/responses AOT compatible#14574
nvborisenko merged 29 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-aot-classic

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 7, 2024 •
edited
Loading

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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Oct 7, 2024

    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.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Oct 7, 2024 •
    edited
    Loading

    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 mentioned this pull request Oct 7, 2024
    Open
    7 tasks
    @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

    nvborisenko commented Oct 17, 2024

    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 nvborisenko mentioned this pull request Oct 17, 2024
    Closed
    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Oct 19, 2024

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

    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Oct 22, 2024

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

    @nvborisenko nvborisenko mentioned this pull request Oct 23, 2024
    Merged
    8 tasks
    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Oct 28, 2024

    Appium tests on Android passed.

    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Oct 28, 2024

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

    @nvborisenko nvborisenko merged commit 5d3414d into SeleniumHQ:trunk Oct 28, 2024
    nvborisenko added a commit that referenced this pull request Oct 28, 2024
    @nvborisenko
    [dotnet] Make classic WebDriver commands/responses AOT compatible (#1…
    1cb48a2 
    …4574)
    @nvborisenko nvborisenko deleted the dotnet-aot-classic branch February 6, 2025 18:17
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

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

    1 participant

    @nvborisenko