[dotnet] Possibility to override underlying HttpClient/HttpClientHandler for all HTTP requests

[nvborisenko]

User description

Motivation and Context

Implements #15280

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

---

Description

• Introduced the ability to override HttpClientHandler for all HTTP requests.

• Added a new protected method CreateHttpClientHandler for custom handler creation.

• Enhanced CreateHttpClient to utilize the new CreateHttpClientHandler method.

• Improved flexibility for HTTP client customization in remote WebDriver interactions.

---

Changes walkthrough 📝

	Relevant files
Enhancement	HttpCommandExecutor.cs Added support for customizable HttpClientHandler                  dotnet/src/webdriver/Remote/HttpCommandExecutor.cs Added a new protected method CreateHttpClientHandler for creating custom HttpClientHandler. Updated CreateHttpClient to use the new CreateHttpClientHandler method. Enhanced documentation for CreateHttpClient and CreateHttpClientHandler. +16/-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.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 Security concerns Credential Exposure: The code handles user credentials for authentication. While the implementation appears to use proper credential objects, care should be taken to ensure credentials are not logged or exposed, especially with the logging handler added on line 254. The credentials should also be properly secured in memory.

⚡ Recommended focus areas for review Resource Management The HttpClient and HttpClientHandler instances should be properly disposed. Consider implementing IDisposable pattern or ensuring proper cleanup in the class. protected virtual HttpClientHandler CreateHttpClientHandler() { HttpClientHandler httpClientHandler = new HttpClientHandler(); string userInfo = this.remoteServerUri.UserInfo; if (!string.IsNullOrEmpty(userInfo) && userInfo.Contains(":")) { string[] userInfoComponents = this.remoteServerUri.UserInfo.Split(new char[] { ':' }, 2); httpClientHandler.Credentials = new NetworkCredential(userInfoComponents[0], userInfoComponents[1]); httpClientHandler.PreAuthenticate = true; } httpClientHandler.Proxy = this.Proxy; return httpClientHandler; } /// <summary> /// Creates an instance of <see cref="HttpClient"/> used by making all HTTP calls to remote end. /// </summary> /// <returns>An instance of <see cref="HttpClient"/>.</returns> protected virtual HttpClient CreateHttpClient() { var httpClientHandler = CreateHttpClientHandler(); HttpMessageHandler handler = httpClientHandler;

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Add null check for handler Suggestion Impact: The commit implements a null check for httpClientHandler using null-coalescing operator with throw, which achieves the same goal as the suggested if-statement approach code diff: - var httpClientHandler = CreateHttpClientHandler(); + var httpClientHandler = CreateHttpClientHandler() ?? throw new InvalidOperationException($"{nameof(CreateHttpClientHandler)} method returned null"); Add null check for the returned HttpClientHandler from CreateHttpClientHandler() before using it to avoid potential NullReferenceException dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [248-252] protected virtual HttpClient CreateHttpClient() { var httpClientHandler = CreateHttpClientHandler(); + if (httpClientHandler == null) + { + throw new InvalidOperationException("CreateHttpClientHandler() returned null"); + } HttpMessageHandler handler = httpClientHandler; Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a potential null reference exception that could occur if CreateHttpClientHandler() returns null. This is a critical defensive programming practice for virtual methods that can be overridden by derived classes.	Medium
Learned best practice	✅ Add null validation check for required class field to prevent potential NullReferenceException Suggestion Impact: While the exact suggested null check wasn't added, the commit added a null check in CreateHttpClient() that validates the result of CreateHttpClientHandler(), which would catch null remoteServerUri issues code diff: + var httpClientHandler = CreateHttpClientHandler() + ?? throw new InvalidOperationException($"{nameof(CreateHttpClientHandler)} method returned null"); The CreateHttpClientHandler() method uses this.remoteServerUri without validating that it's not null. Since this is a required field for the class functionality, add a null check at the start of the method to fail fast with a clear error message. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [229-233] protected virtual HttpClientHandler CreateHttpClientHandler() { + ArgumentNullException.ThrowIfNull(this.remoteServerUri, nameof(remoteServerUri)); HttpClientHandler httpClientHandler = new HttpClientHandler(); string userInfo = this.remoteServerUri.UserInfo; if (!string.IsNullOrEmpty(userInfo) && userInfo.Contains(":")) Apply this suggestion Suggestion importance[1-10]: 6	Low

[RenderMichael]

Looks nice and simple, just a single comment

[RenderMichael]

Quick question, should the CreateHttpClientHandler return an HttpMessageHandler so users can return more general types?

[nvborisenko]

I also was thinking about it, but then users will write:

var handler = base.CreateHttpClientHandler();
handler.Proxy // hm, where is Proxy?

And more useful properties from HttpClientHandler class.

[nvborisenko]

Failed CI not related to this changes.