-
Notifications
You must be signed in to change notification settings - Fork 874
Fix content language initialization and update to use headers backing field #4168
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
Fix content language initialization and update to use headers backing field #4168
Conversation
| { | ||
| return this._contentLanguage != null; | ||
| get { return this.Headers.ContentLanguage; } | ||
| set { this.Headers.ContentLanguage = value; } |
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.
the headers collection doesnt have a setter https://github.com/aws/aws-sdk-net/pull/4168/files#diff-7d76ec6d3c33088a35cba25bbc63006459be9754c861936319892b11e3072652L117
so i believe we should be fine here
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.
With Get-Only: Guaranteed Single Instance
When Headers is get-only, you're guaranteed that there is exactly one HeadersCollection instance throughout the lifetime of the GetObjectResponse object:
var response = new GetObjectResponse();
// First access - creates the HeadersCollection
response.ContentLanguage = "en-US";
// Internally: response.Headers.ContentLanguage = "en-US"
// ↓ This calls the getter, which creates headersCollection once
// Later access - uses THE SAME HeadersCollection
response.Headers.ContentType = "text/html";
// ↑ Returns the same headersCollection instance
// Even later
Console.WriteLine(response.ContentLanguage); // "en-US" ✓
// Still reading from the SAME headersCollection instance| if (responseData.IsHeaderPresent("Content-Encoding")) | ||
| response.Headers.ContentEncoding = S3Transforms.ToString(responseData.GetHeaderValue("Content-Encoding")); | ||
| if (responseData.IsHeaderPresent("Content-Language")) | ||
| response.ContentLanguage = S3Transforms.ToString(responseData.GetHeaderValue("Content-Language")); |
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 are still marshalling response.ContentLanguage somewhere though right?
no dev config since its already in #4074
Description
update the backing field for content language to be in the headers instead so that for TransferUtilityDownloadResponse we can easily just copy the headers only instead of the response.ContentLanguage field.
Also was a bug in #4074 where it was always setting the content language in one case because the
ifstatement didnt have brackets. So this fixes that bug and also makes it so the headers is the backing fieldMotivation and Context
Testing
Types of changes
Checklist
License