Skip to content

Remove nullable disable from aws #312

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

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion shared-infrastructure
1 change: 0 additions & 1 deletion src/ImageSharp.Web.Providers.AWS/AmazonS3ClientFactory.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

using Amazon;
using Amazon.Runtime;
Expand Down
17 changes: 8 additions & 9 deletions src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

using System.Globalization;
using Amazon.S3;
Expand All @@ -17,7 +16,7 @@ namespace SixLabors.ImageSharp.Web.Caching.AWS;
public class AWSS3StorageCache : IImageCache
{
private readonly IAmazonS3 amazonS3Client;
private readonly string bucket;
private readonly string bucketName;

/// <summary>
/// Initializes a new instance of the <see cref="AWSS3StorageCache"/> class.
Expand All @@ -27,19 +26,19 @@ public AWSS3StorageCache(IOptions<AWSS3StorageCacheOptions> cacheOptions)
{
Guard.NotNull(cacheOptions, nameof(cacheOptions));
AWSS3StorageCacheOptions options = cacheOptions.Value;
this.bucket = options.BucketName;
this.bucketName = options.BucketName;
this.amazonS3Client = AmazonS3ClientFactory.CreateClient(options);
}

/// <inheritdoc/>
public async Task<IImageCacheResolver> GetAsync(string key)
public async Task<IImageCacheResolver?> GetAsync(string key)
{
GetObjectMetadataRequest request = new() { BucketName = this.bucket, Key = key };
GetObjectMetadataRequest request = new() { BucketName = this.bucketName, Key = key };
try
{
// HEAD request throws a 404 if not found.
MetadataCollection metadata = (await this.amazonS3Client.GetObjectMetadataAsync(request)).Metadata;
return new AWSS3StorageCacheResolver(this.amazonS3Client, this.bucket, key, metadata);
return new AWSS3StorageCacheResolver(this.amazonS3Client, this.bucketName, key, metadata);
}
catch
{
Expand All @@ -52,7 +51,7 @@ public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
{
PutObjectRequest request = new()
{
BucketName = this.bucket,
BucketName = this.bucketName,
Key = key,
ContentType = metadata.ContentType,
InputStream = stream,
Expand Down Expand Up @@ -82,12 +81,12 @@ public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata)
/// 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(
public static PutBucketResponse? CreateIfNotExists(
AWSS3StorageCacheOptions options,
S3CannedACL acl)
=> AsyncHelper.RunSync(() => CreateIfNotExistsAsync(options, acl));

private static async Task<PutBucketResponse> CreateIfNotExistsAsync(
private static async Task<PutBucketResponse?> CreateIfNotExistsAsync(
AWSS3StorageCacheOptions options,
S3CannedACL acl)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

namespace SixLabors.ImageSharp.Web.Caching.AWS;

Expand All @@ -10,19 +9,19 @@ namespace SixLabors.ImageSharp.Web.Caching.AWS;
public class AWSS3StorageCacheOptions : IAWSS3BucketClientOptions
{
/// <inheritdoc/>
public string Region { get; set; }
public string? Region { get; set; }

/// <inheritdoc/>
public string BucketName { get; set; }
public string BucketName { get; set; } = null!;

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

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

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

/// <inheritdoc/>
public bool UseAccelerateEndpoint { get; set; }
Expand Down
11 changes: 5 additions & 6 deletions src/ImageSharp.Web.Providers.AWS/IAWSS3BucketClientOptions.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

namespace SixLabors.ImageSharp.Web;

Expand All @@ -12,7 +11,7 @@ internal interface IAWSS3BucketClientOptions
/// <summary>
/// Gets or sets the AWS region endpoint (us-east-1/us-west-1/ap-southeast-2).
/// </summary>
string Region { get; set; }
string? Region { get; set; }

/// <summary>
/// Gets or sets the AWS bucket name.
Expand All @@ -24,20 +23,20 @@ internal interface IAWSS3BucketClientOptions
/// 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"/>.
/// </summary>
string AccessKey { get; set; }
string? AccessKey { get; set; }

/// <summary>
/// Gets or sets the AWS secret - Can be used to override keys provided by the environment.
/// 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"/>.
/// </summary>
string AccessSecret { get; set; }
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; }
string? Endpoint { get; set; }

/// <summary>
/// Gets or sets a value indicating whether the S3 accelerate endpoint is used.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

using Amazon.S3;
using Amazon.S3.Model;
Expand Down Expand Up @@ -29,7 +28,7 @@ private readonly Dictionary<string, AmazonS3Client> buckets
= new();

private readonly AWSS3StorageImageProviderOptions storageOptions;
private Func<HttpContext, bool> match;
private Func<HttpContext, bool>? match;

/// <summary>
/// Contains various helper methods based on the current configuration.
Expand Down Expand Up @@ -70,17 +69,23 @@ public bool IsValidRequest(HttpContext context)
=> this.formatUtilities.TryGetExtensionFromUri(context.Request.GetDisplayUrl(), out _);

/// <inheritdoc />
public async Task<IImageResolver> GetAsync(HttpContext context)
public async Task<IImageResolver?> GetAsync(HttpContext context)
{
// Strip the leading slash and bucket name from the HTTP request path and treat
// the remaining path string as the key.
// Path has already been correctly parsed before here.
string bucketName = string.Empty;
IAmazonS3 s3Client = null;
IAmazonS3? s3Client = null;

// We want an exact match here to ensure that bucket names starting with
// the same prefix are not mixed up.
string path = context.Request.Path.Value.TrimStart(SlashChars);
string? path = context.Request.Path.Value?.TrimStart(SlashChars);

if (path is null)
{
return null;
}

int index = path.IndexOfAny(SlashChars);
string nameToMatch = index != -1 ? path.Substring(0, index) : path;

Expand Down Expand Up @@ -121,7 +126,13 @@ private bool IsMatch(HttpContext context)
{
// Only match loosly here for performance.
// Path matching conflicts should be dealt with by configuration.
string path = context.Request.Path.Value.TrimStart(SlashChars);
string? path = context.Request.Path.Value?.TrimStart(SlashChars);

if (path is null)
{
return false;
}

foreach (string bucket in this.buckets.Keys)
{
if (path.StartsWith(bucket, StringComparison.OrdinalIgnoreCase))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

namespace SixLabors.ImageSharp.Web.Providers.AWS;

Expand All @@ -21,19 +20,19 @@ public class AWSS3StorageImageProviderOptions
public class AWSS3BucketClientOptions : IAWSS3BucketClientOptions
{
/// <inheritdoc/>
public string Region { get; set; }
public string? Region { get; set; }

/// <inheritdoc/>
public string BucketName { get; set; }
public string BucketName { 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.

Perhaps we should switch to get; init; for our options types to avoid these kinds of hacks. (We're .NET 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes no difference. Init does not mean that he must set it. It just says that it can not be changed after constructing.

For your desired behavior we need the required keyword and that is net7...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you're right (we keep having this conversation 🤣)

I do thoink for .NET 6 though, options-types should use this pattern to enforce immutability.


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

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

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

/// <inheritdoc/>
public bool UseAccelerateEndpoint { get; set; }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

using Amazon.S3;
using Amazon.S3.Model;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
#nullable disable

using System.Net.Http.Headers;
using Amazon.S3;
Expand All @@ -16,7 +15,7 @@ public class AWSS3StorageImageResolver : IImageResolver
private readonly IAmazonS3 amazonS3;
private readonly string bucketName;
private readonly string imagePath;
private readonly GetObjectMetadataResponse metadataResponse;
private readonly GetObjectMetadataResponse? metadataResponse;

/// <summary>
/// Initializes a new instance of the <see cref="AWSS3StorageImageResolver"/> class.
Expand All @@ -25,7 +24,7 @@ public class AWSS3StorageImageResolver : IImageResolver
/// <param name="bucketName">The bucket name.</param>
/// <param name="imagePath">The image path.</param>
/// <param name="metadataResponse">Optional metadata response.</param>
public AWSS3StorageImageResolver(IAmazonS3 amazonS3, string bucketName, string imagePath, GetObjectMetadataResponse metadataResponse = null)
public AWSS3StorageImageResolver(IAmazonS3 amazonS3, string bucketName, string imagePath, GetObjectMetadataResponse? metadataResponse = null)
{
this.amazonS3 = amazonS3;
this.bucketName = bucketName;
Expand All @@ -41,7 +40,7 @@ public async Task<ImageMetadata> GetMetaDataAsync()
// Try to parse the max age from the source. If it's not zero then we pass it along
// to set the cache control headers for the response.
TimeSpan maxAge = TimeSpan.MinValue;
if (CacheControlHeaderValue.TryParse(metadata.Headers.CacheControl, out CacheControlHeaderValue cacheControl))
if (CacheControlHeaderValue.TryParse(metadata.Headers.CacheControl, out CacheControlHeaderValue? cacheControl))
{
// Weirdly passing null to TryParse returns true.
if (cacheControl?.MaxAge.HasValue == true)
Expand Down