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 Actions type #15208

Merged
merged 2 commits into from
Feb 1, 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 Actions type

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 across the Actions class and related methods.

  • Added null checks and exception handling for method arguments.

  • Improved code clarity with nullable reference types and annotations.

  • Enhanced documentation with detailed exception summaries.


Changes walkthrough 📝

Relevant files
Enhancement
Actions.cs
Nullability annotations and argument validation in `Actions.cs`

dotnet/src/webdriver/Interactions/Actions.cs

  • Enabled nullable reference types with #nullable enable.
  • Annotated fields and method parameters with nullable types.
  • Added null checks and ArgumentNullException for method arguments.
  • Improved exception documentation for various methods.
  • +48/-26 
    Bug fix
    InputDevice.cs
    Add exception handling for `CreatePause` duration               

    dotnet/src/webdriver/Interactions/InputDevice.cs

  • Added exception handling for negative duration in CreatePause.
  • Updated documentation to reflect new exception.
  • +1/-0     
    PauseInteraction.cs
    Validate duration in `PauseInteraction` constructor           

    dotnet/src/webdriver/Interactions/PauseInteraction.cs

  • Added validation for negative duration in constructor.
  • Updated documentation to include exception details.
  • +1/-0     

    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 Dereference

    The GetDriverAs<T> method could return null but is used directly without null check in the Actions constructor, which could lead to a NullReferenceException

    IActionExecutor actionExecutor = GetDriverAs<IActionExecutor>(driver)
        ?? throw new ArgumentException("The IWebDriver object must implement or wrap a driver that implements IActionExecutor.", nameof(driver));
    Potential Bug

    The GetLocatableFromElement method could return null for the target variable even after the element null check, which could lead to unexpected behavior

    ILocatable? target = null;
    IWrapsElement? wrapper = element as IWrapsElement;
    while (wrapper != null)
    {
        target = wrapper.WrappedElement as ILocatable;
        wrapper = wrapper.WrappedElement as IWrapsElement;

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate nullable element before use

    Add null check for scrollOrigin.Element before using it with the '!' operator to
    prevent potential runtime errors.

    dotnet/src/webdriver/Interactions/Actions.cs [577-578]

    -this.actionBuilder.AddAction(this.GetActiveWheel().CreateWheelScroll(scrollOrigin.Element!,
    +if (scrollOrigin.Element == null)
    +    throw new ArgumentException("Element must not be null when viewport is false.", nameof(scrollOrigin));
    +this.actionBuilder.AddAction(this.GetActiveWheel().CreateWheelScroll(scrollOrigin.Element,
         scrollOrigin.XOffset, scrollOrigin.YOffset, deltaX, deltaY, duration));
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical issue where using the null-forgiving operator (!) without validation could cause runtime errors. Adding explicit validation improves code safety and provides better error messaging.

    9
    Add missing null check

    Add null check for driver parameter in constructor to prevent potential
    NullReferenceException.

    dotnet/src/webdriver/Interactions/Actions.cs [55-59]

     public Actions(IWebDriver driver, TimeSpan duration)
     {
    +    if (driver == null)
    +        throw new ArgumentNullException(nameof(driver));
    +        
         IActionExecutor actionExecutor = GetDriverAs<IActionExecutor>(driver)
             ?? throw new ArgumentException("The IWebDriver object must implement or wrap a driver that implements IActionExecutor.", nameof(driver));
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a null check for the driver parameter is crucial to prevent NullReferenceException and provide clear error messages. This is a significant improvement for API robustness and error handling.

    8
    Learned
    best practice
    Use ArgumentNullException.ThrowIfNull() for more concise and standardized null parameter validation

    Add ArgumentNullException.ThrowIfNull() checks at the start of methods that have
    nullable parameters but require non-null values. For example, in the
    ScrollFromOrigin method, add null validation for the scrollOrigin parameter before
    using it.

    dotnet/src/webdriver/Interactions/Actions.cs [560-563]

     public Actions ScrollFromOrigin(WheelInputDevice.ScrollOrigin scrollOrigin, int deltaX, int deltaY)
     {
    -    if (scrollOrigin is null)
    -    {
    -        throw new ArgumentNullException(nameof(scrollOrigin));
    -    }
    +    ArgumentNullException.ThrowIfNull(scrollOrigin, nameof(scrollOrigin));
    • Apply this suggestion
    6

    @RenderMichael RenderMichael merged commit 5b648f3 into SeleniumHQ:trunk Feb 1, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the actions branch February 1, 2025 06:49
    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