Skip to content

Commit 8c6ff70

Browse files
coryancoryan
authored
refactor(storage): reduce indirection on restoring uploads (#7283)
The `RawClient::RestoreResumableUpload()` function is not really used, and it lacks a parameter with the original request (which may have set options, such as custom headers). With this cleanup the function is deprecated. This enables a change to fully support standard parameters when restoring resumable uploads.
1 parent b3655b0 commit 8c6ff70

22 files changed

Lines changed: 130 additions & 100 deletions

‎google/cloud/storage/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ add_library(
174174
internal/patch_builder.h
175175
internal/policy_document_request.cc
176176
internal/policy_document_request.h
177+
internal/raw_client.cc
177178
internal/raw_client.h
178179
internal/raw_client_wrapper_utils.h
179180
internal/resumable_upload_session.cc

‎google/cloud/storage/google_cloud_cpp_storage.bzl‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,7 @@ google_cloud_cpp_storage_srcs = [
191191
"internal/openssl_util.cc",
192192
"internal/patch_builder.cc",
193193
"internal/policy_document_request.cc",
194+
"internal/raw_client.cc",
194195
"internal/resumable_upload_session.cc",
195196
"internal/retry_client.cc",
196197
"internal/retry_object_read_source.cc",

‎google/cloud/storage/internal/curl_client.cc‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,16 @@ StatusOr<ResumableUploadResponse> CurlClient::QueryResumableUpload(
225225
return AsStatus(*response);
226226
}
227227

228+
StatusOr<std::unique_ptr<ResumableUploadSession>>
229+
CurlClient::FullyRestoreResumableSession(ResumableUploadRequest const& request,
230+
std::string const& session_id) {
231+
auto session = absl::make_unique<CurlResumableUploadSession>(
232+
shared_from_this(), request, session_id);
233+
auto response = session->ResetSession();
234+
if (!response) std::move(response).status();
235+
return std::unique_ptr<ResumableUploadSession>(std::move(session));
236+
}
237+
228238
StatusOr<ListBucketsResponse> CurlClient::ListBuckets(
229239
ListBucketsRequest const& request) {
230240
CurlRequestBuilder builder(storage_endpoint_ + "/b", storage_factory_);
@@ -624,15 +634,13 @@ StatusOr<std::unique_ptr<ResumableUploadSession>>
624634
CurlClient::CreateResumableSession(ResumableUploadRequest const& request) {
625635
auto session_id = request.GetOption<UseResumableUploadSession>().value_or("");
626636
if (!session_id.empty()) {
627-
return RestoreResumableSession(session_id);
637+
return FullyRestoreResumableSession(request, session_id);
628638
}
629639

630640
CurlRequestBuilder builder(
631641
upload_endpoint_ + "/b/" + request.bucket_name() + "/o", upload_factory_);
632642
auto status = SetupBuilderCommon(builder, "POST");
633-
if (!status.ok()) {
634-
return status;
635-
}
643+
if (!status.ok()) return status;
636644

637645
// In most cases we use `SetupBuilder()` to set up all these options in the
638646
// request. But in this case we cannot because that might also set
@@ -705,19 +713,8 @@ CurlClient::CreateResumableSession(ResumableUploadRequest const& request) {
705713
}
706714
return std::unique_ptr<ResumableUploadSession>(
707715
absl::make_unique<CurlResumableUploadSession>(
708-
shared_from_this(), std::move(response->upload_session_url),
709-
request.GetOption<CustomHeader>()));
710-
}
711-
712-
StatusOr<std::unique_ptr<ResumableUploadSession>>
713-
CurlClient::RestoreResumableSession(std::string const& session_id) {
714-
auto session = absl::make_unique<CurlResumableUploadSession>(
715-
shared_from_this(), session_id);
716-
auto response = session->ResetSession();
717-
if (response.status().ok()) {
718-
return std::unique_ptr<ResumableUploadSession>(std::move(session));
719-
}
720-
return std::move(response).status();
716+
shared_from_this(), request,
717+
std::move(response->upload_session_url)));
721718
}
722719

723720
StatusOr<EmptyResponse> CurlClient::DeleteResumableUpload(

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,16 @@ class CurlClient : public RawClient,
7272
/// @name Implement the CurlResumableSession operations.
7373
// Note that these member functions are not inherited from RawClient, they are
7474
// called only by `CurlResumableUploadSession`, because the retry loop for
75-
// them is very different from the standard retry loop. Also note that these
76-
// are virtual functions only because we need to override them in the unit
77-
// tests.
75+
// them is very different from the standard retry loop. Also note that some of
76+
// these member functions are virtual, but only because we need to override
77+
// them in the *library* unit tests.
7878
virtual StatusOr<ResumableUploadResponse> UploadChunk(
7979
UploadChunkRequest const&);
8080
virtual StatusOr<ResumableUploadResponse> QueryResumableUpload(
8181
QueryResumableUploadRequest const&);
82+
StatusOr<std::unique_ptr<ResumableUploadSession>>
83+
FullyRestoreResumableSession(ResumableUploadRequest const& request,
84+
std::string const& session_id);
8285
//@}
8386

8487
ClientOptions const& client_options() const override {
@@ -127,8 +130,6 @@ class CurlClient : public RawClient,
127130
ComposeObjectRequest const& request) override;
128131
StatusOr<std::unique_ptr<ResumableUploadSession>> CreateResumableSession(
129132
ResumableUploadRequest const& request) override;
130-
StatusOr<std::unique_ptr<ResumableUploadSession>> RestoreResumableSession(
131-
std::string const& session_id) override;
132133
StatusOr<EmptyResponse> DeleteResumableUpload(
133134
DeleteResumableUploadRequest const& request) override;
134135

‎google/cloud/storage/internal/curl_resumable_upload_session.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ namespace internal {
2323
StatusOr<ResumableUploadResponse> CurlResumableUploadSession::UploadChunk(
2424
ConstBufferSequence const& buffers) {
2525
UploadChunkRequest request(session_id_, next_expected_, buffers);
26-
request.set_option(custom_header());
26+
request.set_multiple_options(request_.GetOption<CustomHeader>());
2727
auto result = client_->UploadChunk(request);
2828
Update(result, TotalBytes(buffers));
2929
return result;
@@ -33,14 +33,15 @@ StatusOr<ResumableUploadResponse> CurlResumableUploadSession::UploadFinalChunk(
3333
ConstBufferSequence const& buffers, std::uint64_t upload_size,
3434
HashValues const& /*full_object_hashes*/) {
3535
UploadChunkRequest request(session_id_, next_expected_, buffers, upload_size);
36-
request.set_option(custom_header());
36+
request.set_multiple_options(request_.GetOption<CustomHeader>());
3737
auto result = client_->UploadChunk(request);
3838
Update(result, TotalBytes(buffers));
3939
return result;
4040
}
4141

4242
StatusOr<ResumableUploadResponse> CurlResumableUploadSession::ResetSession() {
4343
QueryResumableUploadRequest request(session_id_);
44+
request.set_multiple_options(request_.GetOption<CustomHeader>());
4445
auto result = client_->QueryResumableUpload(request);
4546
Update(result, 0);
4647
return result;

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@ namespace internal {
3030
*/
3131
class CurlResumableUploadSession : public ResumableUploadSession {
3232
public:
33-
explicit CurlResumableUploadSession(
34-
std::shared_ptr<CurlClient> client, std::string session_id,
35-
CustomHeader custom_header = CustomHeader())
33+
explicit CurlResumableUploadSession(std::shared_ptr<CurlClient> client,
34+
ResumableUploadRequest request,
35+
std::string session_id)
3636
: client_(std::move(client)),
37-
session_id_(std::move(session_id)),
38-
custom_header_(std::move(custom_header)) {}
37+
request_(std::move(request)),
38+
session_id_(std::move(session_id)) {}
3939

4040
StatusOr<ResumableUploadResponse> UploadChunk(
4141
ConstBufferSequence const& buffers) override;
@@ -50,8 +50,6 @@ class CurlResumableUploadSession : public ResumableUploadSession {
5050

5151
std::string const& session_id() const override { return session_id_; }
5252

53-
CustomHeader const& custom_header() const { return custom_header_; }
54-
5553
bool done() const override { return done_; }
5654

5755
StatusOr<ResumableUploadResponse> const& last_response() const override {
@@ -63,8 +61,8 @@ class CurlResumableUploadSession : public ResumableUploadSession {
6361
std::size_t chunk_size);
6462

6563
std::shared_ptr<CurlClient> client_;
64+
ResumableUploadRequest request_;
6665
std::string session_id_;
67-
CustomHeader custom_header_;
6866
std::uint64_t next_expected_ = 0;
6967
bool done_ = false;
7068
StatusOr<ResumableUploadResponse> last_response_;

‎google/cloud/storage/internal/curl_resumable_upload_session_test.cc‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,10 @@ MATCHER_P(MatchesPayload, value, "Checks whether payload matches a value") {
5656
TEST(CurlResumableUploadSessionTest, Simple) {
5757
auto mock = MockCurlClient::Create();
5858
std::string test_url = "http://invalid.example.com/not-used-in-mock";
59-
CurlResumableUploadSession session(mock, test_url);
59+
auto const header = CustomHeader("x-test-custom", "custom-value");
60+
ResumableUploadRequest request("test-bucket", "test-object");
61+
request.set_option(header);
62+
CurlResumableUploadSession session(mock, request, test_url);
6063

6164
std::string const payload = "test payload";
6265
auto const size = payload.size();
@@ -65,6 +68,8 @@ TEST(CurlResumableUploadSessionTest, Simple) {
6568
EXPECT_EQ(0, session.next_expected_byte());
6669
EXPECT_CALL(*mock, UploadChunk)
6770
.WillOnce([&](UploadChunkRequest const& request) {
71+
EXPECT_EQ(request.GetOption<CustomHeader>().value_or(""),
72+
header.value());
6873
EXPECT_EQ(test_url, request.upload_session_url());
6974
EXPECT_THAT(request.payload(), MatchesPayload(payload));
7075
EXPECT_EQ(0, request.source_size());
@@ -98,7 +103,10 @@ TEST(CurlResumableUploadSessionTest, Reset) {
98103
auto mock = MockCurlClient::Create();
99104
std::string url1 = "http://invalid.example.com/not-used-in-mock-1";
100105
std::string url2 = "http://invalid.example.com/not-used-in-mock-2";
101-
CurlResumableUploadSession session(mock, url1);
106+
auto const header = CustomHeader("x-test-custom", "custom-value");
107+
ResumableUploadRequest request("test-bucket", "test-object");
108+
request.set_option(header);
109+
CurlResumableUploadSession session(mock, request, url1);
102110

103111
std::string const payload = "test payload";
104112
auto const size = payload.size();
@@ -117,6 +125,8 @@ TEST(CurlResumableUploadSessionTest, Reset) {
117125
url2, 2 * size - 1, {}, ResumableUploadResponse::kInProgress, {}};
118126
EXPECT_CALL(*mock, QueryResumableUpload)
119127
.WillOnce([&](QueryResumableUploadRequest const& request) {
128+
EXPECT_EQ(request.GetOption<CustomHeader>().value_or(""),
129+
header.value());
120130
EXPECT_EQ(url1, request.upload_session_url());
121131
return make_status_or(resume_response);
122132
});
@@ -143,7 +153,8 @@ TEST(CurlResumableUploadSessionTest, SessionUpdatedInChunkUpload) {
143153
auto mock = MockCurlClient::Create();
144154
std::string url1 = "http://invalid.example.com/not-used-in-mock-1";
145155
std::string url2 = "http://invalid.example.com/not-used-in-mock-2";
146-
CurlResumableUploadSession session(mock, url1);
156+
ResumableUploadRequest request("test-bucket", "test-object");
157+
CurlResumableUploadSession session(mock, request, url1);
147158

148159
std::string const payload = "test payload";
149160
auto const size = payload.size();
@@ -172,7 +183,8 @@ TEST(CurlResumableUploadSessionTest, SessionUpdatedInChunkUpload) {
172183
TEST(CurlResumableUploadSessionTest, Empty) {
173184
auto mock = MockCurlClient::Create();
174185
std::string test_url = "http://invalid.example.com/not-used-in-mock";
175-
CurlResumableUploadSession session(mock, test_url);
186+
ResumableUploadRequest request("test-bucket", "test-object");
187+
CurlResumableUploadSession session(mock, request, test_url);
176188

177189
std::string const payload{};
178190
auto const size = payload.size();

‎google/cloud/storage/internal/grpc_client.cc‎

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,23 @@ StatusOr<ResumableUploadResponse> GrpcClient::QueryResumableUpload(
194194
return response;
195195
}
196196

197+
StatusOr<std::unique_ptr<ResumableUploadSession>>
198+
GrpcClient::FullyRestoreResumableSession(ResumableUploadRequest const& request,
199+
std::string const& upload_url) {
200+
auto self = shared_from_this();
201+
auto upload_session_params = DecodeGrpcResumableUploadSessionUrl(upload_url);
202+
if (!upload_session_params) {
203+
return upload_session_params.status();
204+
}
205+
auto session = std::unique_ptr<ResumableUploadSession>(
206+
new GrpcResumableUploadSession(self, request, *upload_session_params));
207+
auto response = session->ResetSession();
208+
if (response.status().ok()) {
209+
return session;
210+
}
211+
return std::move(response).status();
212+
}
213+
197214
ClientOptions const& GrpcClient::client_options() const {
198215
return backwards_compatibility_options_;
199216
}
@@ -360,11 +377,9 @@ StatusOr<RewriteObjectResponse> GrpcClient::RewriteObject(
360377

361378
StatusOr<std::unique_ptr<ResumableUploadSession>>
362379
GrpcClient::CreateResumableSession(ResumableUploadRequest const& request) {
363-
if (request.HasOption<UseResumableUploadSession>()) {
364-
auto session_id = request.GetOption<UseResumableUploadSession>().value();
365-
if (!session_id.empty()) {
366-
return RestoreResumableSession(session_id);
367-
}
380+
auto session_id = request.GetOption<UseResumableUploadSession>().value_or("");
381+
if (!session_id.empty()) {
382+
return FullyRestoreResumableSession(request, session_id);
368383
}
369384

370385
auto proto_request = ToProto(request);
@@ -376,26 +391,10 @@ GrpcClient::CreateResumableSession(ResumableUploadRequest const& request) {
376391

377392
auto self = shared_from_this();
378393
return std::unique_ptr<ResumableUploadSession>(new GrpcResumableUploadSession(
379-
self,
394+
self, request,
380395
{request.bucket_name(), request.object_name(), response->upload_id()}));
381396
}
382397

383-
StatusOr<std::unique_ptr<ResumableUploadSession>>
384-
GrpcClient::RestoreResumableSession(std::string const& upload_url) {
385-
auto self = shared_from_this();
386-
auto upload_session_params = DecodeGrpcResumableUploadSessionUrl(upload_url);
387-
if (!upload_session_params) {
388-
return upload_session_params.status();
389-
}
390-
auto session = std::unique_ptr<ResumableUploadSession>(
391-
new GrpcResumableUploadSession(self, *upload_session_params));
392-
auto response = session->ResetSession();
393-
if (response.status().ok()) {
394-
return session;
395-
}
396-
return std::move(response).status();
397-
}
398-
399398
StatusOr<EmptyResponse> GrpcClient::DeleteResumableUpload(
400399
DeleteResumableUploadRequest const&) {
401400
return Status(StatusCode::kUnimplemented, __func__);

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,19 @@ class GrpcClient : public RawClient,
5353
/// @name Implement the ResumableSession operations.
5454
// Note that these member functions are not inherited from RawClient, they are
5555
// called only by `GrpcResumableUploadSession`, because the retry loop for
56-
// them is very different from the standard retry loop. Also note that these
57-
// are virtual functions only because we need to override them in the unit
58-
// tests.
56+
// them is very different from the standard retry loop. Also note that some of
57+
// these member functions are virtual, but only because we need to override
58+
// them in the *library* unit tests.
5959
using WriteObjectStream = ::google::cloud::internal::StreamingWriteRpc<
6060
google::storage::v2::WriteObjectRequest,
6161
google::storage::v2::WriteObjectResponse>;
6262
virtual std::unique_ptr<WriteObjectStream> CreateUploadWriter(
6363
std::unique_ptr<grpc::ClientContext> context);
6464
virtual StatusOr<ResumableUploadResponse> QueryResumableUpload(
6565
QueryResumableUploadRequest const&);
66+
StatusOr<std::unique_ptr<ResumableUploadSession>>
67+
FullyRestoreResumableSession(ResumableUploadRequest const& request,
68+
std::string const& upload_url);
6669
//@}
6770

6871
ClientOptions const& client_options() const override;
@@ -113,8 +116,6 @@ class GrpcClient : public RawClient,
113116
RewriteObjectRequest const&) override;
114117
StatusOr<std::unique_ptr<ResumableUploadSession>> CreateResumableSession(
115118
ResumableUploadRequest const& request) override;
116-
StatusOr<std::unique_ptr<ResumableUploadSession>> RestoreResumableSession(
117-
std::string const& upload_url) override;
118119
StatusOr<EmptyResponse> DeleteResumableUpload(
119120
DeleteResumableUploadRequest const& request) override;
120121

‎google/cloud/storage/internal/grpc_resumable_upload_session.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,10 @@ static_assert(google::storage::v2::ServiceConstants::MAX_WRITE_CHUNK_BYTES >
3333
"the chunk quantum");
3434

3535
GrpcResumableUploadSession::GrpcResumableUploadSession(
36-
std::shared_ptr<GrpcClient> client,
36+
std::shared_ptr<GrpcClient> client, ResumableUploadRequest request,
3737
ResumableUploadSessionGrpcParams session_id_params)
3838
: client_(std::move(client)),
39+
request_(std::move(request)),
3940
session_id_params_(std::move(session_id_params)),
4041
session_url_(EncodeGrpcResumableUploadSessionUrl(session_id_params_)) {}
4142

0 commit comments

Comments
 (0)