Http consistency: HttpSkill, BingConnector, GoogleConnector, and WebF…

…ileDownloadSkill (#1551) ### Description The HttpSkill, BingConnector, GoogleConnector, and WebFileDownloadSkill classes have been modified to accept an instance of an HTTP client, allowing hosting applications/client code the freedom to use their own instance if needed. ### Contribution Checklist  - [x] The code builds clean without any errors or warnings - [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [x] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 Co-authored-by: Lee Miller <lemiller@microsoft.com>
microsoft · Jun 20, 2023 · ac3d655 · ac3d655
1 parent b9b9804
commit ac3d655
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 23 deletions.
diff --git a/dotnet/src/SemanticKernel/CoreSkills/HttpSkill.cs b/dotnet/src/SemanticKernel/CoreSkills/HttpSkill.cs
@@ -25,13 +25,12 @@ namespace Microsoft.SemanticKernel.CoreSkills;
     Justification = "Semantic Kernel operates on strings")]
 public sealed class HttpSkill : IDisposable
 {
-    private static readonly HttpClientHandler s_httpClientHandler = new() { CheckCertificateRevocationList = true };
     private readonly HttpClient _client;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="HttpSkill"/> class.
     /// </summary>
-    public HttpSkill() : this(new HttpClient(s_httpClientHandler, disposeHandler: false))
+    public HttpSkill() : this(new HttpClient(NonDisposableHttpClientHandler.Instance, disposeHandler: false))
     {
     }
 
@@ -112,5 +111,8 @@ private async Task<string> SendRequestAsync(string uri, HttpMethod method, HttpC
     /// <summary>
     /// Disposes resources
     /// </summary>
-    public void Dispose() => this._client.Dispose();
+    [Obsolete("This method is deprecated and will be removed in one of the next SK SDK versions. There is no longer a need to invoke this method, and its call can be safely omitted.")]
+    public void Dispose()
+    {
+    }
 }
diff --git a/dotnet/src/Skills/Skills.Web/Bing/BingConnector.cs b/dotnet/src/Skills/Skills.Web/Bing/BingConnector.cs
@@ -11,6 +11,7 @@
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.SemanticKernel.Diagnostics;
 
 namespace Microsoft.SemanticKernel.Skills.Web.Bing;
 
@@ -20,15 +21,32 @@ namespace Microsoft.SemanticKernel.Skills.Web.Bing;
 public sealed class BingConnector : IWebSearchEngineConnector, IDisposable
 {
     private readonly ILogger _logger;
-    private readonly HttpClientHandler _httpClientHandler;
     private readonly HttpClient _httpClient;
+    private readonly string? _apiKey;
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="BingConnector"/> class.
+    /// </summary>
+    /// <param name="apiKey">The API key to authenticate the connector.</param>
+    /// <param name="logger">An optional logger to log connector-related information.</param>
+    public BingConnector(string apiKey, ILogger<BingConnector>? logger = null) :
+        this(apiKey, new HttpClient(NonDisposableHttpClientHandler.Instance, false), logger)
+    {
+    }
 
-    public BingConnector(string apiKey, ILogger<BingConnector>? logger = null)
+    /// <summary>
+    /// Initializes a new instance of the <see cref="BingConnector"/> class.
+    /// </summary>
+    /// <param name="apiKey">The API key to authenticate the connector.</param>
+    /// <param name="httpClient">The HTTP client to use for making requests.</param>
+    /// <param name="logger">An optional logger to log connector-related information.</param>
+    public BingConnector(string apiKey, HttpClient httpClient, ILogger<BingConnector>? logger = null)
     {
+        Verify.NotNull(httpClient);
+
+        this._apiKey = apiKey;
         this._logger = logger ?? NullLogger<BingConnector>.Instance;
-        this._httpClientHandler = new() { CheckCertificateRevocationList = true };
-        this._httpClient = new HttpClient(this._httpClientHandler);
-        this._httpClient.DefaultRequestHeaders.Add("Ocp-Apim-Subscription-Key", apiKey);
+        this._httpClient = httpClient;
     }
 
     /// <inheritdoc/>
@@ -43,28 +61,47 @@ public async Task<IEnumerable<string>> SearchAsync(string query, int count = 1,
         Uri uri = new($"https://api.bing.microsoft.com/v7.0/search?q={Uri.EscapeDataString(query)}&count={count}&offset={offset}");
 
         this._logger.LogDebug("Sending request: {0}", uri);
-        HttpResponseMessage response = await this._httpClient.GetAsync(uri, cancellationToken).ConfigureAwait(false);
+
+        using HttpResponseMessage response = await this.SendGetRequest(uri, cancellationToken).ConfigureAwait(false);
+
         response.EnsureSuccessStatusCode();
+
         this._logger.LogDebug("Response received: {0}", response.StatusCode);
 
         string json = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
         this._logger.LogTrace("Response content received: {0}", json);
 
         BingSearchResponse? data = JsonSerializer.Deserialize<BingSearchResponse>(json);
+
         WebPage[]? results = data?.WebPages?.Value;
 
         return results == null ? Enumerable.Empty<string>() : results.Select(x => x.Snippet);
     }
 
-    private void Dispose(bool disposing)
+    /// <summary>
+    /// Sends a GET request to the specified URI.
+    /// </summary>
+    /// <param name="uri">The URI to send the request to.</param>
+    /// <param name="cancellationToken">A cancellation token to cancel the request.</param>
+    /// <returns>A <see cref="HttpResponseMessage"/> representing the response from the request.</returns>
+    private async Task<HttpResponseMessage> SendGetRequest(Uri uri, CancellationToken cancellationToken = default)
     {
-        if (disposing)
+        using var httpRequestMessage = new HttpRequestMessage(HttpMethod.Get, uri);
+
+        if (!string.IsNullOrEmpty(this._apiKey))
         {
-            this._httpClient.Dispose();
-            this._httpClientHandler.Dispose();
+            httpRequestMessage.Headers.Add("Ocp-Apim-Subscription-Key", this._apiKey);
         }
+
+        return await this._httpClient.SendAsync(httpRequestMessage, cancellationToken).ConfigureAwait(false);
+    }
+
+    [Obsolete("This method is deprecated and will be removed in one of the next SK SDK versions. There is no longer a need to invoke this method, and its call can be safely omitted.")]
+    private void Dispose(bool disposing)
+    {
     }
 
+    [Obsolete("This method is deprecated and will be removed in one of the next SK SDK versions. There is no longer a need to invoke this method, and its call can be safely omitted.")]
     public void Dispose()
     {
         // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method

diff --git a/dotnet/src/Skills/Skills.Web/Google/GoogleConnector.cs b/dotnet/src/Skills/Skills.Web/Google/GoogleConnector.cs
@@ -9,6 +9,7 @@
 using Google.Apis.Services;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.SemanticKernel.Diagnostics;
 
 namespace Microsoft.SemanticKernel.Skills.Web.Google;
 
@@ -22,17 +23,34 @@ public sealed class GoogleConnector : IWebSearchEngineConnector, IDisposable
     private readonly string? _searchEngineId;
 
     /// <summary>
-    /// Google search connector
+    /// Google search connector.
     /// </summary>
     /// <param name="apiKey">Google Custom Search API (looks like "ABcdEfG1...")</param>
     /// <param name="searchEngineId">Google Search Engine ID (looks like "a12b345...")</param>
     /// <param name="logger">Optional logger</param>
     public GoogleConnector(
         string apiKey,
         string searchEngineId,
+        ILogger<GoogleConnector>? logger = null) : this(new BaseClientService.Initializer { ApiKey = apiKey }, searchEngineId, logger)
+    {
+        Verify.NotNullOrWhiteSpace(apiKey);
+    }
+
+    /// <summary>
+    /// Google search connector.
+    /// </summary>
+    /// <param name="initializer">The connector initializer</param>
+    /// <param name="searchEngineId">Google Search Engine ID (looks like "a12b345...")</param>
+    /// <param name="logger">Optional logger</param>
+    public GoogleConnector(
+        BaseClientService.Initializer initializer,
+        string searchEngineId,
         ILogger<GoogleConnector>? logger = null)
     {
-        this._search = new CustomSearchAPIService(new BaseClientService.Initializer { ApiKey = apiKey });
+        Verify.NotNull(initializer);
+        Verify.NotNullOrWhiteSpace(searchEngineId);
+
+        this._search = new CustomSearchAPIService(initializer);
         this._searchEngineId = searchEngineId;
         this._logger = logger ?? NullLogger<GoogleConnector>.Instance;
     }

diff --git a/dotnet/src/Skills/Skills.Web/Skills.Web.csproj b/dotnet/src/Skills/Skills.Web/Skills.Web.csproj
@@ -8,6 +8,7 @@
   </PropertyGroup>
 
   <Import Project="$(RepoRoot)/dotnet/nuget/nuget-package.props" />
+  <Import Project="$(RepoRoot)/dotnet/src/InternalUtilities/src/InternalUtilities.props" />
 
   <PropertyGroup>
     <!-- NuGet Package Settings -->

diff --git a/dotnet/src/Skills/Skills.Web/WebFileDownloadSkill.cs b/dotnet/src/Skills/Skills.Web/WebFileDownloadSkill.cs
@@ -24,18 +24,26 @@ public sealed class WebFileDownloadSkill : IDisposable
     public const string FilePathParamName = "filePath";
 
     private readonly ILogger _logger;
-    private readonly HttpClientHandler _httpClientHandler;
     private readonly HttpClient _httpClient;
 
     /// <summary>
-    /// Constructor for WebFileDownloadSkill.
+    /// Initializes a new instance of the <see cref="WebFileDownloadSkill"/> class.
     /// </summary>
-    /// <param name="logger">Optional logger.</param>
-    public WebFileDownloadSkill(ILogger<WebFileDownloadSkill>? logger = null)
+    /// <param name="logger">An optional logger to log skill-related information.</param>
+    public WebFileDownloadSkill(ILogger<WebFileDownloadSkill>? logger = null) :
+        this(new HttpClient(NonDisposableHttpClientHandler.Instance, false), logger)
     {
+    }
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="WebFileDownloadSkill"/> class.
+    /// </summary>
+    /// <param name="httpClient">The HTTP client to use for making requests.</param>
+    /// <param name="logger">An optional logger to log skill-related information.</param>
+    public WebFileDownloadSkill(HttpClient httpClient, ILogger<WebFileDownloadSkill>? logger = null)
+    {
+        this._httpClient = httpClient;
         this._logger = logger ?? NullLogger<WebFileDownloadSkill>.Instance;
-        this._httpClientHandler = new() { CheckCertificateRevocationList = true };
-        this._httpClient = new HttpClient(this._httpClientHandler);
     }
 
     /// <summary>
@@ -68,9 +76,8 @@ public async Task DownloadToFileAsync(
     /// <summary>
     /// Implementation of IDisposable.
     /// </summary>
+    [Obsolete("This method is deprecated and will be removed in one of the next SK SDK versions. There is no longer a need to invoke this method, and its call can be safely omitted.")]
     public void Dispose()
     {
-        this._httpClient.Dispose();
-        this._httpClientHandler.Dispose();
     }
 }