Skip to content

[dotnet] Annotate nullability on Navigate() and SwitchTo() - #15211

Merged
RenderMichael merged 1 commit into
SeleniumHQ:trunkfrom
RenderMichael:nullable-navigate
Feb 1, 2025
Merged

[dotnet] Annotate nullability on Navigate() and SwitchTo()#15211
RenderMichael merged 1 commit into
SeleniumHQ:trunkfrom
RenderMichael:nullable-navigate

Conversation

@RenderMichael

@RenderMichael RenderMichael commented Feb 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Navigate() and SwitchTo()

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 in multiple files for better null safety.

  • Added ArgumentNullException checks for method parameters to improve error handling.

  • Updated Navigator and TargetLocator classes to use readonly fields and sealed classes.

  • Improved exception messages and type safety in Frame and Window methods.

Changes walkthrough 📝

Relevant files
Enhancement
INavigation.cs
Add nullability annotations and exceptions in `INavigation`

dotnet/src/webdriver/INavigation.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for GoToUrl and GoToUrlAsync methods.
  • Improved documentation with exception details.
    		• +6/-0     
    ITargetLocator.cs
    Add nullability annotations and exceptions in `ITargetLocator`

    dotnet/src/webdriver/ITargetLocator.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for Frame and Window methods.
  • Improved exception handling with detailed messages.
    		• +8/-0     
    Navigator.cs
    Enhance null safety and immutability in `Navigator`           

    dotnet/src/webdriver/Navigator.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for constructor and GoToUrl methods.
  • Updated driver field to readonly and class to sealed.
    		• +10/-3   
    TargetLocator.cs
    Enhance null safety and immutability in `TargetLocator`   

    dotnet/src/webdriver/TargetLocator.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for constructor and Frame/Window methods.
  • Improved exception messages and type safety in Frame method.
  • Updated driver field to readonly and class to sealed.
    		• +25/-11 
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review

    qodo-code-review Bot commented Feb 1, 2025

    Copy link
    Copy Markdown
    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

    Potential Race Condition

    The GoToUrl method uses Task.Run to execute asynchronous code synchronously, which could lead to race conditions or deadlocks. Consider using async/await pattern consistently.

    Task.Run(async delegate
{
    Null Check Ordering

    The Frame method checks elementReference for null after trying to cast it, which could be reordered for better efficiency and clarity.

    IWebDriverObjectReference? elementReference = frameElement as IWebDriverObjectReference;
if (elementReference == null)

    @qodo-code-review

    qodo-code-review Bot commented Feb 1, 2025

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix unsafe async operation pattern

    The GoToUrl(string url) method uses Task.Run without proper exception handling,
    which could swallow exceptions. Implement proper async/await pattern.

    dotnet/src/webdriver/Navigator.cs [90-93]

     public void GoToUrl(string url)
 {
-    Task.Run(async delegate
-    {
+    if (url == null)
+        throw new ArgumentNullException(nameof(url));
+    this.GoToUrlAsync(url).GetAwaiter().GetResult();
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion fixes a significant issue with async/await pattern implementation that could hide exceptions and cause unexpected behavior. The proposed solution properly handles synchronization and exception propagation.

    		9
    Add null checks for response handling

    The NewWindow method should handle potential null values in the response dictionary
    to prevent runtime exceptions. Add null checks and proper type casting.

    dotnet/src/webdriver/TargetLocator.cs [200-201]

    -Dictionary<string, object> result = (Dictionary<string, object>)response.Value!;
-string newWindowHandle = result["handle"].ToString()!;
+var result = response.Value as Dictionary<string, object> ?? throw new WebDriverException("Invalid response format");
+var handle = result["handle"] ?? throw new WebDriverException("Window handle not found in response");
+string newWindowHandle = handle.ToString() ?? throw new WebDriverException("Invalid window handle");
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses potential runtime exceptions by adding proper null checks and error handling for the response dictionary, which is critical for application stability and error reporting.

    		8
    Learned
    best practice
    Add null validation check at the start of method to prevent potential NullReferenceException

    The GoToUrl(string url) method should validate the url parameter for null at the
    start of the method, similar to how it's done in GoToUrlAsync. Add an
    ArgumentNullException check to maintain consistency and prevent potential
    NullReferenceException.

    dotnet/src/webdriver/Navigator.cs [90-93]

     public void GoToUrl(string url)
 {
+    if (url == null)
+    {
+        throw new ArgumentNullException(nameof(url));
+    }
     Task.Run(async delegate
     {
         await GoToUrlAsync(url).ConfigureAwait(false);
     }).GetAwaiter().GetResult();
 }
    • Apply this suggestion
    		6

    @RenderMichael
    RenderMichael merged commit 42d7dee into SeleniumHQ:trunk Feb 1, 2025
    nvborisenko
    nvborisenko reviewed Feb 1, 2025
    View reviewed changes
    Comment thread dotnet/src/webdriver/INavigation.cs
    /// should the underlying page change while your test is executing the results of
    /// future calls against this interface will be against the freshly loaded page.
    /// </remarks>
    /// <exception cref="ArgumentNullException">If <paramref name="url"/> is null.</exception>

    @nvborisenko nvborisenko Feb 1, 2025

    Copy link
    Copy Markdown
    Member

    Choose a reason for hiding this comment

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

    langword null?

    @RenderMichael RenderMichael Feb 1, 2025

    Copy link
    Copy Markdown
    Contributor Author

    Choose a reason for hiding this comment

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

    Will fix in a quick follow-up

    @RenderMichael
    RenderMichael deleted the nullable-navigate branch February 1, 2025 19:04
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @RenderMichael @sandeepsuryaprasad
    [dotnet] Annotate nullability on Navigate() and SwitchTo() (Selen…
    ffa8941 
    …iumHQ#15211)
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    @RenderMichael Grzegorz Ryznar
    [dotnet] Annotate nullability on Navigate() and SwitchTo() (Selen…
    0f762c9 
    …iumHQ#15211)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @nvborisenko nvborisenko nvborisenko left review comments

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @RenderMichael @nvborisenko