-
Notifications
You must be signed in to change notification settings - Fork 580
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
chore(middleware-flexible-checksums): use RequestChecksumCalculation and ResponseChecksumValidation #6492
base: main
Are you sure you want to change the base?
Conversation
The PR is ready for review. It's marked as draft, as it shouldn't be merged at this time. |
packages/middleware-flexible-checksums/src/flexibleChecksumsInputMiddleware.ts
Outdated
Show resolved
Hide resolved
The default checksum is changed from CRC32 to SHA256 based on benchmarks provided in #6499 |
28c1222
to
aad6e6f
Compare
The default checksum is switched back to CRC32 based on internal discussions with other SDK maintainers |
fe9e256
to
d81266b
Compare
40a8f9e
to
f9c40dd
Compare
f9c40dd
to
cc3e793
Compare
Feature flags were added in |
Code to skip checksum computation when user has set checksum header field was added in |
Behavior reverted in internal communications |
c808a7a
to
bcffb1b
Compare
Commit to set input[requestAlgorithmMember] to default checksum algorithm if it's not set and either requestChecksumCalculation is supported or requestChecksumRequired is set to true: |
Commit to remove the case from flexibleChecksumsMiddleware where the input[requestAlgorithmMember] is not set, as that case is now handled in flexibleChecksumsInputMiddleware: |
The tests are failing because the input is processed from |
dc5b5ae
to
263b09d
Compare
100+ integration tests were added in We also fixed an edge case to skip checksum if input[requestAlgorithmMember] is not set |
Reverted in |
To handle previous ask, the input[requestAlgorithmMember] (if added by SDK) was removed in |
const { requestChecksumRequired, requestAlgorithmMember } = middlewareConfig; | ||
|
||
if (hasHeaderWithPrefix("x-amz-checksum-", headers)) { | ||
// Remove input[requestAlgorithmMember] and header, if it was added by flexibleChecksumsInputMiddleware |
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.
this should state the condition more completely, since
Remove input[requestAlgorithmMember] and header, if it was added by flexibleChecksumsInputMiddleware
sounds like an unconditional removal
it should perhaps say "... and the user has set the checksum via a modeled field"?
This reverts commit ba24558. The ResponseChecksumValidation should be consumed in request middleware.
…equestValidationModeMember]
This reverts commit aad6e6f.
…hecksumValidation
This reverts commit 9204de2.
It sets input[requestAlgorithmMember] to default checksum algorithm if it's not set and either requestChecksumCalculation is supported or requestChecksumRequired is set to true.
Removes the case from flexibleChecksumsMiddleware where the input[requestAlgorithmMember] is not set, as that case is now checked in flexibleChecksumsInputMiddleware. Also removes check for isS3Express and it's default algorithm.
…ddleware This is because the request is not available pre-serialization. This reverts commit 969be81.
5f86077
to
9ebb747
Compare
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
Instead of populating input[requestAlgorithmMember] in InputMiddleware and attempting to remove it from main middleware, we switched to using newly introduced |
Issue
flexibleChecksumsInterceptorMiddleware
Description
flexibleChecksumsInterceptorMiddleware
to populateinput[requestValidationModeMember]
Testing
Unit testing in CI
Also, verified that checksums are computed by default
Output
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.