Skip to content

Add DownloadResponse mapping #4063

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add DownloadResponse mapping #4063

wants to merge 1 commit into from

Conversation

@GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty commented Oct 21, 2025 •
edited
Loading

Stacked PRs:

Description

This change adds a function to map the GetObjectResponse to TransferUtilityDownloadResponse.

Motivation and Context

  1. DOTNET-8280

Testing

  1. Unit tests

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

@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 4b5b83a to b28f801 Compare October 21, 2025 15:57
This was referenced Oct 21, 2025
Merged
Merged
Merged
Closed
Closed
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 21, 2025 20:51
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from b28f801 to a47780d Compare October 21, 2025 20:51
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 21, 2025 20:52
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 22, 2025 16:36
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from a47780d to 5bd3140 Compare October 22, 2025 16:36
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 22, 2025 16:37
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 23, 2025 01:26
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 5bd3140 to 20772b5 Compare October 23, 2025 01:27
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 23, 2025 01:27
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 23, 2025 16:01
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 20772b5 to 7b4b267 Compare October 23, 2025 16:02
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 23, 2025 16:02
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 23, 2025 16:06
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 7b4b267 to 19dcad3 Compare October 23, 2025 16:06
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 23, 2025 16:06
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 23, 2025 16:07
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 19dcad3 to 44c48e2 Compare October 23, 2025 16:07
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 23, 2025 16:07
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 23, 2025 16:24
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 44c48e2 to 1fbd586 Compare October 23, 2025 16:24
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/stacked/4 October 23, 2025 16:24
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/stacked/4 to feature/transfermanager October 23, 2025 17:00
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 1fbd586 to 79996ec Compare October 23, 2025 17:00
GarrettBeatty
GarrettBeatty commented Oct 23, 2025
View reviewed changes
...c/Services/S3/Custom/Model/Internal/MarshallTransformations/GetObjectResponseUnmarshaller.cs
response.ContentLanguage = S3Transforms.ToString(responseData.GetHeaderValue("Content-Language"));
if (responseData.IsHeaderPresent("Content-Length"))
response.Headers.ContentLength = long.Parse(responseData.GetHeaderValue("Content-Length"), CultureInfo.InvariantCulture);
response.Headers.ContentLanguage = S3Transforms.ToString(responseData.GetHeaderValue("Content-Language"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds this to the header to make it easy for TransferUtilityDownloadResponse, since we just copy source.header to target.header

GarrettBeatty
GarrettBeatty commented Oct 23, 2025
View reviewed changes
sdk/test/Services/S3/UnitTests/Custom/ResponseMapperTests.cs
// Resolve alias to actual property name
var resolvedPropertyName = ResolvePropertyName(propertyName, sourceType.Name);
var sourceProperty = sourceType.GetProperty(resolvedPropertyName);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this change because of the issue with contentlanguage. previously we were only setting the property and not the value in the headers. this makes it so we set both the property and the headers value in the source object

GarrettBeatty
GarrettBeatty commented Oct 23, 2025
View reviewed changes
sdk/src/Services/S3/Custom/Transfer/TransferUtilityDownloadResponse.cs
/// <summary>
/// The collection of headers for the response.
/// </summary>
public HeadersCollection Headers { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does it matter if this should be regular set or internal set?

@GarrettBeatty
Add DownloadResponse mapping
1efc02d 
stack-info: PR: #4063, branch: GarrettBeatty/stacked/5
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/stacked/5 branch from 79996ec to 1efc02d Compare October 23, 2025 17:27
@GarrettBeatty GarrettBeatty deleted the GarrettBeatty/stacked/5 branch October 23, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@GarrettBeatty