Skip to content

Conversation

@GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty commented Oct 18, 2025

Stacked PRs:


Description

This change adds a function to map the CompletemultipartUploadResponse to TransferUtilityUploadResponse. This is needed because we eventually need to return TransferUtilityUploadResponse as part of the progress listener for multi part upload here #4061

Motivation and Context

  1. DOTNET-8276

Testing

  1. Unit tests
  2. 8338c01e-36d5-4617-b5af-b4a255e7c141 - pass

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

Copy link
Contributor

Copilot AI left a 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 adds mapping functionality to convert CompleteMultipartUploadResponse objects to TransferUtilityUploadResponse objects in the AWS S3 SDK. This enhances the transfer utility by providing a consistent response interface for multipart upload completions.

  • Added MapCompleteMultipartUploadResponse method in ResponseMapper class
  • Implemented comprehensive unit tests to validate the mapping functionality
  • Added dev config for patch version increment with appropriate changelog

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
sdk/src/Services/S3/Custom/Transfer/Internal/ResponseMapper.cs Added new mapping method to convert CompleteMultipartUploadResponse to TransferUtilityUploadResponse
sdk/test/Services/S3/UnitTests/Custom/ResponseMapperTests.cs Added comprehensive unit tests for the new mapping functionality including null handling scenarios
generator/.DevConfigs/433a9a6d-b8ea-4676-b763-70711e8288e6.json Added dev config for patch version increment

@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to gcbeatty/tuuploadresponse October 18, 2025 15:42
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from ff96820 to 838f273 Compare October 18, 2025 15:42
@GarrettBeatty GarrettBeatty changed the base branch from gcbeatty/tuuploadresponse to GarrettBeatty/stacked/1 October 18, 2025 15:42
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to gcbeatty/tuuploadresponse October 18, 2025 19:54
@GarrettBeatty GarrettBeatty changed the base branch from gcbeatty/tuuploadresponse to GarrettBeatty/stacked/1 October 18, 2025 19:54
@GarrettBeatty GarrettBeatty changed the title add completeuploadresponse mapping to turesponse object Added CompletemultipartResponse to TransferUtilityUploadResponse mapping Oct 18, 2025
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to gcbeatty/tuuploadresponse October 19, 2025 18:13
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from 838f273 to 48947aa Compare October 19, 2025 18:13
@GarrettBeatty GarrettBeatty changed the title Added CompletemultipartResponse to TransferUtilityUploadResponse mapping add completeuploadresponse mapping to turesponse object Oct 19, 2025
@GarrettBeatty GarrettBeatty changed the base branch from gcbeatty/tuuploadresponse to GarrettBeatty/stacked/1 October 19, 2025 18:13
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to gcbeatty/tuuploadresponse October 19, 2025 18:15
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from 48947aa to 01bb712 Compare October 19, 2025 18:15
@GarrettBeatty GarrettBeatty changed the base branch from gcbeatty/tuuploadresponse to GarrettBeatty/stacked/1 October 19, 2025 18:15
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to gcbeatty/tuuploadresponse October 20, 2025 13:38
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from 01bb712 to 8ed36be Compare October 20, 2025 13:38
@GarrettBeatty GarrettBeatty changed the base branch from gcbeatty/tuuploadresponse to GarrettBeatty/stacked/1 October 20, 2025 13:38
@GarrettBeatty GarrettBeatty changed the title add completeuploadresponse mapping to turesponse object This change adds a function to map the CompletemultipartUploadResponse to TransferUtilityUploadResponse Oct 20, 2025
@GarrettBeatty GarrettBeatty marked this pull request as ready for review October 20, 2025 13:41
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 20, 2025 14:06
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from 8ed36be to 7541f14 Compare October 20, 2025 14:06
@GarrettBeatty GarrettBeatty changed the title This change adds a function to map the CompletemultipartUploadResponse to TransferUtilityUploadResponse add completeuploadresponse mapping to turesponse object Oct 20, 2025
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/1 October 20, 2025 14:06
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 21, 2025 15:56
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from 7541f14 to e39e711 Compare October 21, 2025 15:57
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/1 October 21, 2025 15:57
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 21, 2025 20:51
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from e39e711 to c227963 Compare October 21, 2025 20:51
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/1 October 21, 2025 20:52
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 22, 2025 16:36
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from c227963 to ff1c70a Compare October 22, 2025 16:36
@GarrettBeatty GarrettBeatty changed the title add completeuploadresponse mapping to turesponse object ADd mapping of CompletemultipartUploadResponse to TransferUtilityUploadResponse Oct 22, 2025
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/1 October 22, 2025 16:37
@GarrettBeatty GarrettBeatty changed the title ADd mapping of CompletemultipartUploadResponse to TransferUtilityUploadResponse Add mapping of CompletemultipartUploadResponse to TransferUtilityUploadResponse Oct 22, 2025
var response = new TransferUtilityUploadResponse();

// Map all fields as defined in mapping.json "Conversion" -> "CompleteMultipartResponse" -> "UploadResponse"
if (source.IsSetBucketKeyEnabled())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so anytime a new property is added to CompleteMultipartUploadResponse we have to remember to update this mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't really have to validate mapping.json against s3 model per se. If s3 adds a new field, the new TM request/response objects will simply not have those new fields, which means customers will not be able to use them unless we manually update mappings.json and ask SDKs to update their TM implementation to add those new fields

this is the expectation from zoe right now. that its just a one time check we are currently doing. if we get asked to update the mapping.json file then we would need to make updates

"AbortMultipartUploadRequest": {
"Bucket": "BucketName"
},
"CompleteMultipartUploadResponse": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in mapping.json (existing file) we are looking for ServerSideEncryption and SSEKMSKeyId which have slightly different property names in .net compared to other SDKs so we need to add that mapping here

@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 23, 2025 01:26
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from ff1c70a to 6645f1c Compare October 23, 2025 01:27
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/1 October 23, 2025 01:27
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 23, 2025 16:01
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from 6645f1c to e6335e3 Compare October 23, 2025 16:02
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/1 October 23, 2025 16:02
…adResponse

stack-info: PR: #4060, branch: GarrettBeatty/stacked/2
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/1 to feature/transfermanager October 23, 2025 16:05
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/2 branch from e6335e3 to d78eddb Compare October 23, 2025 16:06
@GarrettBeatty GarrettBeatty merged commit 2263452 into feature/transfermanager Oct 23, 2025
1 check passed
@GarrettBeatty GarrettBeatty deleted the GarrettBeatty/stacked/2 branch October 23, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants