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 interactions #15152

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 24, 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.

Description

Annotates nullability on interactions. The last type not annotated is the big Actions one.

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

  • Annotated nullability across multiple files in the dotnet project.

  • Improved code safety with ArgumentNullException checks.

  • Refactored properties to use expression-bodied members.

  • Marked private fields as readonly where applicable.


Changes walkthrough 📝

Relevant files
Enhancement
ElementCoordinates.cs
Annotate nullability and refactor `ElementCoordinates`.   

dotnet/src/webdriver/ElementCoordinates.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for constructor parameter.
  • Refactored properties to use expression-bodied members.
  • Marked element field as readonly.
  • +9/-22   
    ActionSequence.cs
    Annotate nullability and refactor `ActionSequence`.           

    dotnet/src/webdriver/Interactions/ActionSequence.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for constructor parameter.
  • Refactored properties to use expression-bodied members.
  • Marked interactions field as readonly.
  • +6/-12   
    IAction.cs
    Enable nullable reference types in `IAction`.                       

    dotnet/src/webdriver/Interactions/IAction.cs

    • Enabled nullable reference types.
    +2/-0     
    ICoordinates.cs
    Enable nullable reference types in `ICoordinates`.             

    dotnet/src/webdriver/Interactions/ICoordinates.cs

    • Enabled nullable reference types.
    +2/-0     
    InputDeviceKind.cs
    Enable nullable reference types in `InputDeviceKind`.       

    dotnet/src/webdriver/Interactions/InputDeviceKind.cs

    • Enabled nullable reference types.
    +2/-0     
    Interaction.cs
    Annotate nullability and refactor `Interaction`.                 

    dotnet/src/webdriver/Interactions/Interaction.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for constructor parameter.
  • Refactored properties to use expression-bodied members.
  • Removed redundant private field sourceDevice.
  • +5/-13   
    PauseInteraction.cs
    Annotate nullability and refactor `PauseInteraction`.       

    dotnet/src/webdriver/Interactions/PauseInteraction.cs

  • Enabled nullable reference types.
  • Marked duration field as readonly.
  • +3/-1     
    WebElement.cs
    Annotate nullability and improve safety in `WebElement`. 

    dotnet/src/webdriver/WebElement.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for constructor parameter.
  • +2/-1     

    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

    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 Check

    The constructor checks for null id but not for null parentDriver parameter which could lead to NullReferenceException

    public WebElement(WebDriver parentDriver, string id)
    {
        this.driver = parentDriver;
        this.elementId = id ?? throw new ArgumentNullException(nameof(id));
    }
    Type Safety

    Direct cast to IWebDriverObjectReference could throw InvalidCastException if element doesn't implement the interface

    return ((IWebDriverObjectReference)this.element).ObjectReferenceId;

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle potential invalid type cast

    The cast to IWebDriverObjectReference could throw InvalidCastException if element is
    not of the expected type. Add a type check and handle the case when the cast fails.

    dotnet/src/webdriver/ElementCoordinates.cs [70]

    -return ((IWebDriverObjectReference)this.element).ObjectReferenceId;
    +if (this.element is IWebDriverObjectReference reference)
    +    return reference.ObjectReferenceId;
    +throw new InvalidOperationException("Element does not implement IWebDriverObjectReference");
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical runtime safety issue by properly handling potential InvalidCastException, which could crash the application. The improved code provides better error handling and clearer error messages.

    9
    Validate constructor parameter for null

    The constructor should validate that parentDriver is not null to prevent
    NullReferenceException in subsequent operations.

    dotnet/src/webdriver/WebElement.cs [52-56]

     public WebElement(WebDriver parentDriver, string id)
     {
    -    this.driver = parentDriver;
    +    this.driver = parentDriver ?? throw new ArgumentNullException(nameof(parentDriver));
         this.elementId = id ?? throw new ArgumentNullException(nameof(id));
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion prevents potential NullReferenceException by validating a critical constructor parameter. This is an important defensive programming practice that improves code reliability.

    8

    // Note that the OSS dialect of the wire protocol for the Actions API
    // uses the raw ID of the element, not an element reference. To use this,
    // extract the ID using the well-known key to the dictionary for element
    // references.
    return elementReference.ObjectReferenceId;
    return ((IWebDriverObjectReference)this.element).ObjectReferenceId;
    Copy link
    Contributor Author

    @RenderMichael RenderMichael Jan 24, 2025

    Choose a reason for hiding this comment

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

    element is WebElement, which implements IWebDriverObjectReference directly.

    None of the implementations of ObjectReferenceId can return null (WebElement and ShadowRoot).

    public WebElement(WebDriver parentDriver, string id)
    {
    this.driver = parentDriver;
    this.elementId = id;
    this.elementId = id ?? throw new ArgumentNullException(nameof(id));
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    id == null is unreachable here outside of users directly creating one. Adding this check here to enforce IWebDriverObjectReference.ObjectReferenceId never returning null.

    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