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] Annotate nullability on Firefox profile #15207

Merged
merged 1 commit into from
Feb 2, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 1, 2025

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.

Annotate nullability on Firefox profile

Motivation and Context

Contributes to #14640

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

  • Annotated nullability for FirefoxProfile and related classes.

  • Refactored FirefoxProfile to use nullable reference types and improve code clarity.

  • Added nullability checks and exceptions for better error handling.

  • Updated utility methods to support nullable parameters and improve robustness.


Changes walkthrough 📝

Relevant files
Enhancement
FirefoxProfile.cs
Refactor and annotate nullability in FirefoxProfile           

dotnet/src/webdriver/Firefox/FirefoxProfile.cs

  • Enabled nullable reference types for the file.
  • Refactored fields and properties to support nullability.
  • Added nullability checks and exceptions for method parameters.
  • Improved method signatures and internal logic for clarity and safety.
  • +40/-43 
    FirefoxProfileManager.cs
    Add nullability and refactor FirefoxProfileManager             

    dotnet/src/webdriver/Firefox/FirefoxProfileManager.cs

  • Enabled nullable reference types for the file.
  • Refactored GetProfile method to handle nullable inputs.
  • Simplified ExistingProfiles property with expression-bodied syntax.
  • Used switch expressions for platform-specific logic.
  • +13/-31 
    FileUtilities.cs
    Enhance FileUtilities with nullability support                     

    dotnet/src/webdriver/Internal/FileUtilities.cs

  • Updated DeleteDirectory to accept nullable parameters.
  • Added conditional compilation for GenerateRandomTempDirectoryName
    parameter attributes.
  • Improved robustness of utility methods.
  • +6/-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.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Feb 1, 2025

    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 Reference

    The DirectoryInfo.Parent property access in DeleteExtensionsCache() assumes the parent directory exists. While marked with '!', this could still throw NullReferenceException if the extensions directory is at root level.

    string cacheFile = Path.Combine(ex.Parent!.FullName, "extensions.cache");
    File Operations

    The DeleteLockFiles() method attempts to delete files without checking if they exist first. This could throw exceptions if the files don't exist.

    File.Delete(Path.Combine(profileDirectory, ".parentlock"));
    File.Delete(Path.Combine(profileDirectory, "parent.lock"));

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null check for required property

    Add null check for ProfileDirectory before using it in ToBase64String() to avoid
    potential NullReferenceException. The WriteToDisk() call may not have been made yet.

    dotnet/src/webdriver/Firefox/FirefoxProfile.cs [209]

    +if (string.IsNullOrEmpty(this.ProfileDirectory))
    +{
    +    throw new InvalidOperationException("Profile directory not created. Call WriteToDisk() first.");
    +}
     string[] files = Directory.GetFiles(this.ProfileDirectory, "*.*", SearchOption.AllDirectories);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical null reference safety issue by adding a check for ProfileDirectory before using it in ToBase64String(). Without this check, the code could throw a NullReferenceException if WriteToDisk() hasn't been called.

    8
    Validate required method parameter

    Add null check for profileDirectory parameter in DeleteLockFiles() to prevent
    potential NullReferenceException when combining paths.

    dotnet/src/webdriver/Firefox/FirefoxProfile.cs [236-240]

     private void DeleteLockFiles(string profileDirectory)
     {
    +    ArgumentNullException.ThrowIfNull(profileDirectory);
         File.Delete(Path.Combine(profileDirectory, ".parentlock"));
         File.Delete(Path.Combine(profileDirectory, "parent.lock"));
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code safety by adding parameter validation using ArgumentNullException.ThrowIfNull(), which aligns with the PR's focus on nullability and prevents potential runtime errors.

    7
    Learned
    best practice
    Add null validation checks at the start of methods for required parameters marked as nullable to prevent NullReferenceExceptions

    The SetPreference methods should validate the name parameter at the start of the
    method since it's required but marked as nullable. Add ArgumentNullException checks
    using the recommended pattern.

    dotnet/src/webdriver/Firefox/FirefoxProfile.cs [127-129]

     public void SetPreference(string name, string value)
     {
    +    ArgumentNullException.ThrowIfNull(name, nameof(name));
    +    ArgumentNullException.ThrowIfNull(value, nameof(value));
         this.profilePreferences.SetPreference(name, value);
     }
    • Apply this suggestion
    6

    @RenderMichael RenderMichael merged commit 7b45115 into SeleniumHQ:trunk Feb 2, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the firefox-profile branch February 2, 2025 06:20
    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.

    1 participant