feat!: Flexible checksum v2 #1803
Conversation
… & AWSChecksumCalculationMode enum type for both config options.
…umValidation config options.
… to codegen side; add type info for AWSChecksumCalculationMode to codegen side.
… use this in setter codegen.
…ow and add in default algorithm selection logic.
…s user input as it's supposed to.
…m required flag to initializer.
…uest checksum calculation flow.
…nd responseChecksumValidation config is .whenRequired.
…shold, hence checksum header must be sent in original request rather than in trailing header.
| @@ -84,6 +85,40 @@ public class AWSClientConfigDefaultsProvider { | |||
| return resolvedAppID | |||
| } | |||
|
|
|||
| public static func requestChecksumCalculation( | |||
There was a problem hiding this comment.
would be good to add comment strings explaining what these functions do / how theyre used
| logger.info("Checksum validation should not be performed! Skipping workflow...") | ||
| return | ||
| } | ||
|
|
||
| let checksumHeaderIsPresent = priorityList.first { | ||
| response.headers.value(for: "x-amz-checksum-\($0)") != nil | ||
| response.headers.value(for: "x-amz-checksum-\($0)") != nil && | ||
| // Checksum of checksums has "-#" at the end and should be ignored. |
There was a problem hiding this comment.
is this a new requirement?
There was a problem hiding this comment.
Not necessarily a new requirement, it was just something that was left out from V1 SEP. S3 can return checksum of checksum, which ends with -# and should be ignored when validating response checksum.
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
|
|
||
| public enum AWSChecksumCalculationMode: String { |
There was a problem hiding this comment.
I like this idea of whenRequired vs whenSupported
There was a problem hiding this comment.
This addition to config is part of the V2 SEP 👍
There was a problem hiding this comment.
This looks good to me! I like the addition of whenRequired vs whenSupported and adding information on whether validations should be performed to the context. It also looks to me like this isn't a breaking a change so our integration tests should be pretty good.
Approving but please:
- Add comments around methods, especially public ones, where there are none right now
- An integration test that covers a flexible checksum request less than the size of 1 chunk if one doesn't exist already
Issue #
1516
Description of changes
New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.