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 platform-specific WebDrivers #15236

Merged

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 5, 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 the various implementations of WebDriver that we have.

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, Documentation


Description

  • Annotated nullability for platform-specific WebDriver implementations.

  • Added exception handling for null arguments in constructors and methods.

  • Updated XML documentation to include nullability-related exceptions.

  • Refactored code for improved readability and consistency.


Changes walkthrough 📝

Relevant files
Enhancement
ChromeDriver.cs
Add nullability annotations and improve ChromeDriver constructors

dotnet/src/webdriver/Chrome/ChromeDriver.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for null parameters in constructors.
  • Made chromeCustomCommands readonly for immutability.
  • +9/-1     
    ChromiumDriver.cs
    Enhance nullability and error handling in ChromiumDriver 

    dotnet/src/webdriver/Chromium/ChromiumDriver.cs

  • Enabled nullable reference types.
  • Added null checks and exceptions for method parameters.
  • Updated devToolsSession to nullable and added related annotations.
  • Refactored property and method implementations for clarity.
  • +48/-30 
    ChromiumNetworkConditions.cs
    Handle nullable dictionary values in ChromiumNetworkConditions

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs

  • Updated method to handle nullable dictionary values.
  • Added null-forgiving operator for dictionary value assignments.
  • +4/-4     
    EdgeDriver.cs
    Add nullability annotations and improve EdgeDriver constructors

    dotnet/src/webdriver/Edge/EdgeDriver.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for null parameters in constructors.
  • Made edgeCustomCommands readonly for immutability.
  • +9/-1     
    FirefoxDriver.cs
    Enhance nullability and error handling in FirefoxDriver   

    dotnet/src/webdriver/Firefox/FirefoxDriver.cs

  • Enabled nullable reference types.
  • Added null checks and exceptions for method parameters.
  • Updated XML documentation for nullability-related exceptions.
  • Refactored methods for improved error handling and clarity.
  • +48/-24 
    InternetExplorerDriver.cs
    Add nullability annotations and improve InternetExplorerDriver
    constructors

    dotnet/src/webdriver/IE/InternetExplorerDriver.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for null parameters in constructors.
  • Updated XML documentation to reflect nullability-related exceptions.
  • +26/-8   
    SafariDriver.cs
    Enhance nullability and error handling in SafariDriver     

    dotnet/src/webdriver/Safari/SafariDriver.cs

  • Enabled nullable reference types.
  • Added null checks and exceptions for method parameters.
  • Updated XML documentation for nullability-related exceptions.
  • Refactored methods for improved error handling and clarity.
  • +25/-6   

    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 5, 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

    Error Handling

    The NetworkConditions property getter assumes response.Value is a Dictionary<string,object?> without proper type checking, which could lead to runtime errors if the response has unexpected format.

    get
    {
        Response response = this.Execute(GetNetworkConditionsCommand, null);
        if (response.Value is not Dictionary<string, object?> responseDictionary)
        {
            throw new WebDriverException($"GetNetworkConditions command returned successfully, but data was not an object: {response.Value}");
        }
    
        return ChromiumNetworkConditions.FromDictionary(responseDictionary);
    }
    Null Safety

    The GetFullPageScreenshot method assumes screenshotResponse.Value is non-null without proper validation, which could cause NullReferenceException if the response is malformed.

    /// Gets a <see cref="Screenshot"/> object representing the image of the full page on the screen.
    /// </summary>
    /// <returns>A <see cref="Screenshot"/> object containing the image.</returns>
    public Screenshot GetFullPageScreenshot()
    {
        Response screenshotResponse = this.Execute(GetFullPageScreenshotCommand, null);
    
        string base64 = screenshotResponse.Value?.ToString() ?? throw new WebDriverException("GetFullPageScreenshotCommand returned successfully but contained no data");
        return new Screenshot(base64);
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add safe null value handling

    Add null checks before casting dictionary values to avoid potential
    NullReferenceException. The current use of null-forgiving operator (!) is
    unsafe.

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs [99-114]

    -conditions.IsOffline = (bool)offline!;
    -conditions.UploadThroughput = (long)uploadThroughput!;
    -conditions.DownloadThroughput = (long)downloadThroughput!;
    +conditions.IsOffline = offline is bool value ? value : false;
    +conditions.UploadThroughput = uploadThroughput is long value ? value : 0;
    +conditions.DownloadThroughput = downloadThroughput is long value ? value : 0;
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a critical safety issue by replacing unsafe null-forgiving operators with proper null checks and default values, preventing potential runtime NullReferenceExceptions.

    High
    Add null check for response value
    Suggestion Impact:The commit addresses null checking of response values in multiple places, using different approaches like EnsureValueIsNotNull() and null-forgiving operator, showing awareness of the null reference issue raised in the suggestion

    code diff:

    -            if (response.Value is null)
    -            {
    -                throw new WebDriverException("Firefox add-on installation returned no add-on identifier");
    -            }
    -
    -            return (string)response.Value;
    +
    +            return (string)response.Value!;
             }
     
             /// <summary>
    @@ -395,7 +391,8 @@
             {
                 Response screenshotResponse = this.Execute(GetFullPageScreenshotCommand, null);
     
    -            string base64 = screenshotResponse.Value?.ToString() ?? throw new WebDriverException("GetFullPageScreenshotCommand returned successfully but contained no data");
    +            screenshotResponse.EnsureValueIsNotNull();
    +            string base64 = screenshotResponse.Value.ToString()!;

    Add null check for response.Value in GetContext() method before attempting
    ToString() conversion to avoid potential NullReferenceException.

    dotnet/src/webdriver/Firefox/FirefoxDriver.cs [263-266]

     Response commandResponse = this.Execute(GetContextCommand, null);
     
    -if (commandResponse.Value is not string response
    +if (commandResponse.Value is null
    +    || commandResponse.Value is not string response
         || !Enum.TryParse(response, ignoreCase: true, out FirefoxCommandContext output))
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds an important null check for commandResponse.Value before attempting to use it, which prevents potential NullReferenceException. This is a critical defensive programming practice when dealing with response values.

    Medium
    Validate types before casting

    Add type validation before casting to ensure the dictionary values are of the
    expected types, preventing InvalidCastException.

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs [97-100]

    -if (dictionary.TryGetValue("offline", out object? offline))
    +if (dictionary.TryGetValue("offline", out object? offline) && offline is bool)
     {
    -    conditions.IsOffline = (bool)offline!;
    +    conditions.IsOffline = (bool)offline;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds important type validation before casting, preventing potential InvalidCastException. This is a significant safety improvement for handling dictionary values.

    Medium

    @RenderMichael
    Copy link
    Contributor Author

    Test failures are unrelated to this PR

    //java/test/org/openqa/selenium/mobile:NetworkConnectionTest
    //java/test/org/openqa/selenium/remote:RemoteWebDriverScreenshotTest
    //java/test/org/openqa/selenium/remote:RemoteWebDriverScreenshotTest-firefox-beta
    //rb/spec/integration/selenium/webdriver:network-firefox-beta-bidi
    //rb/spec/integration/selenium/webdriver:network-firefox-bidi

    @RenderMichael RenderMichael merged commit e639050 into SeleniumHQ:trunk Feb 9, 2025
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the nullable-webdriver-impls branch February 9, 2025 07:14
    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