Merge pull request #393 from SixLabors/js/feature-client-provider

Fix, cleanup, and optimize new cloud provider code.

Author: JimBobSquarePants

src/ImageSharp.Web.Providers.AWS/AmazonS3BucketClient.cs

@@ -0,0 +1,56 @@
+// Copyright (c) Six Labors.
+// Licensed under the Six Labors Split License.
+
+using Amazon.S3;
+
+namespace SixLabors.ImageSharp.Web;
+
+/// <summary>
+/// Represents a scoped Amazon S3 client instance that is explicitly associated with a single S3 bucket.
+/// This wrapper provides a strongly-typed link between the client and the bucket it operates on,
+/// and optionally manages the lifetime of the underlying <see cref="AmazonS3Client"/>.
+/// </summary>
+public sealed class AmazonS3BucketClient : IDisposable
+{
+    private readonly bool disposeClient;
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="AmazonS3BucketClient"/> class.
+    /// </summary>
+    /// <param name="bucketName">
+    /// The bucket name associated with this client instance.
+    /// </param>
+    /// <param name="client">
+    /// The underlying Amazon S3 client instance. This should be an already configured instance of <see cref="AmazonS3Client"/>.
+    /// </param>
+    /// <param name="disposeClient">
+    /// A value indicating whether the underlying client should be disposed when this instance is disposed.
+    /// </param>
+    public AmazonS3BucketClient(string bucketName, AmazonS3Client client, bool disposeClient = true)
+    {
+        Guard.NotNullOrWhiteSpace(bucketName, nameof(bucketName));
+        Guard.NotNull(client, nameof(client));
+        this.BucketName = bucketName;
+        this.Client = client;
+        this.disposeClient = disposeClient;
+    }
+
+    /// <summary>
+    /// Gets the bucket name associated with this client instance.
+    /// </summary>
+    public string BucketName { get; }
+
+    /// <summary>
+    /// Gets the underlying Amazon S3 client instance.
+    /// </summary>
+    public AmazonS3Client Client { get; }
+
+    /// <inheritdoc/>
+    public void Dispose()
+    {
+        if (this.disposeClient)
+        {
+            this.Client.Dispose();
+        }
+    }
+}

src/ImageSharp.Web.Providers.AWS/AmazonS3ClientFactory.cs

@@ -14,25 +14,20 @@ internal static class AmazonS3ClientFactory
     /// with the same name does not already exist.
     /// </summary>
     /// <param name="options">The AWS S3 Storage cache options.</param>
-    /// <param name="serviceProvider">The current service provider.</param>
     /// <returns>
     /// A new <see cref="AmazonS3Client"/>.
     /// </returns>
     /// <exception cref="ArgumentException">Invalid configuration.</exception>
-    public static AmazonS3Client CreateClient(IAWSS3BucketClientOptions options, IServiceProvider serviceProvider)
+    public static AmazonS3BucketClient CreateClient(IAWSS3BucketClientOptions options)
     {
-        if (options.S3ClientProvider != null)
-        {
-            return options.S3ClientProvider(options, serviceProvider);
-        }
-        else if (!string.IsNullOrWhiteSpace(options.Endpoint))
+        if (!string.IsNullOrWhiteSpace(options.Endpoint))
         {
             // AccessKey can be empty.
             // AccessSecret can be empty.
             // PathStyle endpoint doesn't support AccelerateEndpoint.
             AmazonS3Config config = new() { ServiceURL = options.Endpoint, ForcePathStyle = true, AuthenticationRegion = options.Region };
             SetTimeout(config, options.Timeout);
-            return new AmazonS3Client(options.AccessKey, options.AccessSecret, config);
+            return new(options.BucketName, new AmazonS3Client(options.AccessKey, options.AccessSecret, config));
         }
         else if (!string.IsNullOrWhiteSpace(options.AccessKey))
         {
@@ -41,14 +36,14 @@ public static AmazonS3Client CreateClient(IAWSS3BucketClientOptions options, ISe
             RegionEndpoint region = RegionEndpoint.GetBySystemName(options.Region);
             AmazonS3Config config = new() { RegionEndpoint = region, UseAccelerateEndpoint = options.UseAccelerateEndpoint };
             SetTimeout(config, options.Timeout);
-            return new AmazonS3Client(options.AccessKey, options.AccessSecret, config);
+            return new(options.BucketName, new AmazonS3Client(options.AccessKey, options.AccessSecret, config));
         }
         else if (!string.IsNullOrWhiteSpace(options.Region))
         {
             RegionEndpoint region = RegionEndpoint.GetBySystemName(options.Region);
             AmazonS3Config config = new() { RegionEndpoint = region, UseAccelerateEndpoint = options.UseAccelerateEndpoint };
             SetTimeout(config, options.Timeout);
-            return new AmazonS3Client(config);
+            return new(options.BucketName, new AmazonS3Client(config));
         }
         else
         {

src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs

@@ -13,11 +13,12 @@ namespace SixLabors.ImageSharp.Web.Caching.AWS;
 /// <summary>
 /// Implements an AWS S3 Storage based cache.
 /// </summary>
-public class AWSS3StorageCache : IImageCache
+public class AWSS3StorageCache : IImageCache, IDisposable
 {
-    private readonly IAmazonS3 amazonS3Client;
+    private readonly AmazonS3BucketClient amazonS3Client;
     private readonly string bucketName;
     private readonly string cacheFolder;
+    private bool isDisposed;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="AWSS3StorageCache"/> class.
@@ -28,8 +29,13 @@ public AWSS3StorageCache(IOptions<AWSS3StorageCacheOptions> cacheOptions, IServi
     {
         Guard.NotNull(cacheOptions, nameof(cacheOptions));
         AWSS3StorageCacheOptions options = cacheOptions.Value;
-        this.bucketName = options.BucketName;
-        this.amazonS3Client = AmazonS3ClientFactory.CreateClient(options, serviceProvider);
+
+        this.amazonS3Client =
+            options.S3ClientFactory?.Invoke(options, serviceProvider)
+            ?? AmazonS3ClientFactory.CreateClient(options);
+
+        this.bucketName = this.amazonS3Client.BucketName;
+
         this.cacheFolder = string.IsNullOrEmpty(options.CacheFolder)
             ? string.Empty
             : options.CacheFolder.Trim().Trim('/') + '/';
@@ -43,8 +49,8 @@ public AWSS3StorageCache(IOptions<AWSS3StorageCacheOptions> cacheOptions, IServi
         try
         {
             // HEAD request throws a 404 if not found.
-            MetadataCollection metadata = (await this.amazonS3Client.GetObjectMetadataAsync(request)).Metadata;
-            return new AWSS3StorageCacheResolver(this.amazonS3Client, this.bucketName, keyWithFolder, metadata);
+            MetadataCollection metadata = (await this.amazonS3Client.Client.GetObjectMetadataAsync(request)).Metadata;
+            return new AWSS3StorageCacheResolver(this.amazonS3Client.Client, this.bucketName, keyWithFolder, metadata);
         }
         catch
         {
@@ -70,7 +76,7 @@ public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
             request.Metadata.Add(d.Key, d.Value);
         }
 
-        return this.amazonS3Client.PutObjectAsync(request);
+        return this.amazonS3Client.Client.PutObjectAsync(request);
     }
 
     /// <summary>
@@ -84,23 +90,42 @@ public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
     /// and object data. <see cref="S3CannedACL.Private"/> specifies that the bucket
     /// data is private to the account owner.
     /// </param>
-    /// <param name="serviceProvider">The current service provider.</param>
     /// <returns>
     /// If the bucket does not already exist, a <see cref="PutBucketResponse"/> describing the newly
     /// created bucket. If the container already exists, <see langword="null"/>.
     /// </returns>
-    public static PutBucketResponse? CreateIfNotExists(
-        AWSS3StorageCacheOptions options,
-        S3CannedACL acl,
-        IServiceProvider serviceProvider)
-        => AsyncHelper.RunSync(() => CreateIfNotExistsAsync(options, acl, serviceProvider));
-
-    private static async Task<PutBucketResponse?> CreateIfNotExistsAsync(
-        AWSS3StorageCacheOptions options,
-        S3CannedACL acl,
-        IServiceProvider serviceProvider)
+    public static PutBucketResponse? CreateIfNotExists(AWSS3StorageCacheOptions options, S3CannedACL acl)
+        => AsyncHelper.RunSync(() => CreateIfNotExistsAsync(options, acl));
+
+    /// <summary>
+    /// Releases the unmanaged resources used by the <see cref="AWSS3StorageCache"/> and optionally releases the managed resources.
+    /// </summary>
+    /// <param name="disposing">true to release both managed and unmanaged resources; false to release only unmanaged resources.</param>
+    protected virtual void Dispose(bool disposing)
+    {
+        if (!this.isDisposed)
+        {
+            if (disposing)
+            {
+                this.amazonS3Client?.Dispose();
+            }
+
+            this.isDisposed = true;
+        }
+    }
+
+    /// <inheritdoc/>
+    public void Dispose()
+    {
+        // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
+        this.Dispose(disposing: true);
+        GC.SuppressFinalize(this);
+    }
+
+    private static async Task<PutBucketResponse?> CreateIfNotExistsAsync(AWSS3StorageCacheOptions options, S3CannedACL acl)
     {
-        AmazonS3Client client = AmazonS3ClientFactory.CreateClient(options, serviceProvider);
+        using AmazonS3BucketClient bucketClient = AmazonS3ClientFactory.CreateClient(options);
+        AmazonS3Client client = bucketClient.Client;
 
         bool foundBucket = false;
         ListBucketsResponse listBucketsResponse = await client.ListBucketsAsync();

src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCacheOptions.cs

@@ -1,8 +1,6 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
-using Amazon.S3;
-
 namespace SixLabors.ImageSharp.Web.Caching.AWS;
 
 /// <summary>
@@ -11,7 +9,7 @@ namespace SixLabors.ImageSharp.Web.Caching.AWS;
 public class AWSS3StorageCacheOptions : IAWSS3BucketClientOptions
 {
     /// <inheritdoc/>
-    public Func<IAWSS3BucketClientOptions, IServiceProvider, AmazonS3Client>? S3ClientProvider { get; set; } = null!;
+    public Func<IAWSS3BucketClientOptions, IServiceProvider, AmazonS3BucketClient>? S3ClientFactory { get; set; }
 
     /// <inheritdoc/>
     public string? Region { get; set; }

src/ImageSharp.Web.Providers.AWS/IAWSS3BucketClientOptions.cs

@@ -1,8 +1,6 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
-using Amazon.S3;
-
 namespace SixLabors.ImageSharp.Web;
 
 /// <summary>
@@ -11,49 +9,54 @@ namespace SixLabors.ImageSharp.Web;
 public interface IAWSS3BucketClientOptions
 {
     /// <summary>
-    /// Gets or sets a custom Azure AmazonS3Client provider
+    /// Gets or sets a custom factory method to create an <see cref="AmazonS3BucketClient"/>.
     /// </summary>
-    Func<IAWSS3BucketClientOptions, IServiceProvider, AmazonS3Client>? S3ClientProvider { get; set; }
+    public Func<IAWSS3BucketClientOptions, IServiceProvider, AmazonS3BucketClient>? S3ClientFactory { get; set; }
 
     /// <summary>
     /// Gets or sets the AWS region endpoint (us-east-1/us-west-1/ap-southeast-2).
     /// </summary>
-    string? Region { get; set; }
+    public string? Region { get; set; }
 
     /// <summary>
     /// Gets or sets the AWS bucket name.
+    /// Cannot be <see langword="null"/> when <see cref="S3ClientFactory"/> is not set.
     /// </summary>
-    string BucketName { get; set; }
+    public string BucketName { get; set; }
 
     /// <summary>
     /// Gets or sets the AWS key - Can be used to override keys provided by the environment.
     /// If deploying inside an EC2 instance AWS keys will already be available via environment
-    /// variables and don't need to be specified. Follow AWS best security practices on  <see href="https://docs.aws.amazon.com/general/latest/gr/aws-access-keys-best-practices.html"/>.
+    /// variables and don't need to be specified. Follow AWS best security practices on
+    /// <see href="https://docs.aws.amazon.com/general/latest/gr/aws-access-keys-best-practices.html"/>.
     /// </summary>
-    string? AccessKey { get; set; }
+    public string? AccessKey { get; set; }
 
     /// <summary>
     /// Gets or sets the AWS endpoint - used to override the default service endpoint.
     /// If deploying inside an EC2 instance AWS keys will already be available via environment
-    /// variables and don't need to be specified. Follow AWS best security practices on  <see href="https://docs.aws.amazon.com/general/latest/gr/aws-access-keys-best-practices.html"/>.
+    /// variables and don't need to be specified. Follow AWS best security practices on
+    /// <see href="https://docs.aws.amazon.com/general/latest/gr/aws-access-keys-best-practices.html"/>.
     /// </summary>
-    string? AccessSecret { get; set; }
+    public string? AccessSecret { get; set; }
 
     /// <summary>
     /// Gets or sets the AWS endpoint - used for testing to over region endpoint allowing it
     /// to be set to localhost.
     /// </summary>
-    string? Endpoint { get; set; }
+    public string? Endpoint { get; set; }
 
     /// <summary>
     /// Gets or sets a value indicating whether the S3 accelerate endpoint is used.
-    /// The feature must be enabled on the bucket. Follow AWS instruction on <see href="https://docs.aws.amazon.com/AmazonS3/latest/userguide/transfer-acceleration.html"/>.
+    /// The feature must be enabled on the bucket. Follow AWS instruction on
+    /// <see href="https://docs.aws.amazon.com/AmazonS3/latest/userguide/transfer-acceleration.html"/>.
     /// </summary>
-    bool UseAccelerateEndpoint { get; set; }
+    public bool UseAccelerateEndpoint { get; set; }
 
     /// <summary>
     /// Gets or sets a value indicating the timeout for the S3 client.
-    /// If the value is set, the value is assigned to the Timeout property of the HttpWebRequest/HttpClient object used to send requests.
+    /// If the value is set, the value is assigned to the Timeout property of the HttpWebRequest/HttpClient
+    /// object used to send requests.
     /// </summary>
-    TimeSpan? Timeout { get; set; }
+    public TimeSpan? Timeout { get; set; }
 }