Skip to content

Commit d1c37cc

Browse files
v-pratapv-pratap
authored
feat(storage): migrate sync client to unified checksum options (#16264)
* feat(storage): migrate sync client to unified checksum options * Address review comments for PR #16264 * fix(storage): resolve check formatting and deprecation warnings in tests * fix: correctly resolve legacy checksum options defaults * fix formatting in checksum_helpers.h * address reviewer comments * fix(storage): resolve checksum logic flaws and format issues * refactor(storage): remove unused options.h include
1 parent f0c5fb2 commit d1c37cc

31 files changed

Lines changed: 534 additions & 188 deletions

‎google/cloud/storage/benchmarks/throughput_experiment.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
1516
#include "google/cloud/storage/benchmarks/throughput_experiment.h"
1617
#include "google/cloud/storage/benchmarks/benchmark_utils.h"
1718
#include "google/cloud/storage/client.h"
@@ -477,3 +478,4 @@ std::vector<std::unique_ptr<ThroughputExperiment>> CreateDownloadExperiments(
477478
} // namespace storage_benchmarks
478479
} // namespace cloud
479480
} // namespace google
481+
#include "google/cloud/internal/diagnostics_pop.inc"

‎google/cloud/storage/client.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
1516
#include "google/cloud/storage/client.h"
1617
#include "google/cloud/storage/idempotency_policy.h"
1718
#include "google/cloud/storage/internal/base64.h"
@@ -700,3 +701,5 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
700701
} // namespace storage
701702
} // namespace cloud
702703
} // namespace google
704+
705+
#include "google/cloud/internal/diagnostics_pop.inc"

‎google/cloud/storage/client_object_test.cc‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
1516
#include "google/cloud/storage/client.h"
17+
#include "google/cloud/storage/internal/checksum_helpers.h"
1618
#include "google/cloud/storage/internal/object_metadata_parser.h"
1719
#include "google/cloud/storage/retry_policy.h"
1820
#include "google/cloud/storage/testing/canonical_errors.h"
@@ -91,6 +93,32 @@ TEST_F(ObjectTest, InsertObjectMedia) {
9193
EXPECT_EQ(expected, *actual);
9294
}
9395

96+
TEST_F(ObjectTest, InsertObjectUploadChecksumMD5) {
97+
std::string text = R"""({
98+
"name": "test-bucket-name/test-object-name/1"
99+
})""";
100+
auto expected =
101+
storage::internal::ObjectMetadataParser::FromString(text).value();
102+
103+
EXPECT_CALL(*mock_, InsertObjectMedia)
104+
.WillOnce([&expected](internal::InsertObjectMediaRequest const& request) {
105+
EXPECT_EQ("test-bucket-name", request.bucket_name());
106+
EXPECT_EQ("test-object-name", request.object_name());
107+
EXPECT_EQ("test object contents", request.payload());
108+
EXPECT_TRUE(CurrentOptions().has<UploadChecksumValidationOption>());
109+
EXPECT_EQ(CurrentOptions().get<UploadChecksumValidationOption>(),
110+
ChecksumAlgorithm::kMD5);
111+
return make_status_or(expected);
112+
});
113+
114+
auto client = ClientForMock();
115+
auto actual = client.InsertObject(
116+
"test-bucket-name", "test-object-name", "test object contents",
117+
Options{}.set<UploadChecksumValidationOption>(ChecksumAlgorithm::kMD5));
118+
ASSERT_STATUS_OK(actual);
119+
EXPECT_EQ(expected, *actual);
120+
}
121+
94122
TEST_F(ObjectTest, GetObjectMetadata) {
95123
std::string text = R"""({
96124
"bucket": "test-bucket-name",
@@ -206,6 +234,120 @@ TEST_F(ObjectTest, ReadObject) {
206234
EXPECT_EQ(actual.gcount(), 1024);
207235
}
208236

237+
TEST_F(ObjectTest, ReadObjectChecksumPrecedence) {
238+
EXPECT_CALL(*mock_, ReadObject)
239+
.WillOnce([](internal::ReadObjectRangeRequest const& r) {
240+
EXPECT_TRUE(r.HasOption<DisableMD5Hash>());
241+
EXPECT_FALSE(r.GetOption<DisableMD5Hash>().value());
242+
243+
auto settings =
244+
internal::GetDownloadChecksumSettings(r, CurrentOptions());
245+
// Verify MD5 is enabled (disable_md5 = false) and CRC32C is disabled
246+
// (disable_crc32c = true)
247+
EXPECT_FALSE(settings.md5);
248+
EXPECT_TRUE(settings.crc32c);
249+
250+
auto read_source = std::make_unique<testing::MockObjectReadSource>();
251+
EXPECT_CALL(*read_source, IsOpen()).WillRepeatedly(Return(true));
252+
EXPECT_CALL(*read_source, Read)
253+
.WillOnce(Return(internal::ReadSourceResult{1024, {}}));
254+
EXPECT_CALL(*read_source, Close).Times(1);
255+
return StatusOr<std::unique_ptr<internal::ObjectReadSource>>(
256+
std::move(read_source));
257+
});
258+
auto client = ClientForMock();
259+
auto actual = client.ReadObject(
260+
"test-bucket-name", "test-object-name", DisableMD5Hash(false),
261+
Options{}.set<DownloadChecksumValidationOption>(
262+
ChecksumAlgorithm::kNone));
263+
ASSERT_STATUS_OK(actual.status());
264+
std::vector<char> v(1024);
265+
actual.read(v.data(), v.size());
266+
EXPECT_EQ(actual.gcount(), 1024);
267+
}
268+
269+
TEST_F(ObjectTest, ReadObjectChecksumPrecedenceDisableMD5) {
270+
EXPECT_CALL(*mock_, ReadObject)
271+
.WillOnce([](internal::ReadObjectRangeRequest const& r) {
272+
EXPECT_TRUE(r.HasOption<DisableMD5Hash>());
273+
EXPECT_TRUE(r.GetOption<DisableMD5Hash>().value());
274+
275+
auto settings =
276+
internal::GetDownloadChecksumSettings(r, CurrentOptions());
277+
// DisableMD5Hash(true) should override ChecksumAlgorithm::kMD5
278+
EXPECT_TRUE(settings.md5);
279+
EXPECT_TRUE(settings.crc32c); // kMD5 disables crc32c
280+
281+
auto read_source = std::make_unique<testing::MockObjectReadSource>();
282+
EXPECT_CALL(*read_source, IsOpen()).WillRepeatedly(Return(true));
283+
EXPECT_CALL(*read_source, Read)
284+
.WillOnce(Return(internal::ReadSourceResult{1024, {}}));
285+
EXPECT_CALL(*read_source, Close).Times(1);
286+
return StatusOr<std::unique_ptr<internal::ObjectReadSource>>(
287+
std::move(read_source));
288+
});
289+
auto client = ClientForMock();
290+
auto actual = client.ReadObject(
291+
"test-bucket-name", "test-object-name", DisableMD5Hash(true),
292+
Options{}.set<DownloadChecksumValidationOption>(ChecksumAlgorithm::kMD5));
293+
ASSERT_STATUS_OK(actual.status());
294+
std::vector<char> v(1024);
295+
actual.read(v.data(), v.size());
296+
EXPECT_EQ(actual.gcount(), 1024);
297+
}
298+
299+
TEST_F(ObjectTest, InsertObjectChecksumPrecedence) {
300+
EXPECT_CALL(*mock_, InsertObjectMedia)
301+
.WillOnce([](internal::InsertObjectMediaRequest const& r) {
302+
EXPECT_TRUE(r.HasOption<DisableCrc32cChecksum>());
303+
EXPECT_TRUE(r.GetOption<DisableCrc32cChecksum>().value());
304+
305+
auto settings =
306+
internal::GetUploadChecksumSettings(r, CurrentOptions());
307+
// Verify CRC32C is disabled (disable_crc32c = true) and MD5 remains
308+
// enabled (disable_md5 = false)
309+
EXPECT_TRUE(settings.crc32c);
310+
EXPECT_FALSE(settings.md5);
311+
312+
return make_status_or(
313+
storage::internal::ObjectMetadataParser::FromString(
314+
R"({"name": "test-object-name"})")
315+
.value());
316+
});
317+
auto client = ClientForMock();
318+
auto actual =
319+
client.InsertObject("test-bucket-name", "test-object-name", "payload",
320+
DisableCrc32cChecksum(true),
321+
Options{}.set<UploadChecksumValidationOption>(
322+
ChecksumAlgorithm::kCrc32cAndMD5));
323+
ASSERT_STATUS_OK(actual);
324+
}
325+
326+
TEST_F(ObjectTest, InsertObjectChecksumPrecedenceEnableCrc32c) {
327+
EXPECT_CALL(*mock_, InsertObjectMedia)
328+
.WillOnce([](internal::InsertObjectMediaRequest const& r) {
329+
EXPECT_TRUE(r.HasOption<DisableCrc32cChecksum>());
330+
EXPECT_FALSE(r.GetOption<DisableCrc32cChecksum>().value());
331+
332+
auto settings =
333+
internal::GetUploadChecksumSettings(r, CurrentOptions());
334+
// DisableCrc32cChecksum(false) should override ChecksumAlgorithm::kNone
335+
EXPECT_FALSE(settings.crc32c);
336+
EXPECT_TRUE(settings.md5); // kNone disables md5
337+
338+
return make_status_or(
339+
storage::internal::ObjectMetadataParser::FromString(
340+
R"({"name": "test-object-name"})")
341+
.value());
342+
});
343+
auto client = ClientForMock();
344+
auto actual = client.InsertObject(
345+
"test-bucket-name", "test-object-name", "payload",
346+
DisableCrc32cChecksum(false),
347+
Options{}.set<UploadChecksumValidationOption>(ChecksumAlgorithm::kNone));
348+
ASSERT_STATUS_OK(actual);
349+
}
350+
209351
TEST_F(ObjectTest, WriteObject) {
210352
EXPECT_CALL(*mock_, CreateResumableUpload)
211353
.WillOnce(Return(TransientError()))
@@ -483,3 +625,5 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
483625
} // namespace storage
484626
} // namespace cloud
485627
} // namespace google
628+
629+
#include "google/cloud/internal/diagnostics_pop.inc"

‎google/cloud/storage/google_cloud_cpp_storage.bzl‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ google_cloud_cpp_storage_hdrs = [
5252
"internal/bucket_metadata_cache.h",
5353
"internal/bucket_metadata_parser.h",
5454
"internal/bucket_requests.h",
55+
"internal/checksum_helpers.h",
5556
"internal/complex_option.h",
5657
"internal/compute_engine_util.h",
5758
"internal/connection_factory.h",

‎google/cloud/storage/google_cloud_cpp_storage.cmake‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ add_library(
8080
internal/bucket_metadata_parser.h
8181
internal/bucket_requests.cc
8282
internal/bucket_requests.h
83+
internal/checksum_helpers.h
8384
internal/complex_option.h
8485
internal/compute_engine_util.cc
8586
internal/compute_engine_util.h
@@ -424,6 +425,7 @@ if (BUILD_TESTING)
424425
internal/bucket_acl_requests_test.cc
425426
internal/bucket_metadata_cache_test.cc
426427
internal/bucket_requests_test.cc
428+
internal/checksum_helpers_test.cc
427429
internal/complex_option_test.cc
428430
internal/compute_engine_util_test.cc
429431
internal/connection_impl_bucket_acl_test.cc

‎google/cloud/storage/hashing_options.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ struct [[deprecated(
8383
using ComplexOption<DisableMD5Hash, bool>::ComplexOption;
8484
// GCC <= 7.0 does not use the inherited default constructor, redeclare it
8585
// explicitly
86-
DisableMD5Hash() : DisableMD5Hash(true) {}
86+
DisableMD5Hash() = default;
8787
static char const* name() { return "disable-md5-hash"; }
8888
};
8989

‎google/cloud/storage/hashing_options_test.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
1516
#include "google/cloud/storage/hashing_options.h"
1617
#include "google/cloud/storage/options.h"
1718
#include "google/cloud/options.h"
@@ -78,3 +79,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
7879
} // namespace storage
7980
} // namespace cloud
8081
} // namespace google
82+
#include "google/cloud/internal/diagnostics_pop.inc"

‎google/cloud/storage/internal/checksum_helpers.h‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// Copyright 2024 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_CHECKSUM_HELPERS_H
16+
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_CHECKSUM_HELPERS_H
17+
18+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
19+
#include "google/cloud/storage/hashing_options.h"
20+
#include "google/cloud/storage/options.h"
21+
#include "google/cloud/options.h"
22+
23+
namespace google {
24+
namespace cloud {
25+
namespace storage {
26+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
27+
namespace internal {
28+
29+
struct HashDisabled {
30+
bool md5;
31+
bool crc32c;
32+
};
33+
34+
template <typename Request>
35+
HashDisabled GetDownloadChecksumSettings(Request const& request,
36+
Options const& options) {
37+
bool disable_md5 = true;
38+
bool disable_crc32c = false;
39+
if (options.has<DownloadChecksumValidationOption>()) {
40+
auto const algo = options.get<DownloadChecksumValidationOption>();
41+
disable_md5 = (algo != ChecksumAlgorithm::kMD5 &&
42+
algo != ChecksumAlgorithm::kCrc32cAndMD5);
43+
disable_crc32c = (algo != ChecksumAlgorithm::kCrc32c &&
44+
algo != ChecksumAlgorithm::kCrc32cAndMD5);
45+
}
46+
47+
auto const md5 = request.template GetOption<DisableMD5Hash>();
48+
if (md5.has_value()) {
49+
disable_md5 = md5.value();
50+
}
51+
auto const crc32c = request.template GetOption<DisableCrc32cChecksum>();
52+
if (crc32c.has_value()) {
53+
disable_crc32c = crc32c.value();
54+
}
55+
return {disable_md5, disable_crc32c};
56+
}
57+
58+
template <typename Request>
59+
HashDisabled GetUploadChecksumSettings(Request const& request,
60+
Options const& options) {
61+
bool disable_md5 = true;
62+
bool disable_crc32c = false;
63+
if (options.has<UploadChecksumValidationOption>()) {
64+
auto const algo = options.get<UploadChecksumValidationOption>();
65+
disable_md5 = (algo != ChecksumAlgorithm::kMD5 &&
66+
algo != ChecksumAlgorithm::kCrc32cAndMD5);
67+
disable_crc32c = (algo != ChecksumAlgorithm::kCrc32c &&
68+
algo != ChecksumAlgorithm::kCrc32cAndMD5);
69+
}
70+
71+
auto const md5 = request.template GetOption<DisableMD5Hash>();
72+
if (md5.has_value()) {
73+
disable_md5 = md5.value();
74+
}
75+
auto const crc32c = request.template GetOption<DisableCrc32cChecksum>();
76+
if (crc32c.has_value()) {
77+
disable_crc32c = crc32c.value();
78+
}
79+
return {disable_md5, disable_crc32c};
80+
}
81+
82+
} // namespace internal
83+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
84+
} // namespace storage
85+
} // namespace cloud
86+
} // namespace google
87+
88+
#include "google/cloud/internal/diagnostics_pop.inc"
89+
#endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_CHECKSUM_HELPERS_H

0 commit comments

Comments
 (0)