cleanup: rest client style cleanups (#8451)

* cleanup: rest client style cleanups

1.
   https://github.com/googleapis/google-cloud-cpp/blob/541eb09c32c91c09ea84a55fc0861e916ebec56a/google/cloud/internal/curl_impl.h#L40-L42
   declares a static object (i.e., internal linkage) in a header. We
   want to avoid that because it can lead to ODR issues. See
   https://abseil.io/tips/140 for more details.

2.
   https://github.com/googleapis/google-cloud-cpp/blob/541eb09c32c91c09ea84a55fc0861e916ebec56a/google/cloud/internal/rest_client.h#L38-L44
   These factory functions should be named "Make..." not "Get...". For
   functions that create things, we use the verb "Make", which follows
   the standard's naming (e.g., make_pair).

Author: devjgm

google/cloud/internal/curl_impl.cc

@@ -620,7 +620,7 @@ Status CurlImpl::MakeRequestImpl() {
   TRACE_STATE() << "url_ " << url_ << "\n";
   // Setting BUFFERSIZE is a request, not an order. libcurl can and will write
   // fewer bytes as it wishes.
-  handle_.SetOption(CURLOPT_BUFFERSIZE, kDefaultCurlWriteBufferSize);
+  handle_.SetOption(CURLOPT_BUFFERSIZE, spill_.max_size());
   handle_.SetOption(CURLOPT_URL, url_.c_str());
   handle_.SetOption(CURLOPT_HTTPHEADER, request_headers_.get());
   handle_.SetOption(CURLOPT_USERAGENT, user_agent_.c_str());

google/cloud/internal/curl_impl.h

@@ -37,10 +37,6 @@ extern "C" std::size_t CurlRequestWrite(char* ptr, size_t size, size_t nmemb,
 extern "C" std::size_t CurlRequestHeader(char* contents, std::size_t size,
                                          std::size_t nitems, void* userdata);
 
-// We get better performance using a slightly larger buffer (128KiB) than the
-// default buffer size set by libcurl (16KiB).
-static auto constexpr kDefaultCurlWriteBufferSize = 128 * 1024L;
-
 // This class encapsulates use of libcurl and manages all the necessary state
 // of a request and its associated response.
 class CurlImpl {
@@ -159,8 +155,9 @@ class CurlImpl {
   // WriteCallback. However, the callback *must* save all the bytes, returning
   // fewer bytes read aborts the download. The application may have requested
   // fewer bytes in the call to `Read()`, so we need a place to store the
-  // additional bytes.
-  std::array<char, kDefaultCurlWriteBufferSize> spill_;
+  // additional bytes. We get better performance using a slightly larger buffer
+  // (128KiB) than the default buffer size set by libcurl (16KiB).
+  std::array<char, 128 * 1024L> spill_;
   std::size_t spill_offset_;
 
   Options options_;

google/cloud/internal/curl_rest_client.cc

@@ -208,15 +208,15 @@ StatusOr<std::unique_ptr<RestResponse>> CurlRestClient::Put(
       new CurlRestResponse(options_, std::move(*impl)))};
 }
 
-std::unique_ptr<RestClient> GetDefaultRestClient(std::string endpoint_address,
-                                                 Options options) {
+std::unique_ptr<RestClient> MakeDefaultRestClient(std::string endpoint_address,
+                                                  Options options) {
   auto factory = GetDefaultCurlHandleFactory(options);
   return std::unique_ptr<RestClient>(new CurlRestClient(
       std::move(endpoint_address), std::move(factory), std::move(options)));
 }
 
-std::unique_ptr<RestClient> GetPooledRestClient(std::string endpoint_address,
-                                                Options options) {
+std::unique_ptr<RestClient> MakePooledRestClient(std::string endpoint_address,
+                                                 Options options) {
   std::size_t pool_size = kDefaultPooledCurlHandleFactorySize;
   if (options.has<ConnectionPoolSizeOption>()) {
     pool_size = options.get<ConnectionPoolSizeOption>();

google/cloud/internal/curl_rest_client.h

@@ -65,10 +65,10 @@ class CurlRestClient : public RestClient {
       std::vector<absl::Span<char const>> const& payload) override;
 
  private:
-  friend std::unique_ptr<RestClient> GetDefaultRestClient(
+  friend std::unique_ptr<RestClient> MakeDefaultRestClient(
       std::string endpoint_address, Options options);
 
-  friend class std::unique_ptr<RestClient> GetPooledRestClient(
+  friend class std::unique_ptr<RestClient> MakePooledRestClient(
       std::string endpoint_address, Options options);
 
   CurlRestClient(std::string endpoint_address,

google/cloud/internal/curl_rest_client_integration_test.cc

@@ -133,7 +133,7 @@ class RestClientIntegrationTest : public ::testing::Test {
 
 TEST_F(RestClientIntegrationTest, Get) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, {});
+  auto client = MakeDefaultRestClient(url_, {});
   RestRequest request;
   request.SetPath("get");
   auto response_status = RetryRestRequest([&] { return client->Get(request); });
@@ -150,7 +150,7 @@ TEST_F(RestClientIntegrationTest, Get) {
 TEST_F(RestClientIntegrationTest, Delete) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
   options_.set<UserIpOption>("127.0.0.1");
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("delete");
   request.AddQueryParameter({"key", "value"});
@@ -177,7 +177,7 @@ TEST_F(RestClientIntegrationTest, PatchJsonContentType) {
     "client_email": "[email protected]",
 })""";
 
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("patch");
   request.AddQueryParameter({"type", "service_account"});
@@ -209,7 +209,7 @@ TEST_F(RestClientIntegrationTest, PatchJsonContentType) {
 
 TEST_F(RestClientIntegrationTest, AnythingPostNoContentType) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 
@@ -254,7 +254,7 @@ TEST_F(RestClientIntegrationTest, AnythingPostNoContentType) {
 
 TEST_F(RestClientIntegrationTest, AnythingPostJsonContentType) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 
@@ -268,7 +268,7 @@ TEST_F(RestClientIntegrationTest, AnythingPostJsonContentType) {
 
 TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeSingleSpan) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 
@@ -282,7 +282,7 @@ TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeSingleSpan) {
 
 TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeTwoSpans) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 
@@ -306,7 +306,7 @@ TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeTwoSpans) {
 
 TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeEmptyMiddleSpan) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 
@@ -328,7 +328,7 @@ TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeEmptyMiddleSpan) {
 
 TEST_F(RestClientIntegrationTest, AnythingPutJsonContentTypeEmptyFirstSpan) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 
@@ -356,7 +356,7 @@ TEST_F(RestClientIntegrationTest, ResponseBodyLargerThanSpillBuffer) {
   auto large_json_payload = json.dump();
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
   options_.set<ConnectionPoolSizeOption>(4);
-  auto client = GetPooledRestClient(url_, options_);
+  auto client = MakePooledRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
   request.AddHeader("content-type", "application/json");
@@ -370,7 +370,7 @@ TEST_F(RestClientIntegrationTest, ResponseBodyLargerThanSpillBuffer) {
 
 TEST_F(RestClientIntegrationTest, PostFormData) {
   options_.set<UnifiedCredentialsOption>(MakeInsecureCredentials());
-  auto client = GetDefaultRestClient(url_, options_);
+  auto client = MakeDefaultRestClient(url_, options_);
   RestRequest request;
   request.SetPath("anything");
 

google/cloud/internal/oauth2_authorized_user_credentials.cc

@@ -96,7 +96,7 @@ AuthorizedUserCredentials::AuthorizedUserCredentials(
       rest_client_(std::move(rest_client)) {
   if (!rest_client_) {
     rest_client_ =
-        rest_internal::GetDefaultRestClient(info_.token_uri, options_);
+        rest_internal::MakeDefaultRestClient(info_.token_uri, options_);
   }
 }
 

google/cloud/internal/oauth2_compute_engine_credentials.cc

@@ -93,7 +93,7 @@ ComputeEngineCredentials::ComputeEngineCredentials(
       options_(std::move(options)) {
   if (!rest_client_) {
     options_.set<rest_internal::CurlFollowLocationOption>(true);
-    rest_client_ = rest_internal::GetDefaultRestClient(
+    rest_client_ = rest_internal::MakeDefaultRestClient(
         "http://" + google::cloud::internal::GceMetadataHostname(), options_);
   }
 }

google/cloud/internal/oauth2_minimal_iam_credentials_rest.cc

@@ -40,7 +40,7 @@ MinimalIamCredentialsRestStub::MinimalIamCredentialsRestStub(
       rest_client_(std::move(rest_client)),
       options_(std::move(options)) {
   if (!rest_client_) {
-    rest_client_ = rest_internal::GetDefaultRestClient(endpoint_, options_);
+    rest_client_ = rest_internal::MakeDefaultRestClient(endpoint_, options_);
   }
 }
 

google/cloud/internal/oauth2_service_account_credentials.cc

@@ -173,11 +173,11 @@ ServiceAccountCredentials::ServiceAccountCredentials(
       options_(std::move(options)) {
   if (!rest_client_) {
     if (options_.has<ServiceAccountCredentialsTokenUriOption>()) {
-      rest_client_ = rest_internal::GetDefaultRestClient(
+      rest_client_ = rest_internal::MakeDefaultRestClient(
           options_.get<ServiceAccountCredentialsTokenUriOption>(), options_);
     } else {
       rest_client_ =
-          rest_internal::GetDefaultRestClient(info_.token_uri, options_);
+          rest_internal::MakeDefaultRestClient(info_.token_uri, options_);
     }
   }
 }

google/cloud/internal/rest_client.h

@@ -32,14 +32,14 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
 // Provides factory function to create a CurlRestClient that does not manage a
 // pool of connections.
-std::unique_ptr<RestClient> GetDefaultRestClient(std::string endpoint_address,
-                                                 Options options);
+std::unique_ptr<RestClient> MakeDefaultRestClient(std::string endpoint_address,
+                                                  Options options);
 
 // Provides factory function to create a CurlRestClient that manages a pool of
 // connections which are reused in order to minimize costs of setup and
 // teardown.
-std::unique_ptr<RestClient> GetPooledRestClient(std::string endpoint_address,
-                                                Options options);
+std::unique_ptr<RestClient> MakePooledRestClient(std::string endpoint_address,
+                                                 Options options);
 
 // Provides methods corresponding to HTTP verbs to make requests to RESTful
 // services.