-
Notifications
You must be signed in to change notification settings - Fork 872
Add UploadWithResponseAsync API #4062
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
Conversation
stack-info: PR: #4062, branch: GarrettBeatty/stacked/4
30b6e85 to
21a5f4f
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
Adds typed response support to S3 TransferUtility and introduces new UploadWithResponseAsync APIs that return upload metadata (e.g., ETag). Refactors internal command pipeline to a generic BaseCommand pattern and adds response types for upload/download/stream/directory/abort operations.
- Add UploadWithResponseAsync overloads on TransferUtility returning TransferUtilityUploadResponse
- Introduce BaseCommand and update internal commands to return typed responses
- Add response DTOs for directory/download/open stream/abort operations and update dev config to minor
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/src/Services/S3/Custom/Transfer/_async/TransferUtility.async.cs | Adds UploadWithResponseAsync overloads and wires typed command execution/return. |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityUploadDirectoryResponse.cs | Adds response DTO for upload directory (placeholder). |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityOpenStreamResponse.cs | Adds response DTO for open stream (currently empty). |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadResponse.cs | Adds response DTO for download (placeholder). |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadDirectoryResponse.cs | Adds response DTO for download directory (namespace differs from others). |
| sdk/src/Services/S3/Custom/Transfer/TransferUtilityAbortMultipartUploadsResponse.cs | Adds response DTO for abort multipart uploads (placeholder). |
| sdk/src/Services/S3/Custom/Transfer/TransferUtility.cs | Updates internal GetUploadCommand to typed BaseCommand. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/UploadDirectoryCommand.cs | Returns TransferUtilityUploadDirectoryResponse from ExecuteAsync. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_bcl+netstandard/DownloadDirectoryCommand.cs | Returns TransferUtilityDownloadDirectoryResponse from ExecuteAsync. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/SimpleUploadCommand.async.cs | Returns TransferUtilityUploadResponse and fires completed event. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/OpenStreamCommand.async.cs | Returns TransferUtilityOpenStreamResponse (currently not carrying stream). |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/MultipartUploadCommand.async.cs | Returns TransferUtilityUploadResponse; unseekable path returns mapped response. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/DownloadCommand.async.cs | Returns TransferUtilityDownloadResponse after download completes. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/BaseCommand.async.cs | Makes BaseCommand generic and updates ExecuteCommandAsync signature. |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/AbortMultipartUploadsCommand.async.cs | Returns TransferUtilityAbortMultipartUploadsResponse from ExecuteAsync. |
| sdk/src/Services/S3/Custom/Transfer/Internal/UploadDirectoryCommand.cs | Type aligns with BaseCommand. |
| sdk/src/Services/S3/Custom/Transfer/Internal/SimpleUploadCommand.cs | Type aligns with BaseCommand. |
| sdk/src/Services/S3/Custom/Transfer/Internal/OpenStreamCommand.cs | Type aligns with BaseCommand; removes Return override. |
| sdk/src/Services/S3/Custom/Transfer/Internal/MultipartUploadCommand.cs | Type aligns with BaseCommand. |
| sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs | Type aligns with BaseCommand. |
| sdk/src/Services/S3/Custom/Transfer/Internal/DownloadCommand.cs | Type aligns with BaseCommand. |
| sdk/src/Services/S3/Custom/Transfer/Internal/BaseCommand.cs | Converts BaseCommand to generic and removes Return property. |
| sdk/src/Services/S3/Custom/Transfer/Internal/AbortMultipartUploadsCommand.cs | Type aligns with BaseCommand. |
| generator/.DevConfigs/77d980ad-8f58-4f2e-97f8-d2c8c5ba3732.json | Adds dev config (minor) noting new UploadWithResponseAsync APIs and response-mapping enhancements. |
sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadDirectoryResponse.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/TransferUtilityOpenStreamResponse.cs
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/_async/OpenStreamCommand.async.cs
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/TransferUtilityOpenStreamResponse.cs
Show resolved
Hide resolved
stack-info: PR: #4062, branch: GarrettBeatty/stacked/4
21a5f4f to
e27d144
Compare
e27d144 to
c89de03
Compare
stack-info: PR: #4062, branch: GarrettBeatty/stacked/4
c89de03 to
8d603df
Compare
stack-info: PR: #4062, branch: GarrettBeatty/stacked/4
stack-info: PR: #4062, branch: GarrettBeatty/stacked/4
8d603df to
622b802
Compare
stack-info: PR: #4062, branch: GarrettBeatty/stacked/4
622b802 to
bb1413b
Compare
|
@philasmar this pr is still in draft but wanted to get your feedback on this approach. seems to be the easiest/best way to add this new api and re-use the existing functionality but using generics. i havent filled in the details of each |
| /// <typeparam name="TResponse">Type of response returned by the command</typeparam> | ||
| internal abstract partial class BaseCommand<TResponse> where TResponse : class | ||
| { | ||
| public virtual object Return |
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 is not being used anywhere so removing it to avoid confusion
a3dfaec to
d3860eb
Compare
| }; | ||
| InternalSDKUtils.ApplyValuesV2(request, additionalProperties); | ||
| return transfer.UploadAsync(request, cancellationToken); | ||
| return UploadObjectFromStreamInternalAsync(bucketName, objectKey, stream, additionalProperties, cancellationToken); |
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.
made a helper function here so that we could use the new uploadwithresponseasync api but then still have the same function signature of returning Task only (not Task<TransferUtilityUploadResponse>
| await WhenAllOrFirstExceptionAsync(pendingTasks,cancellationToken) | ||
| .ConfigureAwait(continueOnCapturedContext: false); | ||
|
|
||
| return new TransferUtilityAbortMultipartUploadsResponse(); |
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.
i dont think we plan on populating this at all FYI
d3860eb to
6d18ba4
Compare
| try | ||
| { | ||
| UploadAsync(filePath, bucketName).Wait(); | ||
| UploadWithResponseAsync(filePath, bucketName).Wait(); |
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.
actually i just realized this should say as UploadAsync because otherwise this would eventually create a span with UploadAsyncWithResponse instead of UploadAsync. will update that in next revision. same in other places below
6d18ba4 to
73b483f
Compare
73b483f to
60fdeae
Compare
| /// A cancellation token that can be used by other objects or threads to receive notice of cancellation. | ||
| /// </param> | ||
| /// <returns>The task object representing the asynchronous operation.</returns> | ||
| [Obsolete("Use UploadWithResponseAsync instead which allows you to upload files and also returns response metadata.")] |
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.
why do we need to mark these as obsolete?
| /// A cancellation token that can be used by other objects or threads to receive notice of cancellation. | ||
| /// </param> | ||
| /// <returns>The task object representing the asynchronous operation.</returns> | ||
| [Obsolete("Use UploadWithResponseAsync instead which allows you to upload files and also returns response metadata.")] |
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.
same question. the obsolete reason is not really convincing enough to mark the method as obsolete.
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.
we want to encourage users to use the new functions. we are also creating downloadwithresponse (Which will have multi part download) so having them use the "withResponse" functions of both upload/download makes sense to me
| /// A cancellation token that can be used by other objects or threads to receive notice of cancellation. | ||
| /// </param> | ||
| /// <returns>The task object representing the asynchronous operation.</returns> | ||
| [Obsolete("Use UploadWithResponseAsync instead which allows you to upload files and also returns response metadata.")] |
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.
same comment
| /// A cancellation token that can be used by other objects or threads to receive notice of cancellation. | ||
| /// </param> | ||
| /// <returns>The task object representing the asynchronous operation.</returns> | ||
| [Obsolete("Use UploadWithResponseAsync instead which allows you to upload files and also returns response metadata.")] |
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.
same comment
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.
we should remove the return null statements, everything else are just small nits
| } | ||
|
|
||
| /// <summary> | ||
| /// Uploads the specified file and returns response metadata including ETag, encryption details, and version information. |
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.
nit: spacing in the comments
|
|
||
| #region Internal Helper Methods | ||
|
|
||
| private async Task UploadObjectFromStreamInternalAsync(string bucketName, string objectKey, Stream stream, IDictionary<string, object> additionalProperties, CancellationToken cancellationToken) |
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.
nit: can we pass in TURequest object instead so the number of params goes down? best practice to not have too many params.
sdk/src/Services/S3/Custom/Transfer/_bcl+netstandard/TransferUtility.sync.cs
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/_bcl+netstandard/TransferUtility.sync.cs
Show resolved
Hide resolved
b5e77c5 to
ff5fbf0
Compare
|
opened this pr #4085 |
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 - d704b36b-7329-42dc-80e0-525a2f82bbcc pass
2. Integration tests
Types of changes
Checklist
License