Avoid setting the checksum algorithm to NOT_SET because this is causi…

[thierryba]

…ng an error later on

Issue #, if available: #1961

*Description of changes: this change is about not setting a checksum algorithm if the transfer_config is set to NOT_SET
Not sure it is the best place to fix this.. Also note the strange code that was there for the putObjectRequest where the setter for the checksum algorithm was called 2x.

Check all that applies:

• Did a review by yourself.
• Added proper tests to cover this PR. (If tests are not applicable, explain.)
• Checked if this PR is a breaking (APIs have been changed) change.
• Checked if this PR will not introduce cross-platform inconsistent behavior.
• Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

• Linux
• Windows
• Android
• MacOS
• IOS
• Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sbiscigl]

Not sure it is the best place to fix this

Yeah taking a look at the issue, i would rather address the fact that ChecksumAlgorithm::NOT_SET doesn't work for the S3 client, rather than trying to work around it in transfer manager. Asked on that issue as well, im curious are you seeing this issue on AWS S3 or a query compatible service as i see you call out cloud flare h2 on the issue.

Also note the strange code that was there for the putObjectRequest where the setter for the checksum algorithm was called 2x

yeah likely a oversight on our end, for removing just that second call would be more than happy to merge that, or bundle it in with whatever fix comes out of this investigation.

[thierryba]

@sbiscigl agreed that iit is not the best place to fix this. But the s3client code is generated from what I understand. So how can we fix this?

[sbiscigl]

oh i can take a look at the s3 generated code, theres already a great deal of customization in the code generation process for s3. Editing the code generation process isn't very well documented or easy to understand, completely to our fault, we should make a dev guide for that but I digress, leave it to us.

Just trying to replicate the issue at this point though so i can verify a fix actually fixes it. do you experience the issue with AWS S3 or only with query compatible APIs like you were mentioning with cloudflare.

[thierryba]

@sbiscigl you are right in the sense that I was not able to repro with S3. Of course it would be nice to not set the x-amz-checksum-algorithm if the value is NOT_SET. It seems Cloudflare R2 does not support that header at all ;) and returns a message like "Unable to parse ExceptionName: NotImplemented Message: Header 'x-amz-checksum-algorithm' with value '' not implemented"

[sbiscigl]

NotImplemented Message: Header 'x-amz-checksum-algorithm' with value '' not implemented"

ah so they dont like empty string they want no header, I can take a look at trying to make it so that when the checksum is NOT_SET we just don't serialize that header, let me give that a shot

[thierryba]

@sbiscigl thank you very much.

[sbiscigl]

Alright cool so i created pull/2875 that wont serialize any enum headers that use the value NOT_SET unless they are defined in the service model. NOT_SET is something we define and results in a header without a value. This is against the smithy spec that says we should not be sending empty strings on the wire.

Let me know if that solves your issue in general. If you want to update this PR just to remove the extraneous set call in transfer manager we will accept it. Otherwise i think we can close this PR.

[thierryba]

@sbiscigl it does indeed solve my problem .Thank you very much.

[thierryba]

@sbiscigl I have made the change to remove the duplicate call to SetChecksumAlgorithm

[jmklix]

Closing as fix was merged here: #2875