Skip to content

Commit 04b883a

Browse files
coryancoryan
authored
refactor(storage): simplify internal::CurlClient (#7281)
Refactor a template member function that no longer needs to be a template. This will enable further changes related to RestoreResumableSession.
1 parent e678cf3 commit 04b883a

2 files changed

Lines changed: 85 additions & 101 deletions

File tree

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

Lines changed: 85 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -160,102 +160,6 @@ Status CurlClient::SetupBuilder(CurlRequestBuilder& builder,
160160
return Status();
161161
}
162162

163-
template <typename RequestType>
164-
StatusOr<std::unique_ptr<ResumableUploadSession>>
165-
CurlClient::CreateResumableSessionGeneric(RequestType const& request) {
166-
auto session_id =
167-
request.template GetOption<UseResumableUploadSession>().value_or("");
168-
if (!session_id.empty()) {
169-
return RestoreResumableSession(session_id);
170-
}
171-
172-
CurlRequestBuilder builder(
173-
upload_endpoint_ + "/b/" + request.bucket_name() + "/o", upload_factory_);
174-
auto status = SetupBuilderCommon(builder, "POST");
175-
if (!status.ok()) {
176-
return status;
177-
}
178-
179-
// In most cases we use `SetupBuilder()` to setup all these options in the
180-
// request. But in this case we cannot because that might also set
181-
// `Content-Type` to the wrong value. Instead we have to explicitly list all
182-
// the options here. Somebody could write a clever meta-function to say
183-
// "set all the options except `ContentType`, but I think that is going to be
184-
// very hard to understand.
185-
builder.AddOption(request.template GetOption<EncryptionKey>());
186-
builder.AddOption(request.template GetOption<IfGenerationMatch>());
187-
builder.AddOption(request.template GetOption<IfGenerationNotMatch>());
188-
builder.AddOption(request.template GetOption<IfMetagenerationMatch>());
189-
builder.AddOption(request.template GetOption<IfMetagenerationNotMatch>());
190-
builder.AddOption(request.template GetOption<KmsKeyName>());
191-
builder.AddOption(request.template GetOption<PredefinedAcl>());
192-
builder.AddOption(request.template GetOption<Projection>());
193-
builder.AddOption(request.template GetOption<UserProject>());
194-
builder.AddOption(request.template GetOption<CustomHeader>());
195-
builder.AddOption(request.template GetOption<Fields>());
196-
builder.AddOption(request.template GetOption<IfMatchEtag>());
197-
builder.AddOption(request.template GetOption<IfNoneMatchEtag>());
198-
builder.AddOption(request.template GetOption<QuotaUser>());
199-
builder.AddOption(request.template GetOption<UploadContentLength>());
200-
SetupBuilderUserIp(builder, request);
201-
202-
builder.AddQueryParameter("uploadType", "resumable");
203-
builder.AddHeader("Content-Type: application/json; charset=UTF-8");
204-
nlohmann::json resource;
205-
if (request.template HasOption<WithObjectMetadata>()) {
206-
resource = ObjectMetadataJsonForInsert(
207-
request.template GetOption<WithObjectMetadata>().value());
208-
}
209-
if (request.template HasOption<ContentEncoding>()) {
210-
resource["contentEncoding"] =
211-
request.template GetOption<ContentEncoding>().value();
212-
}
213-
if (request.template HasOption<ContentType>()) {
214-
resource["contentType"] = request.template GetOption<ContentType>().value();
215-
}
216-
if (request.template HasOption<Crc32cChecksumValue>()) {
217-
resource["crc32c"] =
218-
request.template GetOption<Crc32cChecksumValue>().value();
219-
}
220-
if (request.template HasOption<MD5HashValue>()) {
221-
resource["md5Hash"] = request.template GetOption<MD5HashValue>().value();
222-
}
223-
224-
if (resource.empty()) {
225-
builder.AddQueryParameter("name", request.object_name());
226-
} else {
227-
resource["name"] = request.object_name();
228-
}
229-
230-
std::string request_payload;
231-
if (!resource.empty()) {
232-
request_payload = resource.dump();
233-
}
234-
builder.AddHeader("Content-Length: " +
235-
std::to_string(request_payload.size()));
236-
auto http_response = builder.BuildRequest().MakeRequest(request_payload);
237-
if (!http_response.ok()) {
238-
return std::move(http_response).status();
239-
}
240-
if (http_response->status_code >= HttpStatusCode::kMinNotSuccess) {
241-
return AsStatus(*http_response);
242-
}
243-
auto response =
244-
ResumableUploadResponse::FromHttpResponse(*std::move(http_response));
245-
if (!response.ok()) {
246-
return std::move(response).status();
247-
}
248-
if (response->upload_session_url.empty()) {
249-
std::ostringstream os;
250-
os << __func__ << " - invalid server response, parsed to " << *response;
251-
return Status(StatusCode::kInternal, std::move(os).str());
252-
}
253-
return std::unique_ptr<ResumableUploadSession>(
254-
absl::make_unique<CurlResumableUploadSession>(
255-
shared_from_this(), std::move(response->upload_session_url),
256-
std::move(request.template GetOption<CustomHeader>())));
257-
}
258-
259163
CurlClient::CurlClient(google::cloud::Options options)
260164
: opts_(std::move(options)),
261165
backwards_compatibility_options_(
@@ -718,7 +622,91 @@ StatusOr<RewriteObjectResponse> CurlClient::RewriteObject(
718622

719623
StatusOr<std::unique_ptr<ResumableUploadSession>>
720624
CurlClient::CreateResumableSession(ResumableUploadRequest const& request) {
721-
return CreateResumableSessionGeneric(request);
625+
auto session_id = request.GetOption<UseResumableUploadSession>().value_or("");
626+
if (!session_id.empty()) {
627+
return RestoreResumableSession(session_id);
628+
}
629+
630+
CurlRequestBuilder builder(
631+
upload_endpoint_ + "/b/" + request.bucket_name() + "/o", upload_factory_);
632+
auto status = SetupBuilderCommon(builder, "POST");
633+
if (!status.ok()) {
634+
return status;
635+
}
636+
637+
// In most cases we use `SetupBuilder()` to set up all these options in the
638+
// request. But in this case we cannot because that might also set
639+
// `Content-Type` to the wrong value. Instead, we have to explicitly list all
640+
// the options here. Somebody could write a clever meta-function to say
641+
// "set all the options except `ContentType`, but I think that is going to be
642+
// very hard to understand.
643+
builder.AddOption(request.GetOption<EncryptionKey>());
644+
builder.AddOption(request.GetOption<IfGenerationMatch>());
645+
builder.AddOption(request.GetOption<IfGenerationNotMatch>());
646+
builder.AddOption(request.GetOption<IfMetagenerationMatch>());
647+
builder.AddOption(request.GetOption<IfMetagenerationNotMatch>());
648+
builder.AddOption(request.GetOption<KmsKeyName>());
649+
builder.AddOption(request.GetOption<PredefinedAcl>());
650+
builder.AddOption(request.GetOption<Projection>());
651+
builder.AddOption(request.GetOption<UserProject>());
652+
builder.AddOption(request.GetOption<CustomHeader>());
653+
builder.AddOption(request.GetOption<Fields>());
654+
builder.AddOption(request.GetOption<IfMatchEtag>());
655+
builder.AddOption(request.GetOption<IfNoneMatchEtag>());
656+
builder.AddOption(request.GetOption<QuotaUser>());
657+
builder.AddOption(request.GetOption<UploadContentLength>());
658+
SetupBuilderUserIp(builder, request);
659+
660+
builder.AddQueryParameter("uploadType", "resumable");
661+
builder.AddHeader("Content-Type: application/json; charset=UTF-8");
662+
nlohmann::json resource;
663+
if (request.HasOption<WithObjectMetadata>()) {
664+
resource = ObjectMetadataJsonForInsert(
665+
request.GetOption<WithObjectMetadata>().value());
666+
}
667+
if (request.HasOption<ContentEncoding>()) {
668+
resource["contentEncoding"] = request.GetOption<ContentEncoding>().value();
669+
}
670+
if (request.HasOption<ContentType>()) {
671+
resource["contentType"] = request.GetOption<ContentType>().value();
672+
}
673+
if (request.HasOption<Crc32cChecksumValue>()) {
674+
resource["crc32c"] = request.GetOption<Crc32cChecksumValue>().value();
675+
}
676+
if (request.HasOption<MD5HashValue>()) {
677+
resource["md5Hash"] = request.GetOption<MD5HashValue>().value();
678+
}
679+
680+
if (resource.empty()) {
681+
builder.AddQueryParameter("name", request.object_name());
682+
} else {
683+
resource["name"] = request.object_name();
684+
}
685+
686+
std::string request_payload;
687+
if (!resource.empty()) request_payload = resource.dump();
688+
689+
builder.AddHeader("Content-Length: " +
690+
std::to_string(request_payload.size()));
691+
auto http_response = builder.BuildRequest().MakeRequest(request_payload);
692+
if (!http_response.ok()) {
693+
return std::move(http_response).status();
694+
}
695+
if (http_response->status_code >= HttpStatusCode::kMinNotSuccess) {
696+
return AsStatus(*http_response);
697+
}
698+
auto response =
699+
ResumableUploadResponse::FromHttpResponse(*std::move(http_response));
700+
if (!response.ok()) return std::move(response).status();
701+
if (response->upload_session_url.empty()) {
702+
std::ostringstream os;
703+
os << __func__ << " - invalid server response, parsed to " << *response;
704+
return Status(StatusCode::kInternal, std::move(os).str());
705+
}
706+
return std::unique_ptr<ResumableUploadSession>(
707+
absl::make_unique<CurlResumableUploadSession>(
708+
shared_from_this(), std::move(response->upload_session_url),
709+
request.GetOption<CustomHeader>()));
722710
}
723711

724712
StatusOr<std::unique_ptr<ResumableUploadSession>>

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,6 @@ class CurlClient : public RawClient,
224224
StatusOr<ObjectMetadata> InsertObjectMediaSimple(
225225
InsertObjectMediaRequest const& request);
226226

227-
template <typename RequestType>
228-
StatusOr<std::unique_ptr<ResumableUploadSession>>
229-
CreateResumableSessionGeneric(RequestType const& request);
230-
231227
google::cloud::Options opts_;
232228
ClientOptions backwards_compatibility_options_;
233229
std::string const x_goog_api_client_header_;

0 commit comments

Comments
 (0)