Skip to content

Commit 21f3d91

Browse files
coryancoryan
authored
cleanup(storage): deprecate overly permissive functions (#6258)
`storage::Client` has a number of functions that expose more details of the implementation than they should, or are only intended for tests, benchmarks, or to implement additional functionality in the library. This change marks these functions as deprecated, and recommends what to do instead. We also changed our tests, examples, and implementation to use a helper in `google::cloud::internal` to access this functionality.
1 parent 64cea5e commit 21f3d91

17 files changed

+158
-102
lines changed

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,9 @@ TEST_P(ThroughputExperimentIntegrationTest, Upload) {
5151
options.minimum_write_size = 1 * kMiB;
5252
options.enabled_apis = {GetParam()};
5353

54-
auto const& client_options = client->raw_client()->client_options();
54+
auto const& client_options =
55+
google::cloud::storage::internal::ClientImplDetails::GetRawClient(*client)
56+
->client_options();
5557
auto experiments =
5658
CreateUploadExperiments(options, client_options, /*thread_id=*/0);
5759
for (auto& e : experiments) {
@@ -81,7 +83,9 @@ TEST_P(ThroughputExperimentIntegrationTest, Download) {
8183
options.minimum_write_size = 1 * kMiB;
8284
options.enabled_apis = {GetParam()};
8385

84-
auto const& client_options = client->raw_client()->client_options();
86+
auto const& client_options =
87+
google::cloud::storage::internal::ClientImplDetails::GetRawClient(*client)
88+
->client_options();
8589
auto experiments =
8690
CreateDownloadExperiments(options, client_options, /*thread_id=*/0);
8791
for (auto& e : experiments) {

‎google/cloud/storage/client.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ bool Client::UseSimpleUpload(std::string const& file_name,
9898
return false;
9999
}
100100
auto const fs = google::cloud::internal::file_size(file_name);
101-
if (fs <= raw_client()->client_options().maximum_simple_upload_size()) {
101+
if (fs <= raw_client_->client_options().maximum_simple_upload_size()) {
102102
size = static_cast<std::size_t>(fs);
103103
return true;
104104
}
@@ -198,7 +198,7 @@ integrity checks using the DisableMD5Hash() and DisableCrc32cChecksum() options.
198198
StatusOr<ObjectMetadata> Client::UploadStreamResumable(
199199
std::istream& source, internal::ResumableUploadRequest const& request) {
200200
StatusOr<std::unique_ptr<internal::ResumableUploadSession>> session_status =
201-
raw_client()->CreateResumableSession(request);
201+
raw_client_->CreateResumableSession(request);
202202
if (!session_status) {
203203
return std::move(session_status).status();
204204
}
@@ -220,7 +220,7 @@ StatusOr<ObjectMetadata> Client::UploadStreamResumable(
220220

221221
// GCS requires chunks to be a multiple of 256KiB.
222222
auto chunk_size = internal::UploadChunkRequest::RoundUpToQuantum(
223-
raw_client()->client_options().upload_buffer_size());
223+
raw_client_->client_options().upload_buffer_size());
224224

225225
StatusOr<internal::ResumableUploadResponse> upload_response(
226226
internal::ResumableUploadResponse{});
@@ -320,12 +320,12 @@ std::string Client::SigningEmail(SigningAccount const& signing_account) {
320320
if (signing_account.has_value()) {
321321
return signing_account.value();
322322
}
323-
return raw_client()->client_options().credentials()->AccountEmail();
323+
return raw_client_->client_options().credentials()->AccountEmail();
324324
}
325325

326326
StatusOr<Client::SignBlobResponseRaw> Client::SignBlobImpl(
327327
SigningAccount const& signing_account, std::string const& string_to_sign) {
328-
auto credentials = raw_client()->client_options().credentials();
328+
auto credentials = raw_client_->client_options().credentials();
329329

330330
std::string signing_account_email = SigningEmail(signing_account);
331331
// First try to sign locally.
@@ -339,7 +339,7 @@ StatusOr<Client::SignBlobResponseRaw> Client::SignBlobImpl(
339339
// credentials account. In either case, try to sign using the API.
340340
internal::SignBlobRequest sign_request(
341341
signing_account_email, internal::Base64Encode(string_to_sign), {});
342-
auto response = raw_client()->SignBlob(sign_request);
342+
auto response = raw_client_->SignBlob(sign_request);
343343
if (!response) {
344344
return response.status();
345345
}

‎google/cloud/storage/client.h‎

Lines changed: 62 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,11 @@
4545
namespace google {
4646
namespace cloud {
4747
namespace storage {
48-
namespace testing {
49-
class ClientTester;
50-
} // namespace testing
5148
inline namespace STORAGE_CLIENT_NS {
5249
namespace internal {
5350
class NonResumableParallelUploadState;
5451
class ResumableParallelUploadState;
52+
struct ClientImplDetails;
5553
} // namespace internal
5654
/**
5755
* The Google Cloud Storage (GCS) Client.
@@ -66,7 +64,7 @@ class ResumableParallelUploadState;
6664
* comparable to copying a few shared pointers. The first request (or any
6765
* request that requires a new connection) incurs the cost of creating the
6866
* connection and authenticating with the service. Note that the library may
69-
* need to perform other bookeeping operations that may impact performance.
67+
* need to perform other bookkeeping operations that may impact performance.
7068
* For example, access tokens need to be refreshed from time to time, and this
7169
* may impact the performance of some operations.
7270
*
@@ -227,7 +225,7 @@ class Client {
227225
*/
228226
template <typename... Policies>
229227
explicit Client(ClientOptions options, Policies&&... policies)
230-
: Client(CreateDefaultInternalClient(std::move(options)),
228+
: Client(InternalOnly{}, CreateDefaultInternalClient(std::move(options)),
231229
std::forward<Policies>(policies)...) {}
232230

233231
/**
@@ -251,25 +249,48 @@ class Client {
251249
: Client(ClientOptions(std::move(credentials)),
252250
std::forward<Policies>(policies)...) {}
253251

252+
/// Create a Client using ClientOptions::CreateDefaultClientOptions().
253+
static StatusOr<Client> CreateDefaultClient();
254+
254255
/// Builds a client and maybe override the retry, idempotency, and/or backoff
255256
/// policies.
257+
/// @deprecated This was intended only for test code, applications should not
258+
/// use it.
256259
template <typename... Policies>
260+
#if !defined(_MSC_VER) || _MSC_VER >= 1920
261+
GOOGLE_CLOUD_CPP_DEPRECATED(
262+
"applications should not need this."
263+
" Please use the constructors from ClientOptions instead."
264+
" For mocking, please use testing::ClientFromMock() instead."
265+
" Please file a bug at https://github.com/googleapis/google-cloud-cpp"
266+
" if you have a use-case not covered by these.")
267+
#endif // _MSC_VER
268+
// NOLINTNEXTLINE(performance-unnecessary-value-param)
257269
explicit Client(std::shared_ptr<internal::RawClient> client,
258270
Policies&&... policies)
259-
: raw_client_(
260-
Decorate(std::move(client), std::forward<Policies>(policies)...)) {}
271+
: Client(InternalOnly{}, std::move(client),
272+
std::forward<Policies>(policies)...) {
273+
}
261274

262275
/// Define a tag to disable automatic decorations of the RawClient.
263276
struct NoDecorations {};
264277

265278
/// Builds a client with a specific RawClient, without decorations.
279+
/// @deprecated This was intended only for test code, applications should not
280+
/// use it.
281+
GOOGLE_CLOUD_CPP_DEPRECATED(
282+
"applications should not need this."
283+
" Please file a bug at https://github.com/googleapis/google-cloud-cpp"
284+
" if you do.")
266285
explicit Client(std::shared_ptr<internal::RawClient> client, NoDecorations)
267-
: raw_client_(std::move(client)) {}
268-
269-
/// Create a Client using ClientOptions::CreateDefaultClientOptions().
270-
static StatusOr<Client> CreateDefaultClient();
286+
: Client(InternalOnlyNoDecorations{}, std::move(client)) {}
271287

272288
/// Access the underlying `RawClient`.
289+
/// @deprecated Only intended for implementors, do not use.
290+
GOOGLE_CLOUD_CPP_DEPRECATED(
291+
"applications should not need this."
292+
" Please file a bug at https://github.com/googleapis/google-cloud-cpp"
293+
" if you do.")
273294
std::shared_ptr<internal::RawClient> raw_client() const {
274295
return raw_client_;
275296
}
@@ -3103,7 +3124,17 @@ class Client {
31033124
//@}
31043125

31053126
private:
3127+
friend internal::ClientImplDetails;
3128+
31063129
Client() = default;
3130+
struct InternalOnly {};
3131+
struct InternalOnlyNoDecorations {};
3132+
3133+
template <typename... Policies>
3134+
Client(InternalOnly, std::shared_ptr<internal::RawClient> c, Policies&&... p)
3135+
: raw_client_(Decorate(std::move(c), std::forward<Policies>(p)...)) {}
3136+
Client(InternalOnlyNoDecorations, std::shared_ptr<internal::RawClient> c)
3137+
: raw_client_(std::move(c)) {}
31073138
static std::shared_ptr<internal::RawClient> CreateDefaultInternalClient(
31083139
ClientOptions options);
31093140

@@ -3199,7 +3230,6 @@ class Client {
31993230

32003231
friend class internal::NonResumableParallelUploadState;
32013232
friend class internal::ResumableParallelUploadState;
3202-
friend class testing::ClientTester;
32033233
};
32043234

32053235
/**
@@ -3221,6 +3251,26 @@ class Client {
32213251
std::string CreateRandomPrefixName(std::string const& prefix = "");
32223252

32233253
namespace internal {
3254+
struct ClientImplDetails {
3255+
static std::shared_ptr<RawClient> GetRawClient(Client& c) {
3256+
return c.raw_client_;
3257+
}
3258+
static StatusOr<ObjectMetadata> UploadStreamResumable(
3259+
Client& client, std::istream& source,
3260+
internal::ResumableUploadRequest const& request) {
3261+
return client.UploadStreamResumable(source, request);
3262+
}
3263+
template <typename... Policies>
3264+
static Client CreateClient(std::shared_ptr<internal::RawClient> c,
3265+
Policies&&... p) {
3266+
return Client(Client::InternalOnly{}, std::move(c),
3267+
std::forward<Policies>(p)...);
3268+
}
3269+
static Client CreateWithoutDecorations(
3270+
std::shared_ptr<internal::RawClient> c) {
3271+
return Client(Client::InternalOnlyNoDecorations{}, std::move(c));
3272+
}
3273+
};
32243274

32253275
// Just a wrapper to allow for using in `google::cloud::internal::apply`.
32263276
struct DeleteApplyHelper {

‎google/cloud/storage/client_test.cc‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ namespace storage {
2626
inline namespace STORAGE_CLIENT_NS {
2727
namespace {
2828

29+
using ::google::cloud::storage::internal::ClientImplDetails;
2930
using ::google::cloud::storage::testing::canonical_errors::TransientError;
3031
using ::testing::_;
3132
using ::testing::Return;
@@ -86,8 +87,8 @@ class ClientTest : public ::testing::Test {
8687
TEST_F(ClientTest, OverrideRetryPolicy) {
8788
auto const mock_options = ClientOptions(oauth2::CreateAnonymousCredentials());
8889
EXPECT_CALL(*mock_, client_options()).WillRepeatedly(ReturnRef(mock_options));
89-
Client client{std::shared_ptr<internal::RawClient>(mock_),
90-
ObservableRetryPolicy(3)};
90+
auto client = internal::ClientImplDetails::CreateClient(
91+
std::shared_ptr<internal::RawClient>(mock_), ObservableRetryPolicy(3));
9192

9293
// Reset the counters at the beginning of the test.
9394

@@ -105,8 +106,9 @@ TEST_F(ClientTest, OverrideBackoffPolicy) {
105106
using ms = std::chrono::milliseconds;
106107
auto const mock_options = ClientOptions(oauth2::CreateAnonymousCredentials());
107108
EXPECT_CALL(*mock_, client_options()).WillRepeatedly(ReturnRef(mock_options));
108-
Client client{std::shared_ptr<internal::RawClient>(mock_),
109-
ObservableBackoffPolicy(ms(20), ms(100), 2.0)};
109+
auto client = internal::ClientImplDetails::CreateClient(
110+
std::shared_ptr<internal::RawClient>(mock_),
111+
ObservableBackoffPolicy(ms(20), ms(100), 2.0));
110112

111113
// Call an API (any API) on the client, we do not care about the status, just
112114
// that our policy is called.
@@ -122,9 +124,9 @@ TEST_F(ClientTest, OverrideBothPolicies) {
122124
using ms = std::chrono::milliseconds;
123125
auto const mock_options = ClientOptions(oauth2::CreateAnonymousCredentials());
124126
EXPECT_CALL(*mock_, client_options()).WillRepeatedly(ReturnRef(mock_options));
125-
Client client{std::shared_ptr<internal::RawClient>(mock_),
126-
ObservableBackoffPolicy(ms(20), ms(100), 2.0),
127-
ObservableRetryPolicy(3)};
127+
auto client = internal::ClientImplDetails::CreateClient(
128+
std::shared_ptr<internal::RawClient>(mock_),
129+
ObservableBackoffPolicy(ms(20), ms(100), 2.0), ObservableRetryPolicy(3));
128130

129131
// Call an API (any API) on the client, we do not care about the status, just
130132
// that our policy is called.
@@ -143,8 +145,9 @@ TEST_F(ClientTest, DefaultDecorators) {
143145
ClientOptions options(oauth2::CreateAnonymousCredentials());
144146
Client tested(options);
145147

146-
EXPECT_TRUE(tested.raw_client() != nullptr);
147-
auto* retry = dynamic_cast<internal::RetryClient*>(tested.raw_client().get());
148+
EXPECT_TRUE(ClientImplDetails::GetRawClient(tested) != nullptr);
149+
auto* retry = dynamic_cast<internal::RetryClient*>(
150+
ClientImplDetails::GetRawClient(tested).get());
148151
ASSERT_TRUE(retry != nullptr);
149152

150153
auto* curl = dynamic_cast<internal::CurlClient*>(retry->client().get());
@@ -159,8 +162,9 @@ TEST_F(ClientTest, LoggingDecorators) {
159162
options.set_enable_raw_client_tracing(true);
160163
Client tested(options);
161164

162-
EXPECT_TRUE(tested.raw_client() != nullptr);
163-
auto* retry = dynamic_cast<internal::RetryClient*>(tested.raw_client().get());
165+
EXPECT_TRUE(ClientImplDetails::GetRawClient(tested) != nullptr);
166+
auto* retry = dynamic_cast<internal::RetryClient*>(
167+
ClientImplDetails::GetRawClient(tested).get());
164168
ASSERT_TRUE(retry != nullptr);
165169

166170
auto* logging = dynamic_cast<internal::LoggingClient*>(retry->client().get());

‎google/cloud/storage/client_write_object_test.cc‎

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,6 @@
2929
namespace google {
3030
namespace cloud {
3131
namespace storage {
32-
namespace testing {
33-
class ClientTester {
34-
public:
35-
static StatusOr<ObjectMetadata> UploadStreamResumable(
36-
Client& client, std::istream& source,
37-
internal::ResumableUploadRequest const& request) {
38-
return client.UploadStreamResumable(source, request);
39-
}
40-
};
41-
} // namespace testing
4232
inline namespace STORAGE_CLIENT_NS {
4333
namespace {
4434

@@ -98,10 +88,10 @@ TEST_F(WriteObjectTest, WriteObject) {
9888
}
9989

10090
TEST_F(WriteObjectTest, WriteObjectTooManyFailures) {
101-
Client client{std::shared_ptr<internal::RawClient>(mock_),
102-
LimitedErrorCountRetryPolicy(2),
103-
ExponentialBackoffPolicy(std::chrono::milliseconds(1),
104-
std::chrono::milliseconds(1), 2.0)};
91+
auto client = testing::ClientFromMock(
92+
mock_, LimitedErrorCountRetryPolicy(2),
93+
ExponentialBackoffPolicy(std::chrono::milliseconds(1),
94+
std::chrono::milliseconds(1), 2.0));
10595

10696
auto returner = [](internal::ResumableUploadRequest const&) {
10797
return StatusOr<std::unique_ptr<internal::ResumableUploadSession>>(
@@ -227,7 +217,7 @@ TEST_F(WriteObjectTest, UploadStreamResumable) {
227217

228218
ASSERT_TRUE(stream);
229219
auto client = ClientForMock();
230-
auto res = testing::ClientTester::UploadStreamResumable(
220+
auto res = internal::ClientImplDetails::UploadStreamResumable(
231221
client, stream,
232222
internal::ResumableUploadRequest("test-bucket-name", "test-object-name"));
233223
ASSERT_STATUS_OK(res);
@@ -299,7 +289,7 @@ TEST_F(WriteObjectTest, UploadStreamResumableSimulateBug) {
299289
auto client = testing::ClientFromMock(
300290
mock, ExponentialBackoffPolicy(std::chrono::milliseconds(1),
301291
std::chrono::milliseconds(1), 2.0));
302-
auto res = testing::ClientTester::UploadStreamResumable(
292+
auto res = internal::ClientImplDetails::UploadStreamResumable(
303293
client, stream,
304294
internal::ResumableUploadRequest("test-bucket-name", "test-object-name"));
305295
EXPECT_THAT(

‎google/cloud/storage/examples/storage_client_mock_samples.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ TEST(StorageMockingSamples, MockReadObject) {
4040
EXPECT_CALL(*mock, client_options())
4141
.WillRepeatedly(ReturnRef(client_options));
4242

43-
gcs::Client client(mock);
43+
auto client = gcs::testing::ClientFromMock(mock);
4444

4545
std::string const text = "this is a mock http response";
4646
std::size_t offset = 0;
@@ -89,7 +89,7 @@ TEST(StorageMockingSamples, MockWriteObject) {
8989
EXPECT_CALL(*mock, client_options())
9090
.WillRepeatedly(ReturnRef(client_options));
9191

92-
gcs::Client client(mock);
92+
auto client = gcs::testing::ClientFromMock(mock);
9393

9494
gcs::ObjectMetadata expected_metadata;
9595

@@ -140,7 +140,7 @@ TEST(StorageMockingSamples, MockReadObjectFailure) {
140140
EXPECT_CALL(*mock, client_options())
141141
.WillRepeatedly(ReturnRef(client_options));
142142

143-
gcs::Client client(mock);
143+
auto client = gcs::testing::ClientFromMock(mock);
144144

145145
std::string text = "this is a mock http response";
146146
EXPECT_CALL(*mock, ReadObject(_))
@@ -178,7 +178,7 @@ TEST(StorageMockingSamples, MockWriteObjectFailure) {
178178
EXPECT_CALL(*mock, client_options())
179179
.WillRepeatedly(ReturnRef(client_options));
180180

181-
gcs::Client client(mock);
181+
auto client = gcs::testing::ClientFromMock(mock);
182182

183183
EXPECT_CALL(*mock, CreateResumableSession(_))
184184
.WillOnce([](gcs::internal::ResumableUploadRequest const& request) {

‎google/cloud/storage/grpc_plugin.cc‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,17 @@ StatusOr<google::cloud::storage::Client> DefaultGrpcClient() {
4141
google::cloud::storage::Client DefaultGrpcClient(
4242
google::cloud::storage::ClientOptions options, int channel_id) {
4343
if (UseGrpcForMetadata()) {
44-
return storage::Client(storage::internal::GrpcClient::Create(
45-
google::cloud::storage::internal::MakeOptions(std::move(options)),
46-
channel_id));
44+
return storage::internal::ClientImplDetails::CreateClient(
45+
storage::internal::GrpcClient::Create(
46+
google::cloud::storage::internal::MakeOptions(std::move(options)),
47+
channel_id));
4748
}
4849
auto credentials = options.credentials();
49-
return storage::Client(storage::internal::HybridClient::Create(
50-
std::move(credentials),
51-
google::cloud::storage::internal::MakeOptions(std::move(options)),
52-
channel_id));
50+
return storage::internal::ClientImplDetails::CreateClient(
51+
storage::internal::HybridClient::Create(
52+
std::move(credentials),
53+
google::cloud::storage::internal::MakeOptions(std::move(options)),
54+
channel_id));
5355
}
5456

5557
} // namespace STORAGE_CLIENT_NS

0 commit comments

Comments
 (0)