feat: add Goa.Clients.S3#76
Merged
Merged
Conversation
…ject) AOT-clean S3 client mirroring the SQS/Lambda client style: ErrorOr results, source-gen logging, XmlReader error parsing, builder + DI (AddS3). - Virtual-host and path-style (ForcePathStyle) addressing; keys encoded segment-by-segment preserving '/' - Reject object keys with empty/./.. segments or control chars, and validate bucket names, before URI construction (prevents Uri normalization from rerouting requests, e.g. DeleteObject -> DeleteBucket) - SSE-KMS headers, object metadata (validated HTTP-token names), Range GET - Error mapping: 404/NoSuchKey/NoSuchBucket -> NotFound, 403 -> Forbidden, bodyless HEAD 404, DeleteObject 204-always core: add opt-in HttpOptions.UseSingleUriEncoding so the signer uses the once-encoded canonical URI required by S3 (default double-encoding path and all existing clients unchanged); fix Host header to include non-default port to match the signed canonical host. Option keys made readonly. Tests: LocalStack integration (round-trips incl. percent-encoded/unicode/ zero-byte keys, range, missing-key, path-style) with signature validation enabled; AWS SDK signer comparison for the single-encoding path; key/bucket validation units. Core 22, S3 104.
Contributor
|
Warning Review limit reached
More reviews will be available in 1 hour, 12 minutes, and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements the ChangesGoa.Clients.S3 Implementation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant S3ServiceClient
participant S3RequestValidation
participant RequestSigner
participant S3
Caller->>S3ServiceClient: PutObjectAsync(request)
S3ServiceClient->>S3RequestValidation: ValidateBucketName(bucket)
S3ServiceClient->>S3RequestValidation: ValidateKey(key)
S3RequestValidation-->>S3ServiceClient: ErrorOr~Success~
S3ServiceClient->>S3ServiceClient: BuildObjectUri (virtual-host or path-style + EncodeKey)
S3ServiceClient->>S3ServiceClient: Set Content-Type, SSE headers, x-amz-meta-* headers
S3ServiceClient->>RequestSigner: SendAsync (UseSingleUriEncoding=true)
RequestSigner->>S3: SigV4-signed PUT request
S3-->>RequestSigner: HTTP 200 + ETag
RequestSigner-->>S3ServiceClient: HttpResponseMessage
alt 2xx
S3ServiceClient-->>Caller: ErrorOr~PutObjectResponse~ (ETag)
else non-2xx
S3ServiceClient->>S3ServiceClient: ReadErrorAsync (parse XML body, map 404/403)
S3ServiceClient-->>Caller: ErrorOr~Error~ (NotFound / Forbidden / Failure)
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/Clients/Goa.Clients.S3.Tests/S3KeyValidationTests.cs (1)
18-31: ⚡ Quick winDispose the root
ServiceProviderused to create the static client.
CreateClient()builds a container but never disposes it. In test suites this can leave handlers/providers alive and create avoidable resource leaks.Suggested cleanup
public class S3KeyValidationTests { private const string InvalidKeyErrorCode = "S3.InvalidKey"; + private static ServiceProvider? _serviceProvider; // Points at an unroutable endpoint - invalid keys must be rejected before any request is sent. private static readonly IS3Client Client = CreateClient(); private static IS3Client CreateClient() { var services = new ServiceCollection(); @@ - return services.BuildServiceProvider().GetRequiredService<IS3Client>(); + _serviceProvider = services.BuildServiceProvider(); + return _serviceProvider.GetRequiredService<IS3Client>(); } + + [OneTimeTearDown] + public void TearDown() + { + _serviceProvider?.Dispose(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Clients/Goa.Clients.S3.Tests/S3KeyValidationTests.cs` around lines 18 - 31, The CreateClient() method builds a ServiceProvider from the ServiceCollection but never disposes it, causing resource leaks in tests. Store the result of BuildServiceProvider() in a variable, wrap it in a using statement to ensure proper disposal, and retrieve the IS3Client service before the using block exits so the provider is cleaned up after the client is obtained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Clients/Goa.Clients.S3/Operations/PutObject/PutObjectBuilder.cs`:
- Around line 136-145: The Build() method in PutObjectBuilder is returning the
internal _metadata dictionary by reference, which allows reusing the builder to
mutate metadata on previously created requests. Change the Metadata assignment
from _metadata to create and return a copy of the dictionary (such as using new
Dictionary<> constructor or .ToDictionary()) to ensure each built
PutObjectRequest has its own independent copy and remains immutable from
subsequent builder calls.
In `@src/Clients/Goa.Clients.S3/README.md`:
- Around line 156-160: The code snippet in the README demonstrating error
handling with GetObjectAsync and ErrorType.NotFound is missing the required
using statement. Add "using ErrorOr;" at the beginning of the code snippet
before the var result = await _s3.GetObjectAsync line to ensure the code can
compile when copied and pasted by users.
In `@src/Clients/Goa.Clients.S3/S3ServiceClient.cs`:
- Around line 57-61: In the PutObjectAsync method, metadata validation is
missing before headers are added to requestMessage. Create a private validation
helper method (similar to the validation logic used in
PutObjectBuilder.AddMetadata) to validate metadata keys and values as valid HTTP
tokens. Before the foreach loop that iterates through request.Metadata, add
validation to check each entry's Key and Value against the same rules enforced
by the builder, throwing an appropriate exception if validation fails. This
ensures that invalid metadata cannot reach the TryAddWithoutValidation call,
regardless of whether PutObjectRequest is constructed through the builder or
used directly.
In `@src/Clients/Goa.Clients.S3/ServiceExtensions.cs`:
- Around line 44-50: The AddS3 method overload that accepts
Action<S3ServiceClientConfiguration> parameter allows configuration with only a
ServiceUrl without applying the region fallback logic that exists in another
overload, creating inconsistent behavior. Before the validation check at line 47
that ensures either region or service URL is provided, apply the same region
fallback logic used in the other AddS3 overload (referenced at Line 28) to
normalize the configuration and prevent request-sign failures when AWS_REGION
environment variable is unset.
In `@tests/Clients/Goa.Clients.S3.Tests/S3TestFixture.cs`:
- Around line 95-101: The bucket cleanup in the S3TestFixture teardown only
processes the first page of objects returned by ListObjectsV2Async, which means
subsequent pages are left behind. When DeleteBucketAsync is called on a
non-empty bucket, it fails and leaves cleanup warnings. Modify the cleanup logic
to handle pagination by checking the IsTruncated property on the
ListObjectsV2Response and continuously fetch subsequent pages using the
ContinuationToken until all objects are retrieved and deleted before calling
DeleteBucketAsync.
---
Nitpick comments:
In `@tests/Clients/Goa.Clients.S3.Tests/S3KeyValidationTests.cs`:
- Around line 18-31: The CreateClient() method builds a ServiceProvider from the
ServiceCollection but never disposes it, causing resource leaks in tests. Store
the result of BuildServiceProvider() in a variable, wrap it in a using statement
to ensure proper disposal, and retrieve the IS3Client service before the using
block exits so the provider is cleaned up after the client is obtained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6d68793-9283-410e-ab34-39bcb1f64db0
📒 Files selected for processing (32)
Directory.Packages.propsGoa.slnsrc/Clients/Goa.Clients.Core/Http/HttpOptions.cssrc/Clients/Goa.Clients.Core/Http/RequestSigner.cssrc/Clients/Goa.Clients.S3/Class1.cssrc/Clients/Goa.Clients.S3/Errors/S3ErrorCodes.cssrc/Clients/Goa.Clients.S3/Goa.Clients.S3.csprojsrc/Clients/Goa.Clients.S3/IS3Client.cssrc/Clients/Goa.Clients.S3/Log.cssrc/Clients/Goa.Clients.S3/Operations/DeleteObject/DeleteObjectRequest.cssrc/Clients/Goa.Clients.S3/Operations/DeleteObject/DeleteObjectResponse.cssrc/Clients/Goa.Clients.S3/Operations/GetObject/GetObjectRequest.cssrc/Clients/Goa.Clients.S3/Operations/GetObject/GetObjectResponse.cssrc/Clients/Goa.Clients.S3/Operations/HeadObject/HeadObjectRequest.cssrc/Clients/Goa.Clients.S3/Operations/HeadObject/HeadObjectResponse.cssrc/Clients/Goa.Clients.S3/Operations/PutObject/PutObjectBuilder.cssrc/Clients/Goa.Clients.S3/Operations/PutObject/PutObjectRequest.cssrc/Clients/Goa.Clients.S3/Operations/PutObject/PutObjectResponse.cssrc/Clients/Goa.Clients.S3/README.mdsrc/Clients/Goa.Clients.S3/S3RequestValidation.cssrc/Clients/Goa.Clients.S3/S3ServiceClient.cssrc/Clients/Goa.Clients.S3/S3ServiceClientConfiguration.cssrc/Clients/Goa.Clients.S3/ServiceExtensions.cstests/Clients/Goa.Clients.Core.Tests/Goa.Clients.Core.Tests.csprojtests/Clients/Goa.Clients.Core.Tests/Http/RequestSignerComparisonTests.cstests/Clients/Goa.Clients.S3.Tests/Goa.Clients.S3.Tests.csprojtests/Clients/Goa.Clients.S3.Tests/LocalStackFixture.cstests/Clients/Goa.Clients.S3.Tests/PutObjectBuilderTests.cstests/Clients/Goa.Clients.S3.Tests/S3ClientIntegrationTests.cstests/Clients/Goa.Clients.S3.Tests/S3KeyValidationTests.cstests/Clients/Goa.Clients.S3.Tests/S3RequestValidationTests.cstests/Clients/Goa.Clients.S3.Tests/S3TestFixture.cs
💤 Files with no reviewable changes (1)
- src/Clients/Goa.Clients.S3/Class1.cs
- PutObjectBuilder.Build() now copies metadata so built requests are independent of later builder mutations - Validate metadata HTTP-token names for directly-constructed PutObject requests via shared S3RequestValidation.ValidateMetadata (reused by builder) - AddS3(Action<>) overload applies AWS_REGION/us-east-1 fallback so SigV4 signing works when only a ServiceUrl is configured - README error-handling snippet includes `using ErrorOr;` - S3TestFixture cleanup paginates ListObjectsV2 before deleting the bucket - S3KeyValidationTests disposes its ServiceProvider via [After(Class)]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fills in the
Goa.Clients.S3stub with a working, AOT-clean S3 client: PutObject / GetObject / HeadObject / DeleteObject. Mirrors the SQS/Lambda client conventions (ErrorOr results, source-gen logging,XmlReadererror parsing, request builder,AddS3DI).Built to back a document-attachments feature in another service that streams files through its API, but the package is general-purpose.
Notable points
HttpOptions.UseSingleUriEncodingrequest option; the default double-encoding path and every existing client are untouched (verified by the existing AWS SDK comparison tests). Also fixed a latent Core bug where theHostheader omitted a non-default port while the signed canonical host included it (only bites on non-default ports, e.g. LocalStack)../..segments or control chars, and invalid bucket names, are rejected before URI construction. Without this,new Uri(...)normalizes dot-segments and can reroute a request (e.g. a..key turningDeleteObjectintoDeleteBucket).ForcePathStyle) addressing; SSE-KMS headers; validated object metadata;RangeGET; idempotent delete semantics.Tests
S3_SKIP_SIGNATURE_VALIDATION=0): round-trips including percent-encoded, unicode, and zero-byte keys, range GET, missing key, path-style. Note: the LocalStack community image doesn't strictly enforce SigV4, so signing correctness is additionally proven by an AWS SDKAWS4Signercomparison test for a percent-encoded path.🤖 Generated with Claude Code