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 DevToolsSession #15244

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 6, 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.

Annotates nullability on DevToolsSession and modifies nullability on related 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 DevToolsSession and related classes.

  • Fixed typos and improved comments/documentation for clarity.

  • Enhanced error handling and logging in DevToolsSession.

  • Refactored code to ensure null safety and modernized syntax.


Changes walkthrough 📝

Relevant files
Documentation
DevToolsCommandData.cs
Fix typos and improve constructor documentation                   

dotnet/src/webdriver/DevTools/DevToolsCommandData.cs

  • Fixed typos in parameter documentation.
  • Improved exception documentation for constructors.
  • +2/-2     
    Enhancement
    DevToolsSession.cs
    Annotate nullability and improve error handling                   

    dotnet/src/webdriver/DevTools/DevToolsSession.cs

  • Enabled nullable reference types.
  • Annotated fields, properties, and methods for nullability.
  • Fixed typos and improved logging messages.
  • Enhanced error handling and refactored methods for null safety.
  • +73/-77 
    DevToolsSessionLogMessageEventArgs.cs
    Support nullable arguments in constructor                               

    dotnet/src/webdriver/DevTools/DevToolsSessionLogMessageEventArgs.cs

    • Updated constructor to support nullable arguments.
    +1/-1     
    DevToolsVersionInfo.cs
    Simplify regex and improve error handling                               

    dotnet/src/webdriver/DevTools/DevToolsVersionInfo.cs

  • Simplified regex usage for version parsing.
  • Improved error handling for invalid version strings.
  • +2/-4     
    IDevToolsSession.cs
    Annotate nullability and fix documentation typos                 

    dotnet/src/webdriver/DevTools/IDevToolsSession.cs

  • Enabled nullable reference types.
  • Annotated methods and events for nullability.
  • Fixed typos in documentation.
  • +8/-6     

    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 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Safety

    The Domains property is marked as non-null but initialized as null with a TODO comment. This could lead to NullReferenceException if accessed before initialization.

    public DevToolsDomains Domains { get; private set; } = null!; // TODO handle nullability for this
    Error Handling

    The ProcessMessage method assumes messageObject.GetProperty("method") will return a non-null string without proper validation, which could cause runtime errors.

    var method = methodProperty.GetString() ?? throw new InvalidOperationException("CDP message contained \"method\" property with a value of \"null\".");

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent potential shutdown deadlock

    The ShutdownMessageQueue method has an infinite loop condition that depends on a
    connection that was already closed. This could lead to a deadlock. Consider
    adding a timeout or using a more reliable shutdown condition.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [515-518]

    -while (connection.IsActive)
    +using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
    +while (connection.IsActive && !cts.Token.IsCancellationRequested)
     {
    -    await Task.Delay(TimeSpan.FromMilliseconds(10));
    +    await Task.Delay(TimeSpan.FromMilliseconds(10), cts.Token);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion fixes a critical issue where the shutdown process could hang indefinitely. Adding a timeout with proper cancellation support ensures the application can gracefully terminate.

    High
    Fix potential null reference issue

    The Domains property is initialized with null! and marked with a TODO comment.
    This could lead to NullReferenceException if accessed before initialization.
    Consider initializing it in the constructor or making it truly nullable.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [120]

    -public DevToolsDomains Domains { get; private set; } = null!; // TODO handle nullability for this
    +public DevToolsDomains? Domains { get; private set; }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a critical issue where using null! with a TODO comment could lead to runtime NullReferenceExceptions. Making the property nullable with proper initialization would prevent potential crashes.

    Medium

    @RenderMichael
    Copy link
    Contributor Author

    Test failures are unrelated to this PR

    //java/test/org/openqa/selenium/mobile:NetworkConnectionTest
    //java/test/org/openqa/selenium/remote:RemoteWebDriverScreenshotTest
    //java/test/org/openqa/selenium/remote:RemoteWebDriverScreenshotTest-firefox-beta
    //rb/spec/integration/selenium/webdriver:network-firefox-beta-bidi
    //rb/spec/integration/selenium/webdriver:network-firefox-bidi

    @RenderMichael RenderMichael merged commit 0721800 into SeleniumHQ:trunk Feb 6, 2025
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the devtools-session branch February 6, 2025 15:10
    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