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] Do not warn when passing in null driver paths to driver service #15328

Merged
merged 5 commits into from
Feb 25, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Feb 24, 2025

User description

Description

This avoids warning users when passing in null for a driver service's driver directory path or filename.

Motivation and Context

Fixes #15320

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

Bug fix


Description

  • Updated driver service methods to accept nullable parameters.

  • Fixed potential null reference warnings in driver service creation.

  • Ensured compatibility with nullable reference types (NRT) in .NET.


Changes walkthrough 📝

Relevant files
Bug fix
ChromeDriverService.cs
Allow nullable parameters in ChromeDriverService methods 

dotnet/src/webdriver/Chrome/ChromeDriverService.cs

  • Updated CreateDefaultService methods to accept nullable parameters.
  • Adjusted method signatures to use string? for driverPath and
    driverExecutableFileName.
  • +2/-2     
    EdgeDriverService.cs
    Allow nullable parameters in EdgeDriverService methods     

    dotnet/src/webdriver/Edge/EdgeDriverService.cs

  • Updated CreateDefaultService methods to accept nullable parameters.
  • Adjusted method signatures to use string? for driverPath and
    driverExecutableFileName.
  • +2/-2     
    FirefoxDriverService.cs
    Allow nullable parameters in FirefoxDriverService methods

    dotnet/src/webdriver/Firefox/FirefoxDriverService.cs

  • Updated CreateDefaultService methods to accept nullable parameters.
  • Adjusted method signatures to use string? for driverPath and
    driverExecutableFileName.
  • +2/-2     
    InternetExplorerDriverService.cs
    Allow nullable parameters in InternetExplorerDriverService methods

    dotnet/src/webdriver/IE/InternetExplorerDriverService.cs

  • Updated CreateDefaultService methods to accept nullable parameters.
  • Adjusted method signatures to use string? for driverPath and
    driverExecutableFileName.
  • +2/-2     
    SafariDriverService.cs
    Allow nullable parameters in SafariDriverService methods 

    dotnet/src/webdriver/Safari/SafariDriverService.cs

  • Updated CreateDefaultService methods to accept nullable parameters.
  • Adjusted method signatures to use string? for driverPath and
    driverExecutableFileName.
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15320 - Fully compliant

    Compliant requirements:

    • Fix build errors when passing nullable parameters to CreateDefaultService APIs
    • Allow passing null values for driver path and executable name parameters
    • Update NRT annotations for Edge and Firefox drivers (and other affected drivers)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Check

    The File.Exists() method is called directly on potentially null driverPath parameter. This could throw a NullReferenceException if driverPath is null.

    if (File.Exists(driverPath))

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 24, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent null reference exception

    Add null check before calling File.Exists to prevent NullReferenceException when
    driverPath is null

    dotnet/src/webdriver/Chrome/ChromeDriverService.cs [69-70]

    -if (File.Exists(driverPath))
    +if (driverPath != null && File.Exists(driverPath))
     {
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: Critical suggestion that prevents a potential NullReferenceException when driverPath is null, which is now possible since the parameter was made nullable. This check is essential for runtime stability.

    High
    • Update

    Sorry, something went wrong.

    @nvborisenko
    Copy link
    Member

    Interesting behavior:

    if (File.Exists(driverPath))
    {
        fileName = Path.GetFileName(driverPath);
        driverPath = Path.GetDirectoryName(driverPath)!;
    }
    else
    {
        fileName = ChromiumDriverServiceFileName(DefaultChromeDriverServiceExecutableName);
    }

    I am user, I call method ChromeDriverService.CreateDefault("this is my driver path!!!!"), and the library silently ignore it. In my opinion this is bad behavior, the library should take into account everything user specified explicitly, otherwise fallback to defaults.

    @RenderMichael do you think the same?

    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko I completely agree. It would be a breaking change (I think a good one?) so maybe it should come with v5? Or can we do it for 4.30?

    @nvborisenko
    Copy link
    Member

    Good one breaking change. Existing users will not be broken (because they are already in happy branch). So it is a breaking change for users who "thinks they are in happy branch". I propose to fix in in minor selenium version.

    Thanks Mike.

    @RenderMichael
    Copy link
    Contributor Author

    That’s also a good point, in a way every bug fix is a “breaking change”. I will update this PR shortly to throw a good exception.

    Verified

    This commit was signed with the committer’s verified signature.
    davidmehren David Mehren
    @RenderMichael
    Copy link
    Contributor Author

    RenderMichael commented Feb 25, 2025

    Digging into this a little bit, I misinterpreted how this method family functioned, and there is no bug. It looks like the driverPath parameter can be two things:

    • The path directly to the chromedriver.exe/geckodriver.exe/etc. file (with a potentially different file name of course)
    • The directory containing the driver (and it assumes the driver file name is the regular one, eg. chromedriver.exe).

    I refactored the methods to clarify this functionality better.

    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

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

    Thanks @RenderMichael, looks good.

    @RenderMichael RenderMichael merged commit 4134f70 into SeleniumHQ:trunk Feb 25, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the dotnet-driver-service branch February 25, 2025 19:09
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    …ice (SeleniumHQ#15328)
    
    * [dotnet] Do not warn when passing in null driver paths to driver service
    
    * Clarify functionality of CreateDriverService
    
    * fix XML comment for `EdgeDriverService`
    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.

    [🐛 Bug]: Incorrect NRT annotations for CreateDefaultService APIs
    2 participants