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] Add nullability to FirefoxExtension #14964

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 27, 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

Annotates nullability for FirefoxExtension

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


Description

  • Enhanced type safety by adding nullability annotations to FirefoxExtension class
  • Improved null handling in XML and JSON manifest parsing methods
  • Added explicit null checks in constructors and methods with appropriate exceptions
  • Added XML documentation for null parameter exceptions
  • Made fields readonly to improve immutability
  • Simplified JSON parsing logic with null-conditional operators

Changes walkthrough 📝

Relevant files
Enhancement
FirefoxExtension.cs
Add nullability annotations and improve null handling       

dotnet/src/webdriver/Firefox/FirefoxExtension.cs

  • Added nullability annotations with #nullable enable
  • Made fields readonly and added null checks in constructor
  • Improved null handling in XML and JSON parsing methods
  • Added exception documentation for null parameters
  • +19/-24 
    FirefoxProfile.cs
    Add null parameter validation for AddExtension method       

    dotnet/src/webdriver/Firefox/FirefoxProfile.cs

  • Added null check for extensionToInstall parameter
  • Added exception documentation for null parameter
  • +6/-0     

    💡 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

    Null Reference

    The JSON parsing code assumes manifestObject is not null and directly accesses properties with '!'. This could throw NullReferenceException if the manifest.json file is malformed.

    var manifestObject = JsonNode.Parse(File.ReadAllText(manifestJsonPath));
    if (manifestObject!["applications"]?["gecko"]?["id"] is { } idNode)
    {
        id = idNode.ToString().Trim();
    }
    else
    {
        string addInName = manifestObject["name"]!.ToString().Replace(" ", "");
        string addInVersion = manifestObject["version"]!.ToString();
        id = string.Format(CultureInfo.InvariantCulture, "{0}@{1}", addInName, addInVersion);

    public void AddExtension(string extensionToInstall)
    {
    if (extensionToInstall is null)
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    This already throws ArgumentNullException because of the Dictionary.Add just below

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add proper null checks when accessing JSON properties to prevent runtime exceptions

    Add null check for the manifestObject before accessing its properties to prevent
    potential NullReferenceException.

    dotnet/src/webdriver/Firefox/FirefoxExtension.cs [191-192]

    -string addInName = manifestObject["name"]!.ToString().Replace(" ", "");
    -string addInVersion = manifestObject["version"]!.ToString();
    +if (manifestObject?["name"] is null || manifestObject["version"] is null)
    +{
    +    throw new WebDriverException("Extension manifest.json must contain 'name' and 'version' fields");
    +}
    +string addInName = manifestObject["name"].ToString().Replace(" ", "");
    +string addInVersion = manifestObject["version"].ToString();
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Critical suggestion that prevents potential NullReferenceException when accessing required JSON properties. The improved error message also helps with debugging.

    9
    Add parameter validation to prevent null reference exceptions

    Add null check for the profileDirectory parameter in the Install method to match the
    documented exception.

    dotnet/src/webdriver/Firefox/FirefoxExtension.cs [81-83]

     public void Install(string profileDirectory)
     {
    +    if (profileDirectory is null)
    +    {
    +        throw new ArgumentNullException(nameof(profileDirectory));
    +    }
         DirectoryInfo info = new DirectoryInfo(profileDirectory);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a missing null check that should match the documented exception. This is important for maintaining API contract and preventing runtime errors.

    8
    Add validation for XML node existence before accessing its properties

    Add null check for descriptionNode before accessing its attributes to prevent
    potential NullReferenceException.

    dotnet/src/webdriver/Firefox/FirefoxExtension.cs [152-153]

     XmlNode? descriptionNode = rdfXmlDocument.SelectSingleNode("//RDF:Description", rdfNamespaceManager);
    -XmlAttribute? attribute = descriptionNode?.Attributes?["id", EmNamespaceUri];
    +if (descriptionNode is null)
    +{
    +    throw new WebDriverException("Cannot locate RDF:Description node in extension manifest");
    +}
    +XmlAttribute? attribute = descriptionNode.Attributes?["id", EmNamespaceUri];
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important null validation for XML parsing, though the code already has some null safety through the nullable reference types and null-conditional operators.

    7

    @@ -48,6 +48,7 @@ public class FirefoxExtension
    /// <remarks>WebDriver attempts to resolve the <paramref name="fileName"/> parameter
    /// by looking first for the specified file in the directory of the calling assembly,
    /// then using the full path to the file, if a full path is provided.</remarks>
    /// <exception cref="ArgumentNullException">If <paramref name="fileName"/> is <see langword="null"/>.</exception>
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    This is already unreachable with null using normal means, but this constructor is public-facing so we need the validation.

    @nvborisenko nvborisenko removed their assignment Dec 28, 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