fix(storage): use hash values in InsertObject()#7025
Merged
coryan merged 2 commits intogoogleapis:mainfrom Jul 20, 2021
Merged
Conversation
If the application provides a `gcs::Crc32ChecksumValue()` or `gcs::MD5HashValue()` to `InsertObject()` we should use those values to create the metadata portion of the upload. Otherwise we are losing an opportunity to validate the data integrity during the upload. I discovered this while refactoring and restructuring the integration tests for CRC32C checksums and MD5 hashes. Apologies for the massive changes in the tests for what amounts to a small fix. The tests now have 3 portions, one for `InsertObject()`, one for `WriteObject()` and one for `ReadObject()`. Unlike before, the code for XML and JSON tests is shared, though this required some changes in the emulator to make it easier to detect what hash fields (if any) were including in the upload. For `InsertObject()` and `WriteObject()` we test 5 scenarios: (1) using the default settings, which are enabled for CRC32C, and disabled for MD5, (2) explicitly disabling the hash, (3) explicitly enabling the hash, (4) setting the hash to the correct value, and (5) setting the hash to the incorrect value. For `WriteObject()` we also test detecting an invalid hash, both in the case where the library computes the hash and the case where the application provides it. For `ReadObject()` it is simpler, because we cannot provide expected hashes. So we only test the cases where the hash check is in its default setting (enabled for CRC32, disabled for MD5) and that hash mistmatches are detected when the hash is enabled.
Contributor
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Contributor
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7025 +/- ##
=======================================
Coverage 94.47% 94.47%
=======================================
Files 1305 1305
Lines 113388 113350 -38
=======================================
- Hits 107119 107084 -35
+ Misses 6269 6266 -3
Continue to review full report at Codecov.
|
Contributor
Author
|
If you are reviewing this: you might be better off thinking of the integration tests as completely new code. |
devjgm
approved these changes
Jul 20, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If the application provides a
gcs::Crc32ChecksumValue()orgcs::MD5HashValue()toInsertObject()we should use those values tocreate the metadata portion of the upload. Otherwise we are losing an
opportunity to validate the data integrity during the upload.
I discovered this while refactoring and restructuring the integration
tests for CRC32C checksums and MD5 hashes. Apologies for the massive
changes in the tests for what amounts to a small fix.
The tests now have 3 portions, one for
InsertObject(), one forWriteObject()and one forReadObject(). Unlike before, the code forXML and JSON tests is shared, though this required some changes in the
emulator to make it easier to detect what hash fields (if any) were
including in the upload.
For
InsertObject()andWriteObject()we test 5 scenarios: (1) usingthe default settings, which are enabled for CRC32C, and disabled for
MD5, (2) explicitly disabling the hash, (3) explicitly enabling the
hash, (4) setting the hash to the correct value, and (5) setting the
hash to the incorrect value.
For
WriteObject()we also test detecting an invalid hash, both in thecase where the library computes the hash and the case where the
application provides it.
For
ReadObject()it is simpler, because we cannot provide expectedhashes. So we only test the cases where the hash check is in its default
setting (enabled for CRC32, disabled for MD5) and that hash mistmatches
are detected when the hash is enabled.
Fixes #7014
This change is