From 286c139cce59e1f5327631428a0604b0d68e450c Mon Sep 17 00:00:00 2001 From: Michael Render Date: Fri, 24 Jan 2025 12:04:35 -0500 Subject: [PATCH] [dotnet] Annotate nullability for `DriverService` and chromium/safari services (#15101) * [dotnet] Annotate nullability for `DriverService` and some derived types * Default values to `null` instead of sentinel values. --- .../Chromium/ChromiumDriverService.cs | 113 ++++++----------- dotnet/src/webdriver/DriverService.cs | 120 ++++++------------ .../webdriver/Safari/SafariDriverService.cs | 12 +- 3 files changed, 85 insertions(+), 160 deletions(-) diff --git a/dotnet/src/webdriver/Chromium/ChromiumDriverService.cs b/dotnet/src/webdriver/Chromium/ChromiumDriverService.cs index 95958e44812f4..bf9779d750319 100644 --- a/dotnet/src/webdriver/Chromium/ChromiumDriverService.cs +++ b/dotnet/src/webdriver/Chromium/ChromiumDriverService.cs @@ -21,6 +21,8 @@ using System.Globalization; using System.Text; +#nullable enable + namespace OpenQA.Selenium.Chromium { /// @@ -30,15 +32,6 @@ public abstract class ChromiumDriverService : DriverService { private const string DefaultChromeDriverServiceExecutableName = "chromedriver"; - private string logPath = string.Empty; - private string urlPathPrefix = string.Empty; - private string portServerAddress = string.Empty; - private string allowedIPAddresses = string.Empty; - private int adbPort = -1; - private bool disableBuildCheck; - private bool enableVerboseLogging; - private bool enableAppendLog; - /// /// Initializes a new instance of the class. /// @@ -51,94 +44,64 @@ protected ChromiumDriverService(string executablePath, string executableFileName } /// - /// Gets or sets the location of the log file written to by the ChromeDriver executable. + /// Gets or sets the location of the log file written to by the ChromeDriver executable. + /// or signify no log path. /// - public string LogPath - { - get { return this.logPath; } - set { this.logPath = value; } - } + public string? LogPath { get; set; } /// - /// Gets or sets the base URL path prefix for commands (e.g., "wd/url"). + /// Gets or sets the base URL path prefix for commands (e.g., "wd/url"). + /// or signify no prefix. /// - public string UrlPathPrefix - { - get { return this.urlPathPrefix; } - set { this.urlPathPrefix = value; } - } + public string? UrlPathPrefix { get; set; } /// - /// Gets or sets the address of a server to contact for reserving a port. + /// Gets or sets the address of a server to contact for reserving a port. + /// or signify no port server. /// - public string PortServerAddress - { - get { return this.portServerAddress; } - set { this.portServerAddress = value; } - } + public string? PortServerAddress { get; set; } /// - /// Gets or sets the port on which the Android Debug Bridge is listening for commands. + /// Gets or sets the port on which the Android Debug Bridge is listening for commands. + /// A value less than or equal to 0, or , indicates no Android Debug Bridge specified. /// - public int AndroidDebugBridgePort - { - get { return this.adbPort; } - set { this.adbPort = value; } - } + public int? AndroidDebugBridgePort { get; set; } /// /// Gets or sets a value indicating whether to skip version compatibility check /// between the driver and the browser. /// Defaults to . /// - public bool DisableBuildCheck - { - get { return this.disableBuildCheck; } - set { this.disableBuildCheck = value; } - } + public bool DisableBuildCheck { get; set; } /// /// Gets or sets a value indicating whether to enable verbose logging for the ChromeDriver executable. /// Defaults to . /// - public bool EnableVerboseLogging - { - get { return this.enableVerboseLogging; } - set { this.enableVerboseLogging = value; } - } + public bool EnableVerboseLogging { get; set; } /// /// Gets or sets a value indicating whether to enable appending to an existing ChromeDriver log file. /// Defaults to . /// - public bool EnableAppendLog - { - get { return this.enableAppendLog; } - set { this.enableAppendLog = value; } - } + public bool EnableAppendLog { get; set; } /// - /// Gets or sets the comma-delimited list of IP addresses that are approved to - /// connect to this instance of the Chrome driver. Defaults to an empty string, - /// which means only the local loopback address can connect. + /// Gets or sets the comma-delimited list of IP addresses that are approved to connect to this instance of the Chrome driver. + /// A value of or means only the local loopback address can connect. /// [Obsolete($"Use {nameof(AllowedIPAddresses)}")] - public string WhitelistedIPAddresses + public string? WhitelistedIPAddresses { - get { return this.allowedIPAddresses; } - set { this.allowedIPAddresses = value; } + get => this.AllowedIPAddresses; + set => this.AllowedIPAddresses = value; } /// - /// Gets or sets the comma-delimited list of IP addresses that are approved to - /// connect to this instance of the Chrome driver. Defaults to an empty string, - /// which means only the local loopback address can connect. + /// Gets or sets the comma-delimited list of IP addresses that are approved to connect to this instance of the Chrome driver. + /// A value of or means only the local loopback address can connect. /// - public string AllowedIPAddresses - { - get => this.allowedIPAddresses; - set => this.allowedIPAddresses = value; - } + public string? AllowedIPAddresses { get; set; } /// /// Gets the command-line arguments for the driver service. @@ -148,9 +111,9 @@ protected override string CommandLineArguments get { StringBuilder argsBuilder = new StringBuilder(base.CommandLineArguments); - if (this.adbPort > 0) + if (this.AndroidDebugBridgePort is int adb && adb > 0) { - argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --adb-port={0}", this.adbPort); + argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --adb-port={0}", adb); } if (this.SuppressInitialDiagnosticInformation) @@ -158,39 +121,39 @@ protected override string CommandLineArguments argsBuilder.Append(" --silent"); } - if (this.disableBuildCheck) + if (this.DisableBuildCheck) { argsBuilder.Append(" --disable-build-check"); } - if (this.enableVerboseLogging) + if (this.EnableVerboseLogging) { argsBuilder.Append(" --verbose"); } - if (this.enableAppendLog) + if (this.EnableAppendLog) { argsBuilder.Append(" --append-log"); } - if (!string.IsNullOrEmpty(this.logPath)) + if (!string.IsNullOrEmpty(this.LogPath)) { - argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --log-path=\"{0}\"", this.logPath); + argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --log-path=\"{0}\"", this.LogPath); } - if (!string.IsNullOrEmpty(this.urlPathPrefix)) + if (!string.IsNullOrEmpty(this.UrlPathPrefix)) { - argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --url-base={0}", this.urlPathPrefix); + argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --url-base={0}", this.UrlPathPrefix); } - if (!string.IsNullOrEmpty(this.portServerAddress)) + if (!string.IsNullOrEmpty(this.PortServerAddress)) { - argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --port-server={0}", this.portServerAddress); + argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --port-server={0}", this.PortServerAddress); } - if (!string.IsNullOrEmpty(this.allowedIPAddresses)) + if (!string.IsNullOrEmpty(this.AllowedIPAddresses)) { - argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " -allowed-ips={0}", this.allowedIPAddresses)); + argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " -allowed-ips={0}", this.AllowedIPAddresses)); } return argsBuilder.ToString(); diff --git a/dotnet/src/webdriver/DriverService.cs b/dotnet/src/webdriver/DriverService.cs index 80f2218ba57cc..a8907cf976769 100644 --- a/dotnet/src/webdriver/DriverService.cs +++ b/dotnet/src/webdriver/DriverService.cs @@ -20,12 +20,15 @@ using OpenQA.Selenium.Remote; using System; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; using System.Net; using System.Net.Http; using System.Threading.Tasks; +#nullable enable + namespace OpenQA.Selenium { /// @@ -33,15 +36,8 @@ namespace OpenQA.Selenium /// public abstract class DriverService : ICommandServer { - private string driverServicePath; - private string driverServiceExecutableName; - private string driverServiceHostName = "localhost"; - private int driverServicePort; - private bool silent; - private bool hideCommandPromptWindow; private bool isDisposed; - private Process driverServiceProcess; - private TimeSpan initializationTimeout = TimeSpan.FromSeconds(20); + private Process? driverServiceProcess; /// /// Initializes a new instance of the class. @@ -55,29 +51,33 @@ public abstract class DriverService : ICommandServer /// /// If the specified driver service executable does not exist in the specified directory. /// - protected DriverService(string servicePath, int port, string driverServiceExecutableName) + protected DriverService(string? servicePath, int port, string? driverServiceExecutableName) { - this.driverServicePath = servicePath; - this.driverServiceExecutableName = driverServiceExecutableName; - this.driverServicePort = port; + this.DriverServicePath = servicePath; + this.DriverServiceExecutableName = driverServiceExecutableName; + this.Port = port; } /// /// Occurs when the driver process is starting. /// - public event EventHandler DriverProcessStarting; + public event EventHandler? DriverProcessStarting; /// /// Occurs when the driver process has completely started. /// - public event EventHandler DriverProcessStarted; + public event EventHandler? DriverProcessStarted; /// /// Gets the Uri of the service. /// public Uri ServiceUrl { - get { return new Uri(string.Format(CultureInfo.InvariantCulture, "http://{0}:{1}", this.driverServiceHostName, this.driverServicePort)); } + get + { + string url = string.Format(CultureInfo.InvariantCulture, "http://{0}:{1}", this.HostName, this.Port); + return new Uri(url); + } } /// @@ -88,48 +88,30 @@ public Uri ServiceUrl /// (non-local) machines. This property can be used as a workaround so /// that an IP address (like "127.0.0.1" or "::1") can be used instead. /// - public string HostName - { - get { return this.driverServiceHostName; } - set { this.driverServiceHostName = value; } - } + public string HostName { get; set; } = "localhost"; /// /// Gets or sets the port of the service. /// - public int Port - { - get { return this.driverServicePort; } - set { this.driverServicePort = value; } - } + public int Port { get; set; } /// /// Gets or sets a value indicating whether the initial diagnostic information is suppressed /// when starting the driver server executable. Defaults to , meaning /// diagnostic information should be shown by the driver server executable. /// - public bool SuppressInitialDiagnosticInformation - { - get { return this.silent; } - set { this.silent = value; } - } + public bool SuppressInitialDiagnosticInformation { get; set; } /// /// Gets a value indicating whether the service is running. /// - public bool IsRunning - { - get { return this.driverServiceProcess != null && !this.driverServiceProcess.HasExited; } - } + [MemberNotNullWhen(true, nameof(driverServiceProcess))] + public bool IsRunning => this.driverServiceProcess != null && !this.driverServiceProcess.HasExited; /// /// Gets or sets a value indicating whether the command prompt window of the service should be hidden. /// - public bool HideCommandPromptWindow - { - get { return this.hideCommandPromptWindow; } - set { this.hideCommandPromptWindow = value; } - } + public bool HideCommandPromptWindow { get; set; } /// /// Gets the process ID of the running driver service executable. Returns 0 if the process is not running. @@ -159,54 +141,33 @@ public int ProcessId /// /// Gets or sets a value indicating the time to wait for an initial connection before timing out. /// - public TimeSpan InitializationTimeout - { - get { return this.initializationTimeout; } - set { this.initializationTimeout = value; } - } + public TimeSpan InitializationTimeout { get; set; } = TimeSpan.FromSeconds(20); /// /// Gets or sets the executable file name of the driver service. /// - public string DriverServiceExecutableName - { - get { return this.driverServiceExecutableName; } - set { this.driverServiceExecutableName = value; } - } + public string? DriverServiceExecutableName { get; set; } /// /// Gets or sets the path of the driver service. /// - public string DriverServicePath - { - get { return this.driverServicePath; } - set { this.driverServicePath = value; } - } + public string? DriverServicePath { get; set; } /// /// Gets the command-line arguments for the driver service. /// - protected virtual string CommandLineArguments - { - get { return string.Format(CultureInfo.InvariantCulture, "--port={0}", this.driverServicePort); } - } + protected virtual string CommandLineArguments => string.Format(CultureInfo.InvariantCulture, "--port={0}", this.Port); /// /// Gets a value indicating the time to wait for the service to terminate before forcing it to terminate. /// - protected virtual TimeSpan TerminationTimeout - { - get { return TimeSpan.FromSeconds(10); } - } + protected virtual TimeSpan TerminationTimeout => TimeSpan.FromSeconds(10); /// /// Gets a value indicating whether the service has a shutdown API that can be called to terminate /// it gracefully before forcing a termination. /// - protected virtual bool HasShutdown - { - get { return true; } - } + protected virtual bool HasShutdown => true; /// /// Gets a value indicating whether the service is responding to HTTP requests. @@ -231,7 +192,7 @@ protected virtual bool IsInitialized // that the HTTP status returned is a 200 status, and that the resposne has the correct // Content-Type header. A more sophisticated check would parse the JSON response and // validate its values. At the moment we do not do this more sophisticated check. - isInitialized = response.StatusCode == HttpStatusCode.OK && response.Content.Headers.ContentType.MediaType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase); + isInitialized = response.StatusCode == HttpStatusCode.OK && response.Content.Headers.ContentType is { MediaType: string mediaType } && mediaType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase); } } } @@ -256,6 +217,7 @@ public void Dispose() /// /// Starts the DriverService if it is not already running. /// + [MemberNotNull(nameof(driverServiceProcess))] public void Start() { if (this.driverServiceProcess != null) @@ -265,9 +227,14 @@ public void Start() this.driverServiceProcess = new Process(); - if (this.driverServicePath != null) + if (this.DriverServicePath != null) { - this.driverServiceProcess.StartInfo.FileName = Path.Combine(this.driverServicePath, this.driverServiceExecutableName); + if (this.DriverServiceExecutableName is null) + { + throw new InvalidOperationException("If the driver service path is specified, the driver service executable name must be as well"); + } + + this.driverServiceProcess.StartInfo.FileName = Path.Combine(this.DriverServicePath, this.DriverServiceExecutableName); } else { @@ -276,7 +243,7 @@ public void Start() this.driverServiceProcess.StartInfo.Arguments = this.CommandLineArguments; this.driverServiceProcess.StartInfo.UseShellExecute = false; - this.driverServiceProcess.StartInfo.CreateNoWindow = this.hideCommandPromptWindow; + this.driverServiceProcess.StartInfo.CreateNoWindow = this.HideCommandPromptWindow; DriverProcessStartingEventArgs eventArgs = new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo); this.OnDriverProcessStarting(eventArgs); @@ -288,8 +255,7 @@ public void Start() if (!serviceAvailable) { - string msg = "Cannot start the driver service on " + this.ServiceUrl; - throw new WebDriverException(msg); + throw new WebDriverException($"Cannot start the driver service on {this.ServiceUrl}"); } } @@ -327,10 +293,7 @@ protected void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs) throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null"); } - if (this.DriverProcessStarting != null) - { - this.DriverProcessStarting(this, eventArgs); - } + this.DriverProcessStarting?.Invoke(this, eventArgs); } /// @@ -344,10 +307,7 @@ protected void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs) throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null"); } - if (this.DriverProcessStarted != null) - { - this.DriverProcessStarted(this, eventArgs); - } + this.DriverProcessStarted?.Invoke(this, eventArgs); } /// diff --git a/dotnet/src/webdriver/Safari/SafariDriverService.cs b/dotnet/src/webdriver/Safari/SafariDriverService.cs index 4130e086f0a30..b3ee30a85ddd7 100644 --- a/dotnet/src/webdriver/Safari/SafariDriverService.cs +++ b/dotnet/src/webdriver/Safari/SafariDriverService.cs @@ -25,6 +25,8 @@ using System.Text; using System.Threading.Tasks; +#nullable enable + namespace OpenQA.Selenium.Safari { /// @@ -37,10 +39,10 @@ public sealed class SafariDriverService : DriverService /// /// Initializes a new instance of the class. /// - /// The full path to the SafariDriver executable. + /// The directory of the SafariDriver executable. /// The file name of the SafariDriver executable. /// The port on which the SafariDriver executable should listen. - private SafariDriverService(string executablePath, string executableFileName, int port) + private SafariDriverService(string? executablePath, string? executableFileName, int port) : base(executablePath, port, executableFileName) { } @@ -73,7 +75,7 @@ protected override TimeSpan TerminationTimeout // because the executable does not have a clean shutdown command, // which means we have to kill the process. Using a short timeout // gets us to the termination point much faster. - get { return TimeSpan.FromMilliseconds(100); } + get => TimeSpan.FromMilliseconds(100); } /// @@ -84,7 +86,7 @@ protected override bool HasShutdown { // The Safari driver executable does not have a clean shutdown command, // which means we have to kill the process. - get { return false; } + get => false; } /// @@ -107,7 +109,7 @@ public static SafariDriverService CreateDefaultService(string driverPath) if (File.Exists(driverPath)) { fileName = Path.GetFileName(driverPath); - driverPath = Path.GetDirectoryName(driverPath); + driverPath = Path.GetDirectoryName(driverPath)!; } else {