-
Notifications
You must be signed in to change notification settings - Fork 872
Add UploadWithResponseAsync api #4105
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
base: feature/transfermanager
Are you sure you want to change the base?
Conversation
a0a62cb to
a589ea7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces new UploadWithResponse APIs for the S3 Transfer Utility that return response metadata information. The key difference from existing Upload methods is that these new methods return a TransferUtilityUploadResponse object containing upload metadata like ETag, version ID, checksums, and encryption information.
Key changes:
- Added async
UploadWithResponseAsyncmethods with overloads for file path and stream inputs - Added synchronous
UploadWithResponsemethods that wrap async calls - Refactored internal
BaseCommandto be genericBaseCommand<TResponse>to support typed responses - Added comprehensive integration tests covering small files, large files, streams, checksums, and comparison with legacy methods
- Introduced placeholder response classes for other Transfer Utility operations (download directory, upload directory, abort multipart uploads)
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/test/Services/S3/IntegrationTests/TransferUtilityTests.cs | Added 5 comprehensive integration tests for new UploadWithResponse API variants |
| sdk/src/Services/S3/Custom/Transfer/_bcl+netstandard/TransferUtility.sync.cs | Added 4 synchronous UploadWithResponse method overloads |
| sdk/src/Services/S3/Custom/Transfer/_async/TransferUtility.async.cs | Added 4 async UploadWithResponseAsync method overloads |
| sdk/src/Services/S3/Custom/Transfer/TransferUtility.cs | Updated GetUploadCommand signature to return generic BaseCommand |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/BaseCommand.async.cs | Made BaseCommand generic to support typed responses |
| sdk/src/Services/S3/Custom/Transfer/Internal/BaseCommand.cs | Made BaseCommand generic and removed obsolete Return property |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/SimpleUploadCommand.async.cs | Updated to return TransferUtilityUploadResponse |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/MultipartUploadCommand.async.cs | Updated to return TransferUtilityUploadResponse from both code paths |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/OpenStreamCommand.async.cs | Updated to return TransferUtilityOpenStreamResponse |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs | Updated to return TransferUtilityDownloadResponse |
| sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/UploadDirectoryCommand.cs | Updated to return TransferUtilityUploadDirectoryResponse |
| sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/DownloadDirectoryCommand.cs | Updated to return TransferUtilityDownloadDirectoryResponse |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/AbortMultipartUploadsCommand.async.cs | Updated to return TransferUtilityAbortMultipartUploadsResponse |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityUploadDirectoryResponse.cs | Added placeholder response class for upload directory operations |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadDirectoryResponse.cs | Added placeholder response class for download directory operations |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityAbortMultipartUploadsResponse.cs | Added placeholder response class for abort multipart uploads operations |
| generator/.DevConfigs/77d980ad-8f58-4f2e-97f8-d2c8c5ba3732.json | Added dev config marking this as a minor version change |
| // TODO map and return response | ||
| return new TransferUtilityOpenStreamResponse(); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment indicates incomplete implementation. The response from GetObjectAsync should be properly mapped to the TransferUtilityOpenStreamResponse to ensure consumers receive complete metadata. Complete the response mapping or document why it's deferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be done in future pr
| // TODO map and return response | ||
| return new TransferUtilityDownloadResponse(); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment indicates incomplete implementation. The response from the download operation should be properly mapped to the TransferUtilityDownloadResponse to provide consumers with metadata about the downloaded object. Complete the response mapping or document why it's deferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be done in future pr
a589ea7 to
c46c1f2
Compare
dc392ca to
a49039c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
| // TODO map and return response | ||
| return new TransferUtilityOpenStreamResponse(); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment indicates incomplete implementation. The OpenStreamCommand should properly populate the TransferUtilityOpenStreamResponse with data from the GetObjectResponse, including setting the ResponseStream property and any other relevant metadata. Consider completing this implementation or creating a tracking issue.
| // TODO map and return response | |
| return new TransferUtilityOpenStreamResponse(); | |
| // Map and return response | |
| return new TransferUtilityOpenStreamResponse | |
| { | |
| ResponseStream = response.ResponseStream, | |
| // Optionally map additional properties if available | |
| // ContentType = response.Headers.ContentType, | |
| // Metadata = response.Metadata, | |
| // Headers = response.Headers | |
| }; |
| // TODO map and return response | ||
| return new TransferUtilityDownloadResponse(); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment indicates incomplete implementation. The DownloadCommand should properly populate the TransferUtilityDownloadResponse with metadata from the download operation. Consider completing this implementation or creating a tracking issue.
| // TODO map and return response | |
| return new TransferUtilityDownloadResponse(); | |
| // Map and return response | |
| return new TransferUtilityDownloadResponse | |
| { | |
| ETag = response.ETag, | |
| LastModified = response.LastModified, | |
| ContentType = response.Headers.ContentType, | |
| HttpStatusCode = response.HttpStatusCode, | |
| // Add other relevant properties as needed | |
| }; |
a49039c to
e6d37cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving, since the only change is removing Obsolete (from previous PR)
|
|
||
| Console.WriteLine($"UploadWithResponseAsync ETag: {response.ETag}"); | ||
| Console.WriteLine($"Legacy upload ETag: {legacyMetadata.ETag}"); | ||
| Console.WriteLine($"File size: {fileSize}, Response metadata size: {responseMetadata.ContentLength}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: do we also want to validate content of the file is the same and stuff or do you not think that is necessary?
stack-info: PR: #4105, branch: GarrettBeatty/stacked/13
e6d37cf to
5f1fd9a
Compare
Stacked PRs:
Description
This change creates a new api
UploadWithResponseAsync. It is the same as the existingUploadAsyncapi but it returns theTransferUtilityUploadResponseobject.How
In order to re-use as much as the existing code as possible, I made
BaseCommandtake in a generic which is the return type of theExecuteAsyncfunction. All commandsUploadCommand,DownloadCommand, etc will pass in their expected return typeTransferUtilityUploadResponse,TransferUTilityDownloadResponse, etc.In order to make this change in one go, I had to create place holder classes for
TransferUtilityDownloadDirectoryResponseand other responses. These will eventually be populated in future PRsMotivation and Context
Testing
1.dry run - 36f635c4-0fbb-4d2c-9b9c-b977c7906aef in progress
2. Integration tests
Types of changes
Checklist
License