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 FirefoxProfile AOT-safe #14742

Merged
merged 13 commits into from
Nov 16, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 11, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This PR hand-rolls parsing of the webdriver_prefs.json file, instead of parsing into a Dictionary<string, object>

This PR is a lot easier to read with whitespace disabled.

Motivation and Context

An open-typed object dictionary is inherently AOT-unsafe. In this scenario, we know the expected structure of the JSON, so we can manually parse the values.

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

  • Refactored JSON parsing in FirefoxProfile to use JsonObject instead of Dictionary<string, object>, enhancing AOT safety.
  • Updated Preferences class to handle preferences using JsonNode, improving type safety and performance.
  • Added nullability annotations to improve code quality and prevent null reference exceptions.
  • Enhanced exception handling for preference setting and JSON parsing.

Changes walkthrough 📝

Relevant files
Enhancement
FirefoxProfile.cs
Refactor JSON Parsing to Use JsonObject for AOT Safety     

dotnet/src/webdriver/Firefox/FirefoxProfile.cs

  • Replaced Dictionary with JsonObject for JSON parsing.
  • Removed custom JSON serialization options.
  • Improved exception handling for JSON parsing.
  • +6/-12   
    Preferences.cs
    Update Preferences Handling with JsonNode and Nullability

    dotnet/src/webdriver/Firefox/Preferences.cs

  • Changed preference storage to use JsonNode.
  • Updated SetPreferenceValue to handle JsonNode.
  • Added nullability annotations.
  • Improved exception handling for preference setting.
  • +40/-26 

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Handling
    The code assumes JsonNode values will be JsonValue and can be converted to primitive types. Need to validate the error handling for null or invalid JSON node types.

    Error Handling
    The null coalescing operator for JsonObject casting should include more descriptive error message about the expected JSON structure.

    Type Safety
    The preference value parsing logic could be more robust by handling additional numeric types like double/float that may appear in the JSON.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 11, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add input validation to prevent null reference exceptions when parsing JSON data

    Add null check for defaultPreferences string before parsing to prevent potential
    NullReferenceException.

    dotnet/src/webdriver/Firefox/FirefoxProfile.cs [303-304]

     string defaultPreferences = reader.ReadToEnd();
    +if (string.IsNullOrEmpty(defaultPreferences))
    +    throw new WebDriverException("webdriver_prefs.json was empty");
     JsonObject deserializedPreferences = JsonNode.Parse(defaultPreferences) as JsonObject
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important null validation for JSON data, preventing potential runtime crashes. This is a critical defensive programming practice for handling external data.

    8
    Possible bug
    ✅ Add parameter validation to prevent null reference exceptions
    Suggestion Impact:The commit added a null check for the key parameter in the SetPreference method, which aligns with the suggestion to prevent null reference exceptions.

    code diff:

             internal void SetPreference(string key, string value)
             {
    -            this.SetPreferenceValue(key, value);
    +            if (key is null)
    +            {
    +                throw new ArgumentNullException(nameof(key));
    +            }

    Add null check for key parameter in SetPreferenceValue method to prevent potential
    issues with null keys.

    dotnet/src/webdriver/Firefox/Preferences.cs [166-168]

     private void SetPreferenceValue(string key, JsonNode? value)
     {
    +    if (key == null)
    +        throw new ArgumentNullException(nameof(key));
         if (!this.IsSettablePreference(key))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding null validation for method parameters is a crucial defensive programming practice that prevents potential null reference exceptions and improves code reliability.

    7

    💡 Need additional feedback ? start a PR chat

    @RenderMichael
    Copy link
    Contributor Author

    Support for floating-point preferences has been added and the error message has been changed to reflect that.

    This PR is ready for another look!

    @nvborisenko nvborisenko merged commit 83d52f8 into SeleniumHQ:trunk Nov 16, 2024
    10 checks passed
    @nvborisenko
    Copy link
    Member

    Thank you, @RenderMichael !

    jkim2492 pushed a commit to jkim2492/selenium that referenced this pull request Nov 17, 2024
    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.

    2 participants