refactor(amazon): Encapsulate S3 client with wrapper methods (#1636)#1790
refactor(amazon): Encapsulate S3 client with wrapper methods (#1636)#1790
Conversation
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
SummaryThis PR introduces an IS3 interface, refactors the S3 class with comprehensive wrapper methods and XML documentation, and updates IAmazonProvisioner to depend on the interface instead of the concrete class. Critical Issues1. Breaking Change: Client Property VisibilityLocation: src/ModularPipelines.AmazonWebServices/S3.cs The Client property is changed from public to protected. This is a breaking change for any external code that accesses S3.Client directly. While the codebase search shows no internal usages, downstream consumers may be affected. Recommendation: If this breaking change is intentional, it should be documented in the PR description and included in release notes as a breaking change. 2. Breaking Change: Method RenamesLocation: src/ModularPipelines.AmazonWebServices/S3.cs All existing public methods have been renamed (e.g., Bucket to CreateBucketAsync, BucketEncryption to PutBucketEncryptionAsync, etc.). This is another breaking change for existing consumers. 3. Missing Interface Implementation (Potential Bug)Location: src/ModularPipelines.AmazonWebServices/S3.cs The diff shows the CopyObjectAsync method implementation is truncated. Verify that the full S3.cs file compiles and all interface methods are properly implemented. Suggestions1. DI Registration OrderLocation: src/ModularPipelines.AmazonWebServices/Extensions/AmazonExtensions.cs:27-28 Both IS3 and S3 are registered. Consider whether the concrete S3 registration is needed, or add a comment explaining why both are registered. 2. Exception Handling PatternThe DoesBucketExistAsync and DoesObjectExistAsync methods use exception handling for control flow. Consider documenting the behavior for non-404 errors in XML comments. VerdictThe code quality is excellent with comprehensive interface design and documentation. However, the breaking changes to public API must be intentional and documented. Please confirm these breaking changes are intended and will be documented in release notes. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the S3 client wrapper to follow better encapsulation practices by hiding the underlying AWS SDK client and providing a comprehensive interface-based API. The changes address issue #1636 by preventing direct access to the internal AmazonS3Client, enabling future cross-cutting concerns like logging and metrics.
Key changes:
- Introduced
IS3interface with comprehensive S3 operation methods - Changed
AmazonS3Clientproperty from public to protected in theS3class - Added new wrapper methods for common operations with convenient overloads
- Maintained backward compatibility through obsolete methods marked for future removal
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ModularPipelines.AmazonWebServices/IS3.cs | New interface defining all S3 operations with comprehensive documentation |
| src/ModularPipelines.AmazonWebServices/S3.cs | Implements IS3 interface, makes Client protected, adds new wrapper methods, and maintains backward compatibility with obsolete methods |
| src/ModularPipelines.AmazonWebServices/IAmazonProvisioner.cs | Updated to return IS3 interface instead of concrete S3 type |
| src/ModularPipelines.AmazonWebServices/AmazonProvisioner.cs | Updated to inject and store IS3 interface instead of concrete S3 type |
| src/ModularPipelines.AmazonWebServices/Extensions/AmazonExtensions.cs | Registers both IS3 interface and S3 concrete implementation in DI container |
| /// Use this property only when the wrapper methods do not provide the required functionality. | ||
| /// Prefer using the wrapper methods for common operations. | ||
| /// </remarks> | ||
| protected AmazonS3Client Client { get; } |
There was a problem hiding this comment.
The Client property is marked as 'protected', which still allows derived classes to access it and potentially bypass the wrapper methods. Consider making it 'private' to fully encapsulate the client. If extensibility through inheritance is needed, provide protected virtual wrapper methods instead of exposing the client directly.
| public async Task<bool> DoesBucketExistAsync(string bucketName, CancellationToken cancellationToken = default) | ||
| { | ||
| try | ||
| { | ||
| await Client.GetBucketLocationAsync(bucketName, cancellationToken).ConfigureAwait(false); | ||
| return true; | ||
| } | ||
| catch (AmazonS3Exception ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The exception handling in DoesBucketExistAsync only catches 404 (NotFound) errors. Other exceptions like permission errors (403 Forbidden) or network issues will propagate as exceptions, making it inconsistent with the boolean return type semantics. Consider whether permission errors should also return false, or document that only existence checks are performed and other errors will throw.
| public async Task<bool> DoesObjectExistAsync(string bucketName, string key, CancellationToken cancellationToken = default) | ||
| { | ||
| try | ||
| { | ||
| await Client.GetObjectMetadataAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| return true; | ||
| } | ||
| catch (AmazonS3Exception ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The exception handling in DoesObjectExistAsync only catches 404 (NotFound) errors. Similar to DoesBucketExistAsync, other exceptions like permission errors (403 Forbidden) or network issues will propagate as exceptions. Consider whether permission errors should also return false, or document that only existence checks are performed and other errors will throw.
| var response = await Client.GetObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| await response.WriteResponseStreamToFileAsync(filePath, false, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
The DownloadToFileAsync method does not dispose of the GetObjectResponse. The response should be wrapped in a using statement or the response stream should be properly disposed to avoid resource leaks.
| var response = await Client.GetObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | |
| await response.WriteResponseStreamToFileAsync(filePath, false, cancellationToken).ConfigureAwait(false); | |
| using (var response = await Client.GetObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false)) | |
| { | |
| await response.WriteResponseStreamToFileAsync(filePath, false, cancellationToken).ConfigureAwait(false); | |
| } |
| [Obsolete("Use CreateBucketAsync instead. This method will be removed in a future version.")] | ||
| public async Task<PutBucketResponse> Bucket(PutBucketRequest request, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
The obsolete methods lack a specific removal version in the deprecation message. Consider adding a target version (e.g., "This method will be removed in version 3.0.0") to help consumers plan their migration.
| public async Task<PutBucketResponse> CreateBucketAsync(PutBucketRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.PutBucketAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| public async Task<PutBucketEncryptionResponse> BucketEncryption(PutBucketEncryptionRequest request, CancellationToken cancellationToken = default) | ||
| /// <inheritdoc /> | ||
| public async Task<DeleteBucketResponse> DeleteBucketAsync(string bucketName, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.DeleteBucketAsync(bucketName, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<DeleteBucketResponse> DeleteBucketAsync(DeleteBucketRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.DeleteBucketAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<ListBucketsResponse> ListBucketsAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.ListBucketsAsync(cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<bool> DoesBucketExistAsync(string bucketName, CancellationToken cancellationToken = default) | ||
| { | ||
| try | ||
| { | ||
| await Client.GetBucketLocationAsync(bucketName, cancellationToken).ConfigureAwait(false); | ||
| return true; | ||
| } | ||
| catch (AmazonS3Exception ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| #region Object Operations | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<PutObjectResponse> UploadObjectAsync(PutObjectRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.PutObjectAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<PutObjectResponse> UploadFileAsync(string bucketName, string key, string filePath, CancellationToken cancellationToken = default) | ||
| { | ||
| var request = new PutObjectRequest | ||
| { | ||
| BucketName = bucketName, | ||
| Key = key, | ||
| FilePath = filePath, | ||
| }; | ||
| return await Client.PutObjectAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<PutObjectResponse> UploadStreamAsync(string bucketName, string key, Stream inputStream, CancellationToken cancellationToken = default) | ||
| { | ||
| var request = new PutObjectRequest | ||
| { | ||
| BucketName = bucketName, | ||
| Key = key, | ||
| InputStream = inputStream, | ||
| }; | ||
| return await Client.PutObjectAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<GetObjectResponse> DownloadObjectAsync(GetObjectRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.GetObjectAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<GetObjectResponse> DownloadObjectAsync(string bucketName, string key, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.GetObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task DownloadToFileAsync(string bucketName, string key, string filePath, CancellationToken cancellationToken = default) | ||
| { | ||
| var response = await Client.GetObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| await response.WriteResponseStreamToFileAsync(filePath, false, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<DeleteObjectResponse> DeleteObjectAsync(string bucketName, string key, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.DeleteObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<DeleteObjectResponse> DeleteObjectAsync(DeleteObjectRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.DeleteObjectAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<DeleteObjectsResponse> DeleteObjectsAsync(DeleteObjectsRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.DeleteObjectsAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<ListObjectsV2Response> ListObjectsAsync(string bucketName, string? prefix = null, CancellationToken cancellationToken = default) | ||
| { | ||
| var request = new ListObjectsV2Request | ||
| { | ||
| BucketName = bucketName, | ||
| Prefix = prefix, | ||
| }; | ||
| return await Client.ListObjectsV2Async(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<ListObjectsV2Response> ListObjectsAsync(ListObjectsV2Request request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.ListObjectsV2Async(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<bool> DoesObjectExistAsync(string bucketName, string key, CancellationToken cancellationToken = default) | ||
| { | ||
| try | ||
| { | ||
| await Client.GetObjectMetadataAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| return true; | ||
| } | ||
| catch (AmazonS3Exception ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<GetObjectMetadataResponse> GetObjectMetadataAsync(string bucketName, string key, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.GetObjectMetadataAsync(bucketName, key, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<CopyObjectResponse> CopyObjectAsync(CopyObjectRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.CopyObjectAsync(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<CopyObjectResponse> CopyObjectAsync(string sourceBucket, string sourceKey, string destinationBucket, string destinationKey, CancellationToken cancellationToken = default) | ||
| { | ||
| return await Client.CopyObjectAsync(sourceBucket, sourceKey, destinationBucket, destinationKey, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| #endregion |
There was a problem hiding this comment.
The new wrapper methods for common S3 operations (CreateBucketAsync, UploadFileAsync, DownloadObjectAsync, etc.) lack test coverage. Consider adding unit tests to verify the interface implementation correctly delegates to the underlying AWS SDK client, especially for the convenience methods that construct request objects.
| services.TryAddScoped<IAmazon, AmazonWebServices.Amazon>(); | ||
| services.TryAddScoped<IAmazonProvisioner, AmazonProvisioner>(); | ||
| services.TryAddScoped<IS3, S3>(); | ||
| services.TryAddScoped<S3>(); |
There was a problem hiding this comment.
The dependency injection registration adds both IS3 and S3 to the service collection. This creates ambiguity - consumers could inject either IS3 or S3 (the concrete type). Consider registering only the interface (IS3) to enforce abstraction, or document why both registrations are necessary.
| services.TryAddScoped<S3>(); |
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
SummaryIntroduces an interface for the S3 class, renames methods to follow async naming conventions, and adds comprehensive S3 operations. Critical Issues1. Breaking Changes - Public API ModifiedThis PR contains breaking changes to the public API: Property Visibility Change:
Method Renames (Breaking):
Impact: All existing code using the S3 class will need updates. Since this appears to be a public library (NuGet package), this requires either:
2. Missing Method Implementations in DiffThe diff appears truncated. Line 190 in S3.cs shows: return await Cl
... [337 lines truncated] ...Cannot verify if all interface methods are properly implemented. The diff must be complete for review. Suggestions1. Consider Backward CompatibilityAdd obsolete methods alongside new ones to ease migration: [Obsolete("Use CreateBucketAsync instead")]
public async Task<PutBucketResponse> Bucket(PutBucketRequest request, CancellationToken cancellationToken = default)
=> await CreateBucketAsync(request, cancellationToken);2. Exception Handling PatternIn catch (AmazonS3Exception ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound)
{
return false;
}
// What about 403 Forbidden? Should that throw or return false?Current behavior: throws on permission errors. This might be the right choice, but should be intentional. Previous Review StatusUnable to retrieve previous comments due to API token scope limitations. Verdict |
- Add IS3 interface to define S3 operations contract - Make AmazonS3Client property protected instead of public - Add wrapper methods for common S3 operations: - Bucket operations: CreateBucketAsync, DeleteBucketAsync, ListBucketsAsync, DoesBucketExistAsync - Object operations: UploadObjectAsync, UploadFileAsync, UploadStreamAsync, DownloadObjectAsync, DownloadToFileAsync, DeleteObjectAsync, DeleteObjectsAsync, ListObjectsAsync, DoesObjectExistAsync, GetObjectMetadataAsync, CopyObjectAsync - Presigned URLs: GetPresignedUrl - Mark legacy methods as obsolete for backward compatibility - Update IAmazonProvisioner to use IS3 interface - Add comprehensive XML documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make Client property private instead of protected for full encapsulation - Add using statement for GetObjectResponse disposal in DownloadToFileAsync - Remove duplicate S3 concrete type registration from DI 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
thomhurst
force-pushed
the
fix/1636-aws-s3
branch
from
e108c23 to
9bd5f5d
Compare
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
SummaryThis PR refactors the S3 class by introducing an IS3 interface, making the underlying AmazonS3Client private, renaming methods to follow async naming conventions, adding comprehensive XML documentation, and providing backward compatibility through Critical Issues1. Breaking Change: Client Property Removed (Not Addressed)Location: src/ModularPipelines.AmazonWebServices/S3.cs:10 The Impact: Any external code accessing Recommendation: Add a deprecated public property: [Obsolete("Direct access to the underlying client is deprecated. Use the wrapper methods instead. This property will be removed in a future version.")]
public AmazonS3Client Client => _client;
private readonly AmazonS3Client _client;This gives consumers a migration path and warning, rather than immediate breakage. Suggestions1. Inconsistent Property Naming PatternLocation: src/ModularPipelines.AmazonWebServices/S3.cs:10 Using 2. Exception Handling DocumentationLocations:
The methods catch only /// <exception cref="AmazonS3Exception">Thrown when access is denied or other S3 errors occur (not including NotFound).</exception>Previous Review StatusBoth previous review comments raised the breaking change concern. This PR partially addresses it:
VerdictReasoning: While the method deprecations are well-handled, removing public API access without an |
Added public deprecated Client property to provide a migration path for existing code that directly accesses the underlying AmazonS3Client. The property is marked with [Obsolete] to warn consumers to migrate to the wrapper methods, while avoiding immediate breakage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
SummaryThis PR encapsulates the S3 client by making it internal and adding an Critical Issues1. Breaking change without proper versioning consideration Location: 2. Potential null reference issue with private field Location: 3. Missing disposal pattern Locations:
Suggestions
Previous Review StatusCannot access review comments due to GitHub token scope limitations, but based on the PR description this addresses issue #1636. Verdict |
- Make _client field readonly for safety - Implement IDisposable on S3 class to properly dispose AmazonS3Client - Extend IS3 interface with IDisposable - Add standard dispose pattern with GC.SuppressFinalize 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
SummaryRefactors the S3 client to use interface-based abstraction, making the underlying AmazonS3Client internal while exposing wrapper methods for S3 operations. Critical IssuesNone found ✅ Suggestions1. Missing Dispose implementation (S3.cs:13)The
Current code: public class S3 : IS3, IDisposable
{
private readonly AmazonS3Client _client;
private bool _disposed;
// No Dispose() method!Suggested fix: public void Dispose()
{
if (_disposed) return;
_client?.Dispose();
_disposed = true;
}2. Obsolete attribute on public Client property (S3.cs:30)The
This helps users plan their migration. 3. Resource leak in DownloadToFileAsync (S3.cs:130)The method properly uses using var response = await _client.GetObjectAsync(bucketName, key, cancellationToken).ConfigureAwait(false);
await response.WriteResponseStreamToFileAsync(filePath, false, cancellationToken).ConfigureAwait(false);This is actually correct - no issue here. ✅ Verdict✅ APPROVE - No critical issues The refactoring properly encapsulates the S3 client and provides a clean interface. The only suggestion is to complete the IDisposable implementation to avoid potential resource leaks. |
Summary
Fixes #1636
Test plan
🤖 Generated with Claude Code