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 Manage() #15210

Merged
merged 3 commits into from
Feb 3, 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.

Add nullability to Manage()

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

  • Enabled nullable reference types across multiple files for improved null safety.

  • Refactored properties to use expression-bodied members for conciseness.

  • Added null checks and updated constructors to throw ArgumentNullException where necessary.

  • Improved type safety by updating dictionary value types and handling potential null values.


Changes walkthrough 📝

Relevant files
Enhancement
IOptions.cs
Enable nullable reference types in `IOptions`                       

dotnet/src/webdriver/IOptions.cs

  • Enabled nullable reference types.
  • Added #nullable enable directive for null safety.
  • +2/-0     
    ITimeouts.cs
    Enable nullable reference types in `ITimeouts`                     

    dotnet/src/webdriver/ITimeouts.cs

  • Enabled nullable reference types.
  • Added #nullable enable directive for null safety.
  • +2/-0     
    IWindow.cs
    Enable nullable reference types in `IWindow`                         

    dotnet/src/webdriver/IWindow.cs

  • Enabled nullable reference types.
  • Added #nullable enable directive for null safety.
  • +2/-0     
    OptionsManager.cs
    Refactor `OptionsManager` and enable nullable reference types

    dotnet/src/webdriver/OptionsManager.cs

  • Enabled nullable reference types.
  • Refactored properties to use expression-bodied members.
  • Added null checks in the constructor.
  • Changed OptionsManager to a sealed class.
  • +9/-19   
    Timeouts.cs
    Refactor `Timeouts` and enable nullable reference types   

    dotnet/src/webdriver/Timeouts.cs

  • Enabled nullable reference types.
  • Updated dictionary value types to handle potential nulls.
  • Refactored properties to use expression-bodied members.
  • Added null checks in the constructor.
  • +18/-23 
    Window.cs
    Refactor `Window` and enable nullable reference types       

    dotnet/src/webdriver/Window.cs

  • Enabled nullable reference types.
  • Refactored properties to use expression-bodied members.
  • Updated dictionary value types to handle potential nulls.
  • Changed Window to a sealed class.
  • +14/-14 

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

    Null Safety

    The cast of commandResponse.Value to Dictionary<string,object?> assumes the Value is non-null and of correct type. This could throw InvalidCastException if Value is null or wrong type.

    Dictionary<string, object?> responseValue = (Dictionary<string, object?>)commandResponse.Value!;
    Null Safety

    Similar to Timeouts.cs, the cast of commandResponse.Value to Dictionary<string,object?> assumes non-null Value. Should add null check before casting.

    Dictionary<string, object?> rawPosition = (Dictionary<string, object?>)commandResponse.Value!;

    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 response value

    Add null check for commandResponse.Value to prevent potential NullReferenceException
    in case the response value is null.

    dotnet/src/webdriver/Timeouts.cs [116]

    -Dictionary<string, object?> responseValue = (Dictionary<string, object?>)commandResponse.Value!;
    +if (commandResponse.Value == null)
    +    throw new WebDriverException("Response value was null");
    +Dictionary<string, object?> responseValue = (Dictionary<string, object?>)commandResponse.Value;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential null reference vulnerability by adding a proper null check before accessing the response value, which could prevent runtime crashes in edge cases.

    8
    Validate dictionary values before conversion

    Add null check for rawPosition dictionary values before conversion to prevent
    potential NullReferenceException.

    dotnet/src/webdriver/Window.cs [55-57]

     Dictionary<string, object?> rawPosition = (Dictionary<string, object?>)commandResponse.Value!;
    +if (rawPosition["x"] == null || rawPosition["y"] == null)
    +    throw new WebDriverException("Position coordinates were null");
     int x = Convert.ToInt32(rawPosition["x"], CultureInfo.InvariantCulture);
     int y = Convert.ToInt32(rawPosition["y"], CultureInfo.InvariantCulture);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important null validation for dictionary values before conversion to integers, preventing potential NullReferenceException that could occur if the browser returns unexpected data.

    8

    @RenderMichael RenderMichael merged commit 98b22f8 into SeleniumHQ:trunk Feb 3, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the nullable-options branch February 3, 2025 07:18
    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