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 most remaining types #15257

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

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 9, 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 most remaining types

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 most remaining types across the project.

  • Improved code consistency by replacing legacy patterns with modern C# features.

  • Added null checks and exception handling for better error management.

  • Refactored properties and methods to use concise syntax and readonly fields.


Changes walkthrough 📝

Relevant files
Enhancement
19 files
By.cs
Annotated nullability and refactored `By` class for clarity
+89/-82 
Cookie.cs
Added nullability annotations and refactored `Cookie` class
+50/-69 
DevToolsSession.cs
Added nullability annotations to `DevToolsSession`             
+1/-1     
DomMutationData.cs
Added nullability annotations to `DomMutationData`             
+1/-1     
InternetExplorerDriverLogLevel.cs
Enabled nullability in `InternetExplorerDriverLogLevel`   
+2/-0     
InternetExplorerOptions.cs
Refactored InternetExplorerOptions with nullability and readonly
fields
+46/-135
INetwork.cs
Added nullability annotations to `INetwork` interface       
+4/-2     
ISupportsLogs.cs
Enabled nullability in `ISupportsLogs`                                     
+2/-0     
IWrapsElement.cs
Enabled nullability in `IWrapsElement`                                     
+2/-0     
PortUtilities.cs
Added nullability annotations to `PortUtilities`                 
+3/-1     
ReturnedCookie.cs
Added nullability annotations to `ReturnedCookie`               
+4/-2     
LogType.cs
Enabled nullability in `LogType`                                                 
+2/-0     
NetworkAuthenticationHandler.cs
Added nullability annotations to `NetworkAuthenticationHandler`
+4/-2     
NetworkManager.cs
Added nullability annotations and refactored `NetworkManager`
+17/-11 
NetworkRequestHandler.cs
Added nullability annotations to `NetworkRequestHandler` 
+5/-3     
NetworkResponseHandler.cs
Added nullability annotations to `NetworkResponseHandler`
+4/-2     
RelativeBy.cs
Added nullability annotations and improved error handling in
RelativeBy
+28/-19 
DesiredCapabilities.cs
Refactored `DesiredCapabilities` with nullability and concise syntax
+27/-74 
RemoteWebDriver.cs
Added nullability annotations and improved error handling in
RemoteWebDriver
+32/-20 

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 9, 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 Safety

    The NetworkManager class has nullable event handlers but does not check for null before invoking them in the event raising code. This could lead to runtime NullReferenceExceptions.

    public event EventHandler<NetworkRequestSentEventArgs>? NetworkRequestSent;
    
    /// <summary>
    /// Occurs when a browser receives a network response.
    /// </summary>
    public event EventHandler<NetworkResponseReceivedEventArgs>? NetworkResponseReceived;
    Required Properties

    ResponseMatcher and ResponseTransformer are marked as required (non-null) but there is no validation in the constructor to ensure they are initialized. This could lead to NullReferenceExceptions if not set.

    public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = null!;
    
    /// <summary>
    /// Gets or sets a function that accepts an <see cref="HttpResponseData"/> object describing a network
    /// response received by the browser, and returns a modified <see cref="HttpResponseData"/> object to used
    /// as the actual network response.
    /// </summary>
    public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = null!;

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null checks for nullable delegates

    The FindElementMethod and FindElementsMethod properties are marked as nullable
    but used without null checks in FindElement and FindElements methods. Add null
    checks to prevent potential NullReferenceException.

    dotnet/src/webdriver/By.cs [332-333]

     public virtual IWebElement FindElement(ISearchContext context)
     {
    +    if (this.FindElementMethod == null)
    +    {
    +        throw new InvalidOperationException("FindElementMethod not set");
    +    }
         return this.FindElementMethod(context);
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a critical potential NullReferenceException by adding necessary null checks for nullable delegate properties that are used directly. This is especially important since the code has #nullable enable.

    High
    Add defensive null checking

    Add null check for the namesArray value before accessing it to prevent potential
    NullReferenceException if the response is malformed.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [527-528]

    -object[] namesArray = (object[])value["names"]!;
    -return namesArray.Select(obj => obj.ToString()!).ToList();
    +object[]? namesArray = value["names"] as object[];
    +if (namesArray == null)
    +{
    +    throw new WebDriverException("GetDownloadableFiles response did not contain valid 'names' array");
    +}
    +return namesArray.Select(obj => obj?.ToString() ?? string.Empty).ToList();
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds important null checks to prevent potential runtime exceptions when handling malformed server responses, which could improve application stability and provide better error messages.

    Medium
    Avoid null reference runtime exceptions

    Initialize RequestMatcher in the constructor instead of using null! to avoid
    potential null reference exceptions at runtime.

    dotnet/src/webdriver/NetworkRequestHandler.cs [35]

    -public Func<HttpRequestData, bool> RequestMatcher { get; set; } = null!;
    +public Func<HttpRequestData, bool> RequestMatcher { get; set; } = _ => false;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion provides a safer default implementation that prevents null reference exceptions at runtime, which is a critical improvement over using null! that could lead to runtime crashes.

    Medium
    Provide safe default function implementations

    Initialize ResponseMatcher and ResponseTransformer in the constructor with safe
    default implementations instead of using null! to avoid potential null reference
    exceptions.

    dotnet/src/webdriver/NetworkResponseHandler.cs [35-42]

    -public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = null!;
    -public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = null!;
    +public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = _ => false;
    +public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = response => response;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion replaces potentially dangerous null! assignments with safe default implementations for both ResponseMatcher and ResponseTransformer, preventing runtime exceptions and providing sensible default behavior.

    Medium
    Add safe default function implementation

    Initialize UriMatcher with a safe default implementation instead of using null!
    to avoid potential null reference exceptions.

    dotnet/src/webdriver/NetworkAuthenticationHandler.cs [35]

    -public Func<Uri, bool> UriMatcher { get; set; } = null!;
    +public Func<Uri, bool> UriMatcher { get; set; } = _ => false;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion replaces the null! assignment with a safe default implementation for UriMatcher, preventing potential null reference exceptions and providing a reasonable default behavior.

    Medium
    Validate transformed response not null

    Add null check for the response transformer result before using it with
    ContinueRequestWithResponse to prevent potential NullReferenceException.

    dotnet/src/webdriver/NetworkManager.cs [272]

    -await this.session.Value.Domains.Network.ContinueRequestWithResponse(requestData, handler.ResponseTransformer(e.ResponseData)).ConfigureAwait(false);
    +var transformedResponse = handler.ResponseTransformer(e.ResponseData);
    +if (transformedResponse == null)
    +{
    +    throw new WebDriverException("Response transformer returned null response");
    +}
    +await this.session.Value.Domains.Network.ContinueRequestWithResponse(requestData, transformedResponse).ConfigureAwait(false);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds a critical null check for the transformed response before using it, which could prevent NullReferenceException and provide clearer error messaging when the transformer returns invalid data.

    Medium
    Learned
    best practice
    Add null validation checks for nullable delegate properties before invoking them to prevent NullReferenceExceptions

    The FindElement and FindElements methods should validate that their
    FindElementMethod and FindElementsMethod properties are not null before using
    them, since they are marked as nullable. Add ArgumentNullException checks at the
    start of these methods.

    dotnet/src/webdriver/By.cs [330-344]

     public virtual IWebElement FindElement(ISearchContext context)
     {
    +    ArgumentNullException.ThrowIfNull(this.FindElementMethod, nameof(FindElementMethod));
         return this.FindElementMethod(context);
     }
     
    -public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context) 
    +public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context)
     {
    +    ArgumentNullException.ThrowIfNull(this.FindElementsMethod, nameof(FindElementsMethod)); 
         return this.FindElementsMethod(context);
     }
    • Apply this suggestion
    Low

    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