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

Cache folder configuration for Azure and AWS Image Caches (Lombiq Technologies: OCORE-136) #353

Merged
merged 8 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public class AWSS3StorageCache : IImageCache
{
private readonly IAmazonS3 amazonS3Client;
private readonly string bucketName;
private readonly string cacheFolder;

/// <summary>
/// Initializes a new instance of the <see cref="AWSS3StorageCache"/> class.
Expand All @@ -28,17 +29,21 @@ public AWSS3StorageCache(IOptions<AWSS3StorageCacheOptions> cacheOptions)
AWSS3StorageCacheOptions options = cacheOptions.Value;
this.bucketName = options.BucketName;
this.amazonS3Client = AmazonS3ClientFactory.CreateClient(options);
this.cacheFolder = string.IsNullOrEmpty(options.CacheFolder)
? string.Empty
: options.CacheFolder.Trim().Trim('/') + '/';
Comment on lines +32 to +34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I DRY this into a helper class, not to copy it between the two cache implementations?

Copy link
Member

Choose a reason for hiding this comment

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

I would leave as is and keep them separate. They're two separate assemblies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK then.

}

/// <inheritdoc/>
public async Task<IImageCacheResolver?> GetAsync(string key)
{
GetObjectMetadataRequest request = new() { BucketName = this.bucketName, Key = key };
string keyWithFolder = this.GetKeyWithFolder(key);
GetObjectMetadataRequest request = new() { BucketName = this.bucketName, Key = keyWithFolder };
try
{
// HEAD request throws a 404 if not found.
MetadataCollection metadata = (await this.amazonS3Client.GetObjectMetadataAsync(request)).Metadata;
return new AWSS3StorageCacheResolver(this.amazonS3Client, this.bucketName, key, metadata);
return new AWSS3StorageCacheResolver(this.amazonS3Client, this.bucketName, keyWithFolder, metadata);
}
catch
{
Expand All @@ -52,7 +57,7 @@ public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
PutObjectRequest request = new()
{
BucketName = this.bucketName,
Key = key,
Key = this.GetKeyWithFolder(key),
ContentType = metadata.ContentType,
InputStream = stream,
AutoCloseStream = false
Expand Down Expand Up @@ -118,6 +123,9 @@ public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
return null;
}

private string GetKeyWithFolder(string key)
=> this.cacheFolder + key;

/// <summary>
/// <see href="https://github.com/aspnet/AspNetIdentity/blob/b7826741279450c58b230ece98bd04b4815beabf/src/Microsoft.AspNet.Identity.Core/AsyncHelper.cs"/>
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ public class AWSS3StorageCacheOptions : IAWSS3BucketClientOptions
/// <inheritdoc/>
public string BucketName { get; set; } = null!;

/// <summary>
/// Gets or sets the cache folder's name that'll store cache files under the configured bucket.
/// </summary>
public string CacheFolder { get; set; } = null!;
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want this to be nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually do. I changed this for the Azure class too.


/// <inheritdoc/>
public string? AccessKey { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ namespace SixLabors.ImageSharp.Web.Caching.Azure;
public class AzureBlobStorageCache : IImageCache
{
private readonly BlobContainerClient container;
private readonly string cacheFolder;

/// <summary>
/// Initializes a new instance of the <see cref="AzureBlobStorageCache"/> class.
Expand All @@ -27,12 +28,15 @@ public AzureBlobStorageCache(IOptions<AzureBlobStorageCacheOptions> cacheOptions
AzureBlobStorageCacheOptions options = cacheOptions.Value;

this.container = new BlobContainerClient(options.ConnectionString, options.ContainerName);
this.cacheFolder = string.IsNullOrEmpty(options.CacheFolder)
? string.Empty
: options.CacheFolder.Trim().Trim('/') + '/';
}

/// <inheritdoc/>
public async Task<IImageCacheResolver?> GetAsync(string key)
{
BlobClient blob = this.container.GetBlobClient(key);
BlobClient blob = this.container.GetBlobClient(this.GetBlobName(key));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should instead perhaps the whole right side be a method, like GetBlob()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored this.


if (!await blob.ExistsAsync())
{
Expand All @@ -45,7 +49,7 @@ public AzureBlobStorageCache(IOptions<AzureBlobStorageCacheOptions> cacheOptions
/// <inheritdoc/>
public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
{
BlobClient blob = this.container.GetBlobClient(key);
BlobClient blob = this.container.GetBlobClient(this.GetBlobName(key));

BlobHttpHeaders headers = new()
{
Expand Down Expand Up @@ -79,4 +83,7 @@ public static Response<BlobContainerInfo> CreateIfNotExists(
AzureBlobStorageCacheOptions options,
PublicAccessType accessType)
=> new BlobContainerClient(options.ConnectionString, options.ContainerName).CreateIfNotExists(accessType);

private string GetBlobName(string key)
=> this.cacheFolder + key;
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,15 @@ public class AzureBlobStorageCacheOptions

/// <summary>
/// Gets or sets the Azure Blob Storage container name.
/// Must conform to Azure Blob Storage container naming guidlines.
/// Must conform to Azure Blob Storage container naming guidelines.
/// <see href="https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#container-names"/>
/// </summary>
public string ContainerName { get; set; } = null!;

/// <summary>
/// Gets or sets the cache folder's name that'll store cache files under the configured container.
/// Must conform to Azure Blob Storage directory naming guidelines.
/// <see href="https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#directory-names"/>
/// </summary>
public string CacheFolder { get; set; } = null!;
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want this to be nullable?

}
Loading