Add missing fields to Transfer Utility request objects #4056
Conversation
There was a problem hiding this comment.
i didnt look too closely at the individual fields and compare them to the mapping.json file but this seems to be on the correct path.
Also i think once youre ready to publish the final PR, if its possible to easily do
- 1 PR for the Download Changes
- 1 PR for the Upload Changes
sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartUploadCommand.cs
Outdated
Show resolved
Hide resolved
philasmar
force-pushed
the
asmarp/add-missing-fields
branch
2 times, most recently
from
7b82718 to
63497b4
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request adds missing fields to Transfer Utility request objects in the AWS S3 SDK to align with the S3 specification. The changes refactor the inheritance structure by moving common properties from derived classes to base classes (BaseUploadRequest and BaseDownloadRequest), and ensure that all required fields are properly propagated through the various upload/download command implementations.
Key changes:
- Consolidated common upload properties into
BaseUploadRequestbase class - Added missing download-related properties to
BaseDownloadRequestandTransferUtilityDownloadDirectoryRequest - Updated internal command classes to properly map newly added fields
- Enhanced test infrastructure to validate request object completeness against S3 specifications
- Added comprehensive test coverage for all request conversion paths
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| BaseUploadRequest.cs | Added common upload properties (BucketName, ContentType, encryption settings, ACL, metadata, etc.) |
| BaseDownloadRequest.cs | Added ExpectedBucketOwner, IfMatch, IfNoneMatch, and ResponseHeaderOverrides properties |
| TransferUtilityUploadRequest.cs | Removed duplicate properties now inherited from BaseUploadRequest |
| TransferUtilityUploadDirectoryRequest.cs | Removed duplicate properties now inherited from BaseUploadRequest |
| TransferUtilityDownloadDirectoryRequest.cs | Added missing download-related properties |
| SimpleUploadCommand.cs | Updated to map newly added fields to PutObjectRequest |
| MultipartUploadCommand.cs | Updated to map fields across InitiateMultipartUpload, UploadPart, and CompleteMultipartUpload |
| UploadDirectoryCommand.cs | Updated to propagate fields from directory request to individual file requests |
| DownloadDirectoryCommand.cs | Updated to propagate fields to TransferUtilityDownloadRequest and ListObjects requests |
| BaseCommand.cs | Updated ConvertToGetObjectRequest to map new download fields |
| HeadersCollection.cs | Added ContentLanguage property |
| ResponseMapperTests.cs | Enhanced test infrastructure to support nested property aliases and validate request completeness |
| property-aliases.json | Extended with comprehensive property name mappings for all request types |
| 9d07dc1e-d82d-4f94-8700-c7b57f87205d.json | Added dev config for patch version bump |
philasmar
force-pushed
the
asmarp/add-missing-fields
branch
from
710ce0e to
7187dca
Compare
| "ServerSideEncryption": "ServerSideEncryptionMethod", | ||
| "SSEKMSEncryptionContext": "ServerSideEncryptionKeyManagementServiceEncryptionContext", | ||
| "Restore": "RestoreExpiration" | ||
| "PutObjectResponse": { |
There was a problem hiding this comment.
can you also update the mapping.json file in your PR (and rerun the tests to make sure we have everything)
There was a problem hiding this comment.
Will create a separate ticket to update the mapping.json once the change is merged. Also, a quick check showed that we need to do some updates on our end. Prefer to make that a separate task.
| } | ||
|
|
||
| private AbortMultipartUploadRequest ConstructAbortMultipartUploadRequest(MultipartUpload upload) | ||
| internal AbortMultipartUploadRequest ConstructAbortMultipartUploadRequest(MultipartUpload upload) |
There was a problem hiding this comment.
can you add a new SIM item/reminder to add a test case for this one. i see you have one for the other abort method but this one needs item too
|
Dry run passed |
3 participants
Description
Add missing fields for Upload and Download requests based on the S3 spec for the Transfer Utility.
Motivation and Context
Add missing fields for Upload and Download requests based on the S3 spec for the Transfer Utility.
Testing
Ran tests locally
Ran succesfull dry run
Screenshots (if appropriate)
Types of changes
Checklist
License