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] Improve bidi exception when it is not enabled #15163

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 26, 2025

PR Type

Bug fix


Description

  • Replaced generic exception with a specific BiDiException.

  • Improved error message for unsupported bidirectional protocol.


Changes walkthrough 📝

Relevant files
Bug fix
WebDriver.Extensions.cs
Refined exception handling and error message                         

dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs

  • Replaced System.Exception with BiDiException.
  • Updated error message for better clarity.
  • +1/-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: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Reference

    The code uses null-forgiving operator (!) when calling ToString() on webSocketUrl, but there's no guarantee webSocketUrl is not null at that point. Consider adding a null check before calling ToString().

    var bidi = await BiDi.ConnectAsync(webSocketUrl.ToString()!).ConfigureAwait(false);

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 26, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add null parameter validation
    Suggestion Impact:The commit implements null validation for webDriver parameter using a different syntax than suggested

    code diff:

    +        if (webDriver is null) throw new ArgumentNullException(nameof(webDriver));

    Add null check for webDriver parameter to prevent NullReferenceException when
    calling the extension method with null

    dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs [29-31]

     public static async Task<BiDi> AsBiDiAsync(this IWebDriver webDriver)
     {
    +    ArgumentNullException.ThrowIfNull(webDriver);
         var webSocketUrl = ((IHasCapabilities)webDriver).Capabilities.GetCapability("webSocketUrl");
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding null parameter validation is crucial for preventing NullReferenceException and providing better error handling. This is a significant defensive programming practice that improves code robustness and provides clearer error messages.

    8
    Learned
    best practice
    ✅ Add proper validation for nullable values before using them to prevent potential runtime errors
    Suggestion Impact:The commit improves null handling by adding null checks for webDriver and properly handling webSocketUrl as a string, removing the null-forgiving operator

    code diff:

    +        if (webDriver is null) throw new ArgumentNullException(nameof(webDriver));
    +
    +        string? webSocketUrl = null;
    +
    +        if (webDriver is IHasCapabilities hasCapabilities)
    +        {
    +            webSocketUrl = hasCapabilities.Capabilities.GetCapability("webSocketUrl")?.ToString();
    +        }
     
             if (webSocketUrl is null) throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
     
    -        var bidi = await BiDi.ConnectAsync(webSocketUrl.ToString()!).ConfigureAwait(false);
    +        var bidi = await BiDi.ConnectAsync(webSocketUrl).ConfigureAwait(false);

    The code uses null-forgiving operator (!) on webSocketUrl.ToString() which could
    lead to runtime errors if webSocketUrl is null. Instead, validate the value before
    using it and provide a clear error message.

    dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs [33-35]

    -if (webSocketUrl is null) throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
    -var bidi = await BiDi.ConnectAsync(webSocketUrl.ToString()!).ConfigureAwait(false);
    +if (webSocketUrl is null)
    +    throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
     
    +var webSocketUrlStr = webSocketUrl.ToString();
    +if (string.IsNullOrEmpty(webSocketUrlStr))
    +    throw new BiDiException("Invalid empty webSocketUrl value");
    +    
    +var bidi = await BiDi.ConnectAsync(webSocketUrlStr).ConfigureAwait(false);
    +
    • Apply this suggestion
    6

    Sorry, something went wrong.

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Better errors are always helpful.

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
    @nvborisenko nvborisenko merged commit ffe8d4b into SeleniumHQ:trunk Jan 27, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-bidi-exception branch January 27, 2025 10:45
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    None yet

    2 participants