-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
enhancement: flexible checksums #2999
base: master
Are you sure you want to change the base?
Conversation
3d43db0
to
22fb848
Compare
16cb321
to
c1b77ce
Compare
c1b77ce
to
0762584
Compare
a507de5
to
dce99fd
Compare
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.
Overall looks good. Just some minor changes. I also did not see a test for when generating a presigned url with PUT operations and the checksum here should be omitted?
dce99fd
to
ccae080
Compare
fef1fad
to
abe5426
Compare
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.
It looks great. Just two minor suggestion that are not required to address.
src/S3/CalculatesChecksumTrait.php
Outdated
throw new InvalidArgumentException( | ||
"Invalid checksum requested: {$requestedAlgorithm}." | ||
. " Valid algorithms are {$validAlgorithms}." | ||
); |
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.
There is another place that validates if the checksum is valid or not. For example here. I know one validates if is valid based on what the operation supports and the other one validates if is supported by the SDK in specific. Would not be worth to make that more clear here?, like state why it is not supported.
For example:
throw new InvalidArgumentException(
"Invalid checksum requested: {$requestedAlgorithm}."
. " Valid algorithms supported by this runtime are {$validAlgorithms}."
);
abe5426
to
30bd5fe
Compare
Description of changes:
Adds default checksum calculation, deprecates the
AddContentMD5
config option as S3 plans to deprecate MD5 checksums soon, adds two S3 client config options:request_checksum_calculation
andresponse_checksum_validation
with acceptable values for both being eitherwhen_supported
orwhen_required
.request_checksum_calculation
will default towhen_supported
, which means if an operation supports checksum calculation (indicated via modeling), the SDK will calculate the checksum and add the appropriate header. If it is set towhen_required
, the SDK will only compute and send a checksum when the operation requires anhttpRequestChecksum
.response_checksum_validation
also defaults towhen_supported
, which means if an operation models response algorithms, an attempt to validate a response checksum will be made.when_required
will only validate response checksums when response checksum are modeled ANDChecksumMode
is set toenabled
on the command.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.