From 9c192b33cdcbc2d5465094c77b61a5d23e0a184c Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Sat, 29 Apr 2023 16:06:08 +0000 Subject: [PATCH 1/2] fix(storage): support per-request options With the `rest_internal::RestClient` implementation we were not respecting some per-request options. This changes the implementation of `rest_internal::RestClient` to use the options provided in `rest_internal::RestContext`. That seemed cleaner than using the options span. I gave myself some leeway by adding default constructors to some `storage::internal::*Request` classes. It makes it easier to write unit tests where the contents of the request are uninteresting. I included an integration test in `rest_internal::RestClient`. --- google/cloud/internal/curl_rest_client.cc | 61 +- google/cloud/internal/curl_rest_client.h | 4 +- .../curl_rest_client_integration_test.cc | 29 +- google/cloud/internal/rest_context.h | 10 +- .../storage/internal/bucket_acl_requests.h | 1 + .../cloud/storage/internal/bucket_requests.cc | 4 + .../cloud/storage/internal/bucket_requests.h | 1 + .../internal/default_object_acl_requests.h | 1 + .../storage/internal/hmac_key_requests.h | 4 + .../storage/internal/object_acl_requests.h | 1 + .../cloud/storage/internal/object_requests.cc | 3 + .../cloud/storage/internal/object_requests.h | 3 +- google/cloud/storage/internal/rest_client.cc | 110 +-- .../storage/internal/rest_client_test.cc | 642 ++++++++++++++++++ .../storage/internal/sign_blob_requests.h | 1 + 15 files changed, 788 insertions(+), 87 deletions(-) diff --git a/google/cloud/internal/curl_rest_client.cc b/google/cloud/internal/curl_rest_client.cc index 5ec7627fa1e53..ccc972f22ee00 100644 --- a/google/cloud/internal/curl_rest_client.cc +++ b/google/cloud/internal/curl_rest_client.cc @@ -94,37 +94,33 @@ std::string CurlRestClient::HostHeader(Options const& options, return {}; } -CurlRestClient::CurlRestClient(std::string endpoint_address, - std::shared_ptr factory, - Options options) +CurlRestClient::CurlRestClient( + std::string endpoint_address, std::shared_ptr factory, + std::shared_ptr credentials) : endpoint_address_(std::move(endpoint_address)), handle_factory_(std::move(factory)), x_goog_api_client_header_("x-goog-api-client: " + google::cloud::internal::ApiClientHeader()), - options_(std::move(options)) { - if (options_.has()) { - credentials_ = MapCredentials(options_.get()); - } -} + credentials_(std::move(credentials)) {} StatusOr> CurlRestClient::CreateCurlImpl( RestContext const& context, RestRequest const& request) { auto handle = CurlHandle::MakeFromPool(*handle_factory_); - auto impl = - std::make_unique(std::move(handle), handle_factory_, options_); + auto impl = std::make_unique(std::move(handle), handle_factory_, + context.options()); if (credentials_) { auto auth_header = oauth2_internal::AuthorizationHeader(*credentials_); if (!auth_header.ok()) return std::move(auth_header).status(); impl->SetHeader(auth_header.value()); } - impl->SetHeader(HostHeader(options_, endpoint_address_)); + impl->SetHeader(HostHeader(context.options(), endpoint_address_)); impl->SetHeader(x_goog_api_client_header_); impl->SetHeaders(context, request); RestRequest::HttpParameters additional_parameters; // The UserIp option has been deprecated in favor of quotaUser. Only add the // parameter if the option has been set. - if (options_.has()) { - auto user_ip = options_.get(); + if (context.options().has()) { + auto user_ip = context.options().get(); if (user_ip.empty()) user_ip = impl->LastClientIpAddress(); if (!user_ip.empty()) additional_parameters.emplace_back("userIp", user_ip); } @@ -145,7 +141,7 @@ StatusOr> CurlRestClient::Delete( auto response = (*impl)->MakeRequest(CurlImpl::HttpMethod::kDelete, context); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(options_, std::move(*impl)))}; + new CurlRestResponse(context.options(), std::move(*impl)))}; } StatusOr> CurlRestClient::Get( @@ -155,7 +151,7 @@ StatusOr> CurlRestClient::Get( auto response = (*impl)->MakeRequest(CurlImpl::HttpMethod::kGet, context); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(options_, std::move(*impl)))}; + new CurlRestResponse(context.options(), std::move(*impl)))}; } StatusOr> CurlRestClient::Patch( @@ -167,7 +163,7 @@ StatusOr> CurlRestClient::Patch( context, request, **impl, payload); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(options_, std::move(*impl)))}; + new CurlRestResponse(context.options(), std::move(*impl)))}; } StatusOr> CurlRestClient::Post( @@ -179,7 +175,7 @@ StatusOr> CurlRestClient::Post( request, **impl, payload); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(options_, std::move(*impl)))}; + new CurlRestResponse(context.options(), std::move(*impl)))}; } StatusOr> CurlRestClient::Post( @@ -198,7 +194,7 @@ StatusOr> CurlRestClient::Post( request, **impl, {form_payload}); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(options_, std::move(*impl)))}; + new CurlRestResponse(context.options(), std::move(*impl)))}; } StatusOr> CurlRestClient::Put( @@ -210,32 +206,45 @@ StatusOr> CurlRestClient::Put( request, **impl, payload); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(options_, std::move(*impl)))}; + new CurlRestResponse(context.options(), std::move(*impl)))}; } -std::unique_ptr MakeDefaultRestClient(std::string endpoint_address, - Options options) { +std::unique_ptr MakeDefaultRestClient( + std::string endpoint_address, + // NOLINTNEXTLINE(performance-unnecessary-value-param) + Options options) { auto factory = GetDefaultCurlHandleFactory(options); + + auto credentials = + options.has() + ? MapCredentials(options.get()) + : std::shared_ptr(); return MakeTracingRestClient(std::make_unique( - std::move(endpoint_address), std::move(factory), std::move(options))); + std::move(endpoint_address), std::move(factory), std::move(credentials))); } -std::unique_ptr MakePooledRestClient(std::string endpoint_address, - Options options) { +std::unique_ptr MakePooledRestClient( + std::string endpoint_address, + // NOLINTNEXTLINE(performance-unnecessary-value-param) + Options options) { std::size_t pool_size = kDefaultPooledCurlHandleFactorySize; if (options.has()) { pool_size = options.get(); } + auto credentials = + options.has() + ? MapCredentials(options.get()) + : std::shared_ptr(); if (pool_size > 0) { auto pool = std::make_shared(pool_size, options); return MakeTracingRestClient(std::make_unique( - std::move(endpoint_address), std::move(pool), std::move(options))); + std::move(endpoint_address), std::move(pool), std::move(credentials))); } auto pool = std::make_shared(options); return MakeTracingRestClient(std::make_unique( - std::move(endpoint_address), std::move(pool), std::move(options))); + std::move(endpoint_address), std::move(pool), std::move(credentials))); } GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/internal/curl_rest_client.h b/google/cloud/internal/curl_rest_client.h index f9636ba644421..78826980adc0f 100644 --- a/google/cloud/internal/curl_rest_client.h +++ b/google/cloud/internal/curl_rest_client.h @@ -43,7 +43,8 @@ class CurlRestClient : public RestClient { static std::string HostHeader(Options const& options, std::string const& endpoint); CurlRestClient(std::string endpoint_address, - std::shared_ptr factory, Options options); + std::shared_ptr factory, + std::shared_ptr credentials); ~CurlRestClient() override = default; CurlRestClient(CurlRestClient const&) = delete; @@ -77,7 +78,6 @@ class CurlRestClient : public RestClient { std::shared_ptr handle_factory_; std::string x_goog_api_client_header_; std::shared_ptr credentials_; - Options options_; }; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/internal/curl_rest_client_integration_test.cc b/google/cloud/internal/curl_rest_client_integration_test.cc index d1abb68151507..01097f2ecd3c2 100644 --- a/google/cloud/internal/curl_rest_client_integration_test.cc +++ b/google/cloud/internal/curl_rest_client_integration_test.cc @@ -155,13 +155,13 @@ TEST_F(RestClientIntegrationTest, Get) { TEST_F(RestClientIntegrationTest, Delete) { options_.set(MakeInsecureCredentials()); - options_.set("127.0.0.1"); auto client = MakeDefaultRestClient(url_, options_); RestRequest request; request.SetPath("delete"); request.AddQueryParameter({"key", "value"}); auto response_status = RetryRestRequest([&] { - rest_internal::RestContext context; + rest_internal::RestContext context( + Options{}.set("127.0.0.1")); return client->Delete(context, request); }); ASSERT_STATUS_OK(response_status); @@ -533,6 +533,31 @@ TEST_F(RestClientIntegrationTest, CaptureMetadata) { ASSERT_TRUE(parsed_response.is_object()) << "body=" << *body; } +TEST_F(RestClientIntegrationTest, PerRequestOptions) { + auto client = MakeDefaultRestClient(url_, {}); + RestRequest request; + request.SetPath("anything"); + auto const version = google::cloud::version_string(); + auto const p1 = "p1/" + google::cloud::version_string(); + auto const p2 = "p2/" + google::cloud::version_string(); + auto response_status = RetryRestRequest([&] { + rest_internal::RestContext context( + Options{}.set({p1, p2})); + return client->Get(context, request); + }); + ASSERT_STATUS_OK(response_status); + auto response = *std::move(response_status); + ASSERT_THAT(response->StatusCode(), Eq(HttpStatusCode::kOk)); + auto body = ReadAll(std::move(*response).ExtractPayload()); + ASSERT_STATUS_OK(body); + auto parsed_response = nlohmann::json::parse(*body, nullptr, false); + ASSERT_TRUE(parsed_response.is_object()) << "body=" << *body; + auto headers = parsed_response.find("headers"); + ASSERT_TRUE(headers != parsed_response.end()) << "body=" << *body; + EXPECT_THAT(headers->value("User-Agent", ""), HasSubstr(p1)); + EXPECT_THAT(headers->value("User-Agent", ""), HasSubstr(p2)); +} + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace rest_internal diff --git a/google/cloud/internal/rest_context.h b/google/cloud/internal/rest_context.h index 482eb788e0d9c..266cab988c07b 100644 --- a/google/cloud/internal/rest_context.h +++ b/google/cloud/internal/rest_context.h @@ -15,6 +15,7 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_REST_CONTEXT_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_REST_CONTEXT_H +#include "google/cloud/options.h" #include "google/cloud/version.h" #include "absl/types/optional.h" #include @@ -36,7 +37,13 @@ class RestContext { public: using HttpHeaders = std::unordered_map>; RestContext() = default; - explicit RestContext(HttpHeaders headers) : headers_(std::move(headers)) {} + explicit RestContext(Options options, HttpHeaders headers) + : options_(std::move(options)), headers_(std::move(headers)) {} + explicit RestContext(Options options) : RestContext(std::move(options), {}) {} + explicit RestContext(HttpHeaders headers) + : RestContext({}, std::move(headers)) {} + + Options const& options() const { return options_; } HttpHeaders const& headers() const { return headers_; } @@ -104,6 +111,7 @@ class RestContext { private: friend bool operator==(RestContext const& lhs, RestContext const& rhs); + Options options_; HttpHeaders headers_; absl::optional local_ip_address_; absl::optional local_port_; diff --git a/google/cloud/storage/internal/bucket_acl_requests.h b/google/cloud/storage/internal/bucket_acl_requests.h index 1741c5b5f9f91..9d675af692f43 100644 --- a/google/cloud/storage/internal/bucket_acl_requests.h +++ b/google/cloud/storage/internal/bucket_acl_requests.h @@ -152,6 +152,7 @@ std::ostream& operator<<(std::ostream& os, UpdateBucketAclRequest const& r); class PatchBucketAclRequest : public GenericBucketAclRequest { public: + PatchBucketAclRequest() = default; PatchBucketAclRequest(std::string bucket, std::string entity, BucketAccessControl const& original, BucketAccessControl const& new_acl); diff --git a/google/cloud/storage/internal/bucket_requests.cc b/google/cloud/storage/internal/bucket_requests.cc index e560a6e175df1..445c9e88e3bd2 100644 --- a/google/cloud/storage/internal/bucket_requests.cc +++ b/google/cloud/storage/internal/bucket_requests.cc @@ -302,6 +302,10 @@ std::ostream& operator<<(std::ostream& os, GetBucketIamPolicyRequest const& r) { return os << "}"; } +SetNativeBucketIamPolicyRequest::SetNativeBucketIamPolicyRequest() + : SetNativeBucketIamPolicyRequest( + std::string{}, NativeIamPolicy(std::vector{})) {} + SetNativeBucketIamPolicyRequest::SetNativeBucketIamPolicyRequest( std::string bucket_name, NativeIamPolicy const& policy) : bucket_name_(std::move(bucket_name)), diff --git a/google/cloud/storage/internal/bucket_requests.h b/google/cloud/storage/internal/bucket_requests.h index 28db87394742c..225fd0ba7d3d8 100644 --- a/google/cloud/storage/internal/bucket_requests.h +++ b/google/cloud/storage/internal/bucket_requests.h @@ -213,6 +213,7 @@ std::ostream& operator<<(std::ostream& os, GetBucketIamPolicyRequest const& r); class SetNativeBucketIamPolicyRequest : public GenericRequest { public: + SetNativeBucketIamPolicyRequest(); explicit SetNativeBucketIamPolicyRequest(std::string bucket_name, NativeIamPolicy const& policy); diff --git a/google/cloud/storage/internal/default_object_acl_requests.h b/google/cloud/storage/internal/default_object_acl_requests.h index 412761f146c96..1ba439793e246 100644 --- a/google/cloud/storage/internal/default_object_acl_requests.h +++ b/google/cloud/storage/internal/default_object_acl_requests.h @@ -168,6 +168,7 @@ std::ostream& operator<<(std::ostream& os, class PatchDefaultObjectAclRequest : public GenericDefaultObjectAclRequest { public: + PatchDefaultObjectAclRequest() = default; PatchDefaultObjectAclRequest(std::string bucket, std::string entity, ObjectAccessControl const& original, ObjectAccessControl const& new_acl); diff --git a/google/cloud/storage/internal/hmac_key_requests.h b/google/cloud/storage/internal/hmac_key_requests.h index 55f403867eb1b..ec0addb40cdb0 100644 --- a/google/cloud/storage/internal/hmac_key_requests.h +++ b/google/cloud/storage/internal/hmac_key_requests.h @@ -118,6 +118,7 @@ class ListHmacKeysRequest : public GenericHmacKeyRequest { public: + ListHmacKeysRequest() = default; explicit ListHmacKeysRequest(std::string project_id) : GenericHmacKeyRequest(std::move(project_id)) {} @@ -150,6 +151,7 @@ std::ostream& operator<<(std::ostream& os, ListHmacKeysResponse const& r); class DeleteHmacKeyRequest : public GenericHmacKeyRequest { public: + DeleteHmacKeyRequest() = default; explicit DeleteHmacKeyRequest(std::string project_id, std::string access_id) : GenericHmacKeyRequest(std::move(project_id)), access_id_(std::move(access_id)) {} @@ -165,6 +167,7 @@ std::ostream& operator<<(std::ostream& os, DeleteHmacKeyRequest const& r); /// Represents a request to call the `HmacKeys: get` API. class GetHmacKeyRequest : public GenericHmacKeyRequest { public: + GetHmacKeyRequest() = default; explicit GetHmacKeyRequest(std::string project_id, std::string access_id) : GenericHmacKeyRequest(std::move(project_id)), access_id_(std::move(access_id)) {} @@ -181,6 +184,7 @@ std::ostream& operator<<(std::ostream& os, GetHmacKeyRequest const& r); class UpdateHmacKeyRequest : public GenericHmacKeyRequest { public: + UpdateHmacKeyRequest() = default; explicit UpdateHmacKeyRequest(std::string project_id, std::string access_id, HmacKeyMetadata resource) : GenericHmacKeyRequest(std::move(project_id)), diff --git a/google/cloud/storage/internal/object_acl_requests.h b/google/cloud/storage/internal/object_acl_requests.h index 1f8037485e3de..bb1ed00e7e168 100644 --- a/google/cloud/storage/internal/object_acl_requests.h +++ b/google/cloud/storage/internal/object_acl_requests.h @@ -152,6 +152,7 @@ std::ostream& operator<<(std::ostream& os, UpdateObjectAclRequest const& r); class PatchObjectAclRequest : public GenericObjectAclRequest { public: + PatchObjectAclRequest() = default; PatchObjectAclRequest(std::string bucket, std::string object, std::string entity, ObjectAccessControl const& original, ObjectAccessControl const& new_acl); diff --git a/google/cloud/storage/internal/object_requests.cc b/google/cloud/storage/internal/object_requests.cc index c7da9a28bad0e..68362dc40a380 100644 --- a/google/cloud/storage/internal/object_requests.cc +++ b/google/cloud/storage/internal/object_requests.cc @@ -161,6 +161,9 @@ std::ostream& operator<<(std::ostream& os, GetObjectMetadataRequest const& r) { return os << "}"; } +InsertObjectMediaRequest::InsertObjectMediaRequest() + : hash_function_(CreateHashFunction(*this)) {} + InsertObjectMediaRequest::InsertObjectMediaRequest(std::string bucket_name, std::string object_name, absl::string_view payload) diff --git a/google/cloud/storage/internal/object_requests.h b/google/cloud/storage/internal/object_requests.h index 1989545356b93..a94f7396c9094 100644 --- a/google/cloud/storage/internal/object_requests.h +++ b/google/cloud/storage/internal/object_requests.h @@ -111,8 +111,7 @@ class InsertObjectMediaRequest MD5HashValue, PredefinedAcl, Projection, UserProject, UploadFromOffset, UploadLimit, WithObjectMetadata> { public: - InsertObjectMediaRequest() = default; - + InsertObjectMediaRequest(); InsertObjectMediaRequest(std::string bucket_name, std::string object_name, absl::string_view payload); diff --git a/google/cloud/storage/internal/rest_client.cc b/google/cloud/storage/internal/rest_client.cc index b9d159998d3af..e3157d6e3c663 100644 --- a/google/cloud/storage/internal/rest_client.cc +++ b/google/cloud/storage/internal/rest_client.cc @@ -112,6 +112,8 @@ StatusOr CreateFromJson( Status AddAuthorizationHeader(Options const& options, RestRequestBuilder& builder) { + // In tests this option may not be set. And over time we want to retire it. + if (!options.has()) return {}; auto auth_header = options.get()->AuthorizationHeader(); if (!auth_header) return AuthHeaderError(std::move(auth_header).status()); @@ -186,7 +188,7 @@ StatusOr RestClient::ListBuckets( if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); builder.AddQueryParameter("project", request.project_id()); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -202,7 +204,7 @@ StatusOr RestClient::CreateBucket( builder.AddQueryParameter("project", request.project_id()); builder.AddHeader("Content-Type", "application/json"); auto payload = request.json_payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); auto response = CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -226,7 +228,7 @@ StatusOr RestClient::GetBucketMetadata( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -240,7 +242,7 @@ StatusOr RestClient::DeleteBucket( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } @@ -256,7 +258,7 @@ StatusOr RestClient::UpdateBucket( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.json_payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -273,7 +275,7 @@ StatusOr RestClient::PatchBucket( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Patch(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -288,7 +290,7 @@ StatusOr RestClient::GetNativeBucketIamPolicy( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CreateFromJson( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -304,7 +306,7 @@ StatusOr RestClient::SetNativeBucketIamPolicy( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto const& payload = request.json_payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CreateFromJson( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -322,7 +324,7 @@ StatusOr RestClient::TestBucketIamPermissions( builder.AddQueryParameter("permissions", p); } request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -338,7 +340,7 @@ StatusOr RestClient::LockBucketRetentionPolicy( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); builder.AddOption(IfMetagenerationMatch(request.metageneration())); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(std::string{})})); @@ -411,7 +413,7 @@ StatusOr RestClient::InsertObjectMediaMultipart( auto trailer = crlf + marker + "--" + crlf; // 6. Return the results as usual. - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString(storage_rest_client_->Post( context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(header), absl::MakeConstSpan(request.payload()), @@ -439,7 +441,7 @@ StatusOr RestClient::InsertObjectMediaSimple( } builder.AddQueryParameter("uploadType", "media"); builder.AddQueryParameter("name", request.object_name()); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(request.payload())})); @@ -485,7 +487,7 @@ StatusOr RestClient::CopyObject( .dump(); } - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(json_payload)})); @@ -500,7 +502,7 @@ StatusOr RestClient::GetObjectMetadata( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -523,7 +525,7 @@ StatusOr> RestClient::ReadObject( builder.AddHeader("Cache-Control", "no-transform"); } - rest_internal::RestContext context; + rest_internal::RestContext context(current); auto response = storage_rest_client_->Get(context, std::move(builder).BuildRequest()); if (!response.ok()) return response.status(); @@ -542,7 +544,7 @@ StatusOr RestClient::ListObjects( if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); builder.AddQueryParameter("pageToken", request.page_token()); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -556,7 +558,7 @@ StatusOr RestClient::DeleteObject( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } @@ -572,7 +574,7 @@ StatusOr RestClient::UpdateObject( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.json_payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -589,7 +591,7 @@ StatusOr RestClient::PatchObject( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Patch(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -607,7 +609,7 @@ StatusOr RestClient::ComposeObject( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.JsonPayload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -635,7 +637,7 @@ StatusOr RestClient::RewriteObject( .dump(); } - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(json_payload)})); @@ -681,7 +683,7 @@ StatusOr RestClient::CreateResumableUpload( std::string request_payload; if (!resource.empty()) request_payload = resource.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(request_payload)})); @@ -702,7 +704,7 @@ StatusOr RestClient::QueryResumableUpload( code >= rest::HttpStatusCode::kMinNotSuccess); }; - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {}), failure_predicate); @@ -721,7 +723,7 @@ StatusOr RestClient::DeleteResumableUpload( code >= rest::HttpStatusCode::kMinNotSuccess); }; - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest()), failure_predicate); @@ -752,7 +754,7 @@ StatusOr RestClient::UploadChunk( code >= rest::HttpStatusCode::kMinNotSuccess); }; - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), request.payload()), @@ -768,7 +770,7 @@ StatusOr RestClient::ListBucketAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -782,7 +784,7 @@ StatusOr RestClient::GetBucketAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -801,7 +803,7 @@ StatusOr RestClient::CreateBucketAcl( object["entity"] = request.entity(); object["role"] = request.role(); auto payload = object.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -816,7 +818,7 @@ StatusOr RestClient::DeleteBucketAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } @@ -835,7 +837,7 @@ StatusOr RestClient::UpdateBucketAcl( object["entity"] = request.entity(); object["role"] = request.role(); auto payload = object.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -852,7 +854,7 @@ StatusOr RestClient::PatchBucketAcl( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Patch(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -868,7 +870,7 @@ StatusOr RestClient::ListObjectAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -888,7 +890,7 @@ StatusOr RestClient::CreateObjectAcl( object["entity"] = request.entity(); object["role"] = request.role(); auto payload = object.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -904,7 +906,7 @@ StatusOr RestClient::DeleteObjectAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } @@ -919,7 +921,7 @@ StatusOr RestClient::GetObjectAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -939,7 +941,7 @@ StatusOr RestClient::UpdateObjectAcl( object["entity"] = request.entity(); object["role"] = request.role(); auto payload = object.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -957,7 +959,7 @@ StatusOr RestClient::PatchObjectAcl( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Patch(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -972,7 +974,7 @@ StatusOr RestClient::ListDefaultObjectAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -991,7 +993,7 @@ StatusOr RestClient::CreateDefaultObjectAcl( object["entity"] = request.entity(); object["role"] = request.role(); auto payload = object.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -1007,7 +1009,7 @@ StatusOr RestClient::DeleteDefaultObjectAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } @@ -1022,7 +1024,7 @@ StatusOr RestClient::GetDefaultObjectAcl( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -1042,7 +1044,7 @@ StatusOr RestClient::UpdateDefaultObjectAcl( object["entity"] = request.entity(); object["role"] = request.role(); auto payload = object.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -1060,7 +1062,7 @@ StatusOr RestClient::PatchDefaultObjectAcl( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Patch(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -1075,7 +1077,7 @@ StatusOr RestClient::GetServiceAccount( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -1089,7 +1091,7 @@ StatusOr RestClient::ListHmacKeys( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -1104,7 +1106,7 @@ StatusOr RestClient::CreateHmacKey( if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); builder.AddQueryParameter("serviceAccountEmail", request.service_account()); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Post( context, std::move(builder).BuildRequest(), @@ -1120,7 +1122,7 @@ StatusOr RestClient::DeleteHmacKey( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } @@ -1134,7 +1136,7 @@ StatusOr RestClient::GetHmacKey( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -1157,7 +1159,7 @@ StatusOr RestClient::UpdateHmacKey( } builder.AddHeader("Content-Type", "application/json"); auto payload = json_payload.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Put(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -1177,7 +1179,7 @@ StatusOr RestClient::SignBlob( } builder.AddHeader("Content-Type", "application/json"); auto payload = json_payload.dump(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( iam_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -1192,7 +1194,7 @@ StatusOr RestClient::ListNotifications( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ParseFromRestResponse( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -1208,7 +1210,7 @@ StatusOr RestClient::CreateNotification( request.AddOptionsToHttpRequest(builder); builder.AddHeader("Content-Type", "application/json"); auto payload = request.json_payload(); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Post(context, std::move(builder).BuildRequest(), {absl::MakeConstSpan(payload)})); @@ -1224,7 +1226,7 @@ StatusOr RestClient::GetNotification( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return CheckedFromString( storage_rest_client_->Get(context, std::move(builder).BuildRequest())); } @@ -1239,7 +1241,7 @@ StatusOr RestClient::DeleteNotification( auto auth = AddAuthorizationHeader(current, builder); if (!auth.ok()) return auth; request.AddOptionsToHttpRequest(builder); - rest_internal::RestContext context; + rest_internal::RestContext context(current); return ReturnEmptyResponse( storage_rest_client_->Delete(context, std::move(builder).BuildRequest())); } diff --git a/google/cloud/storage/internal/rest_client_test.cc b/google/cloud/storage/internal/rest_client_test.cc index 4610bfe37bf7e..6afd43bfeb16f 100644 --- a/google/cloud/storage/internal/rest_client_test.cc +++ b/google/cloud/storage/internal/rest_client_test.cc @@ -13,6 +13,9 @@ // limitations under the License. #include "google/cloud/storage/internal/rest_client.h" +#include "google/cloud/storage/testing/canonical_errors.h" +#include "google/cloud/testing_util/mock_rest_client.h" +#include "google/cloud/testing_util/status_matchers.h" #include #include @@ -23,7 +26,20 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace internal { namespace { +using ::google::cloud::internal::OptionsSpan; +using ::google::cloud::rest_internal::RestContext; +using ::google::cloud::rest_internal::RestRequest; +using ::google::cloud::storage::testing::canonical_errors::PermanentError; +using ::google::cloud::testing_util::MockRestClient; +using ::google::cloud::testing_util::StatusIs; +using ::testing::_; +using ::testing::An; +using ::testing::ElementsAre; using ::testing::Eq; +using ::testing::HasSubstr; +using ::testing::Matcher; +using ::testing::ResultOf; +using ::testing::Return; TEST(RestClientTest, ResolveStorageAuthorityProdEndpoint) { auto options = @@ -85,6 +101,632 @@ TEST(RestClientTest, ResolveIamAuthorityOptionSpecified) { EXPECT_THAT(result_options.get(), Eq("auth_option_set")); } +Options TestOptions() { + return Options{} + .set({"p1/v1", "p2/v2"}) + .set("vTest"); +} + +Matcher ExpectedContext() { + return ResultOf( + "context includes UserAgentProductsOption", + [](RestContext& context) { + return context.options().get(); + }, + ElementsAre("p1/v1", "p2/v2")); +} + +Matcher ExpectedRequest() { + return ResultOf( + "request includes TargetApiVersionOption", + [](RestRequest const& r) { return r.path(); }, + HasSubstr("storage/vTest/")); +} + +Matcher> const&> ExpectedPayload() { + return An> const&>(); +} + +TEST(RestClientTest, ListBuckets) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(TestOptions(), mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListBuckets(ListBucketsRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateBucket) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateBucket(CreateBucketRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetBucketMetadata) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetBucketMetadata(GetBucketMetadataRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteBucket) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteBucket(DeleteBucketRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UpdateBucket) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UpdateBucket(UpdateBucketRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, PatchBucket) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Patch(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->PatchBucket(PatchBucketRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetNativeBucketIamPolicy) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetNativeBucketIamPolicy(GetBucketIamPolicyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, SetNativeBucketIamPolicy) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = + tested->SetNativeBucketIamPolicy(SetNativeBucketIamPolicyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, TestBucketIamPermissions) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = + tested->TestBucketIamPermissions(TestBucketIamPermissionsRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, LockBucketRetentionPolicy) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = + tested->LockBucketRetentionPolicy(LockBucketRetentionPolicyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, InsertObjectMedia) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->InsertObjectMedia(InsertObjectMediaRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetObjectMetadata) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetObjectMetadata(GetObjectMetadataRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ReadObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ReadObject(ReadObjectRangeRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ListObjects) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListObjects(ListObjectsRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteObject(DeleteObjectRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UpdateObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UpdateObject(UpdateObjectRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, PatchObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Patch(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->PatchObject(PatchObjectRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ComposeObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ComposeObject(ComposeObjectRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateResumableUpload) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateResumableUpload(ResumableUploadRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, QueryResumableUpload) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), _, _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->QueryResumableUpload(QueryResumableUploadRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteResumableUpload) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteResumableUpload(DeleteResumableUploadRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UploadChunk) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), _, _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UploadChunk(UploadChunkRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ListBucketAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListBucketAcl(ListBucketAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CopyObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CopyObject(CopyObjectRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateBucketAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateBucketAcl(CreateBucketAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetBucketAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetBucketAcl(GetBucketAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteBucketAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteBucketAcl(DeleteBucketAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UpdateBucketAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UpdateBucketAcl(UpdateBucketAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, PatchBucketAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Patch(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->PatchBucketAcl(PatchBucketAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ListObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListObjectAcl(ListObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateObjectAcl(CreateObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteObjectAcl(DeleteObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetObjectAcl(GetObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UpdateObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UpdateObjectAcl(UpdateObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, PatchObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Patch(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->PatchObjectAcl(PatchObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, RewriteObject) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->RewriteObject(RewriteObjectRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ListDefaultObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListDefaultObjectAcl(ListDefaultObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateDefaultObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateDefaultObjectAcl(CreateDefaultObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteDefaultObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteDefaultObjectAcl(DeleteDefaultObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetDefaultObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetDefaultObjectAcl(GetDefaultObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UpdateDefaultObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UpdateDefaultObjectAcl(UpdateDefaultObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, PatchDefaultObjectAcl) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Patch(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->PatchDefaultObjectAcl(PatchDefaultObjectAclRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetServiceAccount) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetServiceAccount(GetProjectServiceAccountRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ListHmacKeys) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListHmacKeys(ListHmacKeysRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateHmacKey) { + auto mock = std::make_shared(); + auto expected_payload = + An> const&>(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), expected_payload)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateHmacKey(CreateHmacKeyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteHmacKey) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteHmacKey(DeleteHmacKeyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetHmacKey) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetHmacKey(GetHmacKeyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, UpdateHmacKey) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), ExpectedRequest(), _)) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->UpdateHmacKey(UpdateHmacKeyRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, SignBlob) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Post(ExpectedContext(), _, ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->SignBlob(SignBlobRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, ListNotifications) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->ListNotifications(ListNotificationsRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, CreateNotification) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, + Post(ExpectedContext(), ExpectedRequest(), ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->CreateNotification(CreateNotificationRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, GetNotification) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Get(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->GetNotification(GetNotificationRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestClientTest, DeleteNotification) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Delete(ExpectedContext(), ExpectedRequest())) + .WillOnce(Return(PermanentError())); + auto tested = RestClient::Create(Options{}, mock, mock); + OptionsSpan span(TestOptions()); + auto status = tested->DeleteNotification(DeleteNotificationRequest()); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/internal/sign_blob_requests.h b/google/cloud/storage/internal/sign_blob_requests.h index 99779143de34e..977c94a43da44 100644 --- a/google/cloud/storage/internal/sign_blob_requests.h +++ b/google/cloud/storage/internal/sign_blob_requests.h @@ -48,6 +48,7 @@ namespace internal { class SignBlobRequest : public internal::GenericRequestBase { public: + SignBlobRequest() = default; explicit SignBlobRequest(std::string service_account, std::string base64_encoded_blob, std::vector delegates) From 290774ab2c8a4a102aa5f5487ba21ebbbc54745e Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Sat, 29 Apr 2023 19:09:01 +0000 Subject: [PATCH 2/2] Fix host header computation --- google/cloud/internal/curl_rest_client.cc | 83 +++++++++---------- google/cloud/internal/curl_rest_client.h | 9 +- .../curl_rest_client_integration_test.cc | 4 +- 3 files changed, 47 insertions(+), 49 deletions(-) diff --git a/google/cloud/internal/curl_rest_client.cc b/google/cloud/internal/curl_rest_client.cc index ccc972f22ee00..3c02852698610 100644 --- a/google/cloud/internal/curl_rest_client.cc +++ b/google/cloud/internal/curl_rest_client.cc @@ -94,33 +94,38 @@ std::string CurlRestClient::HostHeader(Options const& options, return {}; } -CurlRestClient::CurlRestClient( - std::string endpoint_address, std::shared_ptr factory, - std::shared_ptr credentials) +CurlRestClient::CurlRestClient(std::string endpoint_address, + std::shared_ptr factory, + Options options) : endpoint_address_(std::move(endpoint_address)), handle_factory_(std::move(factory)), x_goog_api_client_header_("x-goog-api-client: " + google::cloud::internal::ApiClientHeader()), - credentials_(std::move(credentials)) {} + options_(std::move(options)) { + if (options_.has()) { + credentials_ = MapCredentials(options_.get()); + } +} StatusOr> CurlRestClient::CreateCurlImpl( - RestContext const& context, RestRequest const& request) { + RestContext const& context, RestRequest const& request, + Options const& options) { auto handle = CurlHandle::MakeFromPool(*handle_factory_); - auto impl = std::make_unique(std::move(handle), handle_factory_, - context.options()); + auto impl = + std::make_unique(std::move(handle), handle_factory_, options); if (credentials_) { auto auth_header = oauth2_internal::AuthorizationHeader(*credentials_); if (!auth_header.ok()) return std::move(auth_header).status(); impl->SetHeader(auth_header.value()); } - impl->SetHeader(HostHeader(context.options(), endpoint_address_)); + impl->SetHeader(HostHeader(options, endpoint_address_)); impl->SetHeader(x_goog_api_client_header_); impl->SetHeaders(context, request); RestRequest::HttpParameters additional_parameters; // The UserIp option has been deprecated in favor of quotaUser. Only add the // parameter if the option has been set. - if (context.options().has()) { - auto user_ip = context.options().get(); + if (options.has()) { + auto user_ip = options.get(); if (user_ip.empty()) user_ip = impl->LastClientIpAddress(); if (!user_ip.empty()) additional_parameters.emplace_back("userIp", user_ip); } @@ -136,53 +141,58 @@ StatusOr> CurlRestClient::CreateCurlImpl( // CreateCurlImpl relies heavily on member variables. StatusOr> CurlRestClient::Delete( RestContext& context, RestRequest const& request) { - auto impl = CreateCurlImpl(context, request); + auto options = internal::MergeOptions(context.options(), options_); + auto impl = CreateCurlImpl(context, request, options); if (!impl.ok()) return impl.status(); auto response = (*impl)->MakeRequest(CurlImpl::HttpMethod::kDelete, context); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(context.options(), std::move(*impl)))}; + new CurlRestResponse(std::move(options), std::move(*impl)))}; } StatusOr> CurlRestClient::Get( RestContext& context, RestRequest const& request) { - auto impl = CreateCurlImpl(context, request); + auto options = internal::MergeOptions(context.options(), options_); + auto impl = CreateCurlImpl(context, request, options); if (!impl.ok()) return impl.status(); auto response = (*impl)->MakeRequest(CurlImpl::HttpMethod::kGet, context); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(context.options(), std::move(*impl)))}; + new CurlRestResponse(std::move(options), std::move(*impl)))}; } StatusOr> CurlRestClient::Patch( RestContext& context, RestRequest const& request, std::vector> const& payload) { - auto impl = CreateCurlImpl(context, request); + auto options = internal::MergeOptions(context.options(), options_); + auto impl = CreateCurlImpl(context, request, options); if (!impl.ok()) return impl.status(); Status response = MakeRequestWithPayload(CurlImpl::HttpMethod::kPatch, context, request, **impl, payload); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(context.options(), std::move(*impl)))}; + new CurlRestResponse(std::move(options), std::move(*impl)))}; } StatusOr> CurlRestClient::Post( RestContext& context, RestRequest const& request, std::vector> const& payload) { - auto impl = CreateCurlImpl(context, request); + auto options = internal::MergeOptions(context.options(), options_); + auto impl = CreateCurlImpl(context, request, options); if (!impl.ok()) return impl.status(); Status response = MakeRequestWithPayload(CurlImpl::HttpMethod::kPost, context, request, **impl, payload); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(context.options(), std::move(*impl)))}; + new CurlRestResponse(std::move(options), std::move(*impl)))}; } StatusOr> CurlRestClient::Post( RestContext& context, RestRequest const& request, std::vector> const& form_data) { context.AddHeader("content-type", "application/x-www-form-urlencoded"); - auto impl = CreateCurlImpl(context, request); + auto options = internal::MergeOptions(context.options(), options_); + auto impl = CreateCurlImpl(context, request, options); if (!impl.ok()) return impl.status(); std::string form_payload = absl::StrJoin( form_data, "&", @@ -194,57 +204,44 @@ StatusOr> CurlRestClient::Post( request, **impl, {form_payload}); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(context.options(), std::move(*impl)))}; + new CurlRestResponse(std::move(options), std::move(*impl)))}; } StatusOr> CurlRestClient::Put( RestContext& context, RestRequest const& request, std::vector> const& payload) { - auto impl = CreateCurlImpl(context, request); + auto options = internal::MergeOptions(context.options(), options_); + auto impl = CreateCurlImpl(context, request, options); if (!impl.ok()) return impl.status(); Status response = MakeRequestWithPayload(CurlImpl::HttpMethod::kPut, context, request, **impl, payload); if (!response.ok()) return response; return {std::unique_ptr( - new CurlRestResponse(context.options(), std::move(*impl)))}; + new CurlRestResponse(std::move(options), std::move(*impl)))}; } -std::unique_ptr MakeDefaultRestClient( - std::string endpoint_address, - // NOLINTNEXTLINE(performance-unnecessary-value-param) - Options options) { +std::unique_ptr MakeDefaultRestClient(std::string endpoint_address, + Options options) { auto factory = GetDefaultCurlHandleFactory(options); - - auto credentials = - options.has() - ? MapCredentials(options.get()) - : std::shared_ptr(); return MakeTracingRestClient(std::make_unique( - std::move(endpoint_address), std::move(factory), std::move(credentials))); + std::move(endpoint_address), std::move(factory), std::move(options))); } -std::unique_ptr MakePooledRestClient( - std::string endpoint_address, - // NOLINTNEXTLINE(performance-unnecessary-value-param) - Options options) { +std::unique_ptr MakePooledRestClient(std::string endpoint_address, + Options options) { std::size_t pool_size = kDefaultPooledCurlHandleFactorySize; if (options.has()) { pool_size = options.get(); } - auto credentials = - options.has() - ? MapCredentials(options.get()) - : std::shared_ptr(); - if (pool_size > 0) { auto pool = std::make_shared(pool_size, options); return MakeTracingRestClient(std::make_unique( - std::move(endpoint_address), std::move(pool), std::move(credentials))); + std::move(endpoint_address), std::move(pool), std::move(options))); } auto pool = std::make_shared(options); return MakeTracingRestClient(std::make_unique( - std::move(endpoint_address), std::move(pool), std::move(credentials))); + std::move(endpoint_address), std::move(pool), std::move(options))); } GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/internal/curl_rest_client.h b/google/cloud/internal/curl_rest_client.h index 78826980adc0f..cbaf7bf7abfe9 100644 --- a/google/cloud/internal/curl_rest_client.h +++ b/google/cloud/internal/curl_rest_client.h @@ -43,8 +43,7 @@ class CurlRestClient : public RestClient { static std::string HostHeader(Options const& options, std::string const& endpoint); CurlRestClient(std::string endpoint_address, - std::shared_ptr factory, - std::shared_ptr credentials); + std::shared_ptr factory, Options options); ~CurlRestClient() override = default; CurlRestClient(CurlRestClient const&) = delete; @@ -71,13 +70,15 @@ class CurlRestClient : public RestClient { std::vector> const& payload) override; private: - StatusOr> CreateCurlImpl( - RestContext const& context, RestRequest const& request); + StatusOr> CreateCurlImpl(RestContext const& context, + RestRequest const& request, + Options const& options); std::string endpoint_address_; std::shared_ptr handle_factory_; std::string x_goog_api_client_header_; std::shared_ptr credentials_; + Options options_; }; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/internal/curl_rest_client_integration_test.cc b/google/cloud/internal/curl_rest_client_integration_test.cc index 01097f2ecd3c2..32903c84cf725 100644 --- a/google/cloud/internal/curl_rest_client_integration_test.cc +++ b/google/cloud/internal/curl_rest_client_integration_test.cc @@ -155,13 +155,13 @@ TEST_F(RestClientIntegrationTest, Get) { TEST_F(RestClientIntegrationTest, Delete) { options_.set(MakeInsecureCredentials()); + options_.set("127.0.0.1"); auto client = MakeDefaultRestClient(url_, options_); RestRequest request; request.SetPath("delete"); request.AddQueryParameter({"key", "value"}); auto response_status = RetryRestRequest([&] { - rest_internal::RestContext context( - Options{}.set("127.0.0.1")); + rest_internal::RestContext context; return client->Delete(context, request); }); ASSERT_STATUS_OK(response_status);