refactor: success predicates for rest_internal::RestResponse (#10370)

We often need to determine if a `RestResponse` contains a successful
HTTP response or an error.  These helpers make the code easier to read
and provide more consistent treatment of `RestResponse`. Callers that
deviate from the standard success/failure codes can write their own
functions (e.g., GCS treats 308 as "success" in some RPCs).

google/cloud/internal/external_account_token_source_url.cc

@@ -28,11 +28,6 @@ namespace {
 // Represent the headers in the credentials configuration file.
 using Headers = std::map<std::string, std::string>;
 
-bool IsSuccess(rest_internal::HttpStatusCode code) {
-  return rest_internal::HttpStatusCode::kMinSuccess <= code &&
-         code <= rest_internal::kMinRedirects;
-}
-
 Status DecorateHttpError(Status const& status,
                          internal::ErrorContext const& ec) {
   auto builder = GCP_ERROR_INFO().WithContext(ec).WithReason("HTTP REQUEST");
@@ -53,7 +48,7 @@ StatusOr<std::string> FetchContents(HttpClientFactory const& client_factory,
   auto status = client->Get(request);
   if (!status) return DecorateHttpError(std::move(status).status(), ec);
   auto response = *std::move(status);
-  if (!IsSuccess(response->StatusCode())) {
+  if (IsHttpError(*response)) {
     return DecorateHttpError(rest_internal::AsStatus(std::move(*response)), ec);
   }
   auto payload = std::move(*response).ExtractPayload();

google/cloud/internal/oauth2_authorized_user_credentials.cc

@@ -108,9 +108,7 @@ StatusOr<internal::AccessToken> AuthorizedUserCredentials::GetToken(
   if (!response.ok()) return std::move(response).status();
   std::unique_ptr<rest_internal::RestResponse> real_response =
       std::move(response.value());
-  if (real_response->StatusCode() >= 300) {
-    return AsStatus(std::move(*real_response));
-  }
+  if (IsHttpError(*real_response)) return AsStatus(std::move(*real_response));
   return ParseAuthorizedUserRefreshResponse(*real_response, tp);
 }
 

google/cloud/internal/oauth2_compute_engine_credentials.cc

@@ -110,10 +110,7 @@ StatusOr<internal::AccessToken> ComputeEngineCredentials::GetToken(
       "computeMetadata/v1/instance/service-accounts/" + email + "/token",
       false);
   if (!response) return std::move(response).status();
-  if ((*response)->StatusCode() >= 300) {
-    return AsStatus(std::move(**response));
-  }
-
+  if (IsHttpError(**response)) return AsStatus(std::move(**response));
   return ParseComputeEngineRefreshResponse(**response, tp);
 }
 
@@ -157,9 +154,7 @@ std::string ComputeEngineCredentials::RetrieveServiceAccountInfo(
       "computeMetadata/v1/instance/service-accounts/" + service_account_email_ +
           "/",
       true);
-  if (!response || (*response)->StatusCode() >= 300) {
-    return service_account_email_;
-  }
+  if (!response || IsHttpError(**response)) return service_account_email_;
   auto metadata = ParseMetadataServerResponse(**response);
   if (!metadata) return service_account_email_;
   service_account_email_ = std::move(metadata->email);

google/cloud/internal/oauth2_external_account_credentials.cc

@@ -113,9 +113,7 @@ StatusOr<internal::AccessToken> ExternalAccountCredentials::GetToken(
   auto client = client_factory_(options_);
   auto response = client->Post(request, form_data);
   if (!response) return std::move(response).status();
-  if (MapHttpCodeToStatus((*response)->StatusCode()) != StatusCode::kOk) {
-    return AsStatus(std::move(**response));
-  }
+  if (IsHttpError(**response)) return AsStatus(std::move(**response));
   auto payload = rest_internal::ReadAll(std::move(**response).ExtractPayload());
   if (!payload) return std::move(payload.status());
 

google/cloud/internal/oauth2_minimal_iam_credentials_rest.cc

@@ -64,12 +64,8 @@ MinimalIamCredentialsRestStub::GenerateAccessToken(
   };
 
   auto response = rest_client_->Post(rest_request, {payload.dump()});
-
   if (!response) return std::move(response).status();
-  if ((*response)->StatusCode() >=
-      rest_internal::HttpStatusCode::kMinNotSuccess) {
-    return AsStatus(std::move(**response));
-  }
+  if (IsHttpError(**response)) return AsStatus(std::move(**response));
   auto response_payload =
       rest_internal::ReadAll(std::move(**response).ExtractPayload());
   if (!response_payload.ok()) return response_payload.status();

google/cloud/internal/oauth2_service_account_credentials.cc

@@ -357,8 +357,7 @@ StatusOr<internal::AccessToken> ServiceAccountCredentials::GetTokenOAuth(
   if (!response.ok()) return std::move(response).status();
   std::unique_ptr<rest_internal::RestResponse> real_response =
       std::move(response.value());
-  if (real_response->StatusCode() >= 300)
-    return AsStatus(std::move(*real_response));
+  if (IsHttpError(*real_response)) return AsStatus(std::move(*real_response));
   return ParseServiceAccountRefreshResponse(*real_response, tp);
 }
 

google/cloud/internal/rest_response.cc

@@ -136,6 +136,17 @@ StatusCode MapHttpCodeToStatus(std::int32_t code) {
   return StatusCode::kUnknown;
 }
 
+bool IsHttpSuccess(RestResponse const& response) {
+  static_assert(HttpStatusCode::kMinSuccess < HttpStatusCode::kMinNotSuccess,
+                "Invalid HTTP code success range");
+  return response.StatusCode() < HttpStatusCode::kMinNotSuccess &&
+         response.StatusCode() >= HttpStatusCode::kMinSuccess;
+}
+
+bool IsHttpError(RestResponse const& response) {
+  return !IsHttpSuccess(response);
+}
+
 Status AsStatus(HttpStatusCode http_status_code, std::string payload) {
   auto const status_code = MapHttpCodeToStatus(http_status_code);
   if (status_code == StatusCode::kOk) return {};

google/cloud/internal/rest_response.h

@@ -82,6 +82,12 @@ class RestResponse {
 /// Convert a HTTP status code to a google::cloud::StatusCode.
 StatusCode MapHttpCodeToStatus(std::int32_t code);
 
+/// Determines if @p response contains a successful result.
+bool IsHttpSuccess(RestResponse const& response);
+
+/// Determines if @p response contains an error.
+bool IsHttpError(RestResponse const& response);
+
 /**
  * Maps a response to a `Status`.
  *

google/cloud/internal/rest_response_test.cc

@@ -26,6 +26,7 @@ namespace {
 
 using ::google::cloud::testing_util::IsOk;
 using ::google::cloud::testing_util::MakeMockHttpPayloadSuccess;
+using ::google::cloud::testing_util::MockRestResponse;
 using ::google::cloud::testing_util::StatusIs;
 using ::testing::ByMove;
 using ::testing::Contains;
@@ -88,6 +89,30 @@ INSTANTIATE_TEST_SUITE_P(
       return std::to_string(std::get<0>(info.param));
     });
 
+TEST(RestResponse, IsHttpSuccessVsError) {
+  struct TestCase {
+    HttpStatusCode code;
+    bool expected;
+  } const cases[] = {
+      {HttpStatusCode::kOk, true},
+      {HttpStatusCode::kCreated, true},
+      {HttpStatusCode::kContinue, false},
+      {HttpStatusCode::kMinNotSuccess, false},
+      {HttpStatusCode::kForbidden, false},
+      {HttpStatusCode::kNotModified, false},
+  };
+
+  for (auto const test : cases) {
+    SCOPED_TRACE("Testing with " + std::to_string(test.code));
+    MockRestResponse mock;
+    EXPECT_CALL(mock, StatusCode).WillRepeatedly(Return(test.code));
+    auto const is_success = IsHttpSuccess(mock);
+    auto const is_error = IsHttpError(mock);
+    EXPECT_EQ(is_success, test.expected);
+    EXPECT_EQ(is_error, !test.expected);
+  }
+}
+
 TEST(AsStatus, RestResponseIsOk) {
   auto response = absl::make_unique<testing_util::MockRestResponse>();
   EXPECT_CALL(*response, StatusCode).WillOnce(Return(HttpStatusCode::kOk));

google/cloud/storage/benchmarks/benchmark_utils.cc

@@ -266,7 +266,7 @@ absl::optional<std::string> GetMetadata(RestClient& metadata_server,
   auto response = *std::move(response_status);
   auto const status_code = response->StatusCode();
   auto contents = ReadAll(std::move(*response).ExtractPayload());
-  if (status_code != 200) return absl::nullopt;
+  if (status_code != rest_internal::HttpStatusCode::kOk) return absl::nullopt;
   if (!contents) return absl::nullopt;
   // A lot of metadata attributes have the full resource name (e.e.,
   // projects/.../zones/..), we just want the last portion.

google/cloud/storage/internal/rest_client.cc

@@ -89,9 +89,7 @@ auto CheckedFromString(StatusOr<std::unique_ptr<rest::RestResponse>> response)
   if (!response.ok()) {
     return std::move(response).status();
   }
-  if ((*response)->StatusCode() >= rest::kMinNotSuccess) {
-    return rest::AsStatus(std::move(**response));
-  }
+  if (IsHttpError(**response)) return rest::AsStatus(std::move(**response));
   auto payload = rest::ReadAll(std::move(**response).ExtractPayload());
   if (!payload.ok()) return std::move(payload).status();
   return Parser::FromString(*payload);
@@ -114,9 +112,7 @@ template <typename ReturnType>
 StatusOr<ReturnType> CreateFromJson(
     StatusOr<std::unique_ptr<rest::RestResponse>> response) {
   if (!response.ok()) return std::move(response).status();
-  if ((*response)->StatusCode() >= rest::kMinNotSuccess) {
-    return rest::AsStatus(std::move(**response));
-  }
+  if (IsHttpError(**response)) return rest::AsStatus(std::move(**response));
   auto payload = rest::ReadAll(std::move(**response).ExtractPayload());
   if (!payload.ok()) return std::move(payload).status();
   return ReturnType::CreateFromJson(*payload);
@@ -494,9 +490,7 @@ StatusOr<ObjectMetadata> RestClient::InsertObjectMediaXml(
       storage_rest_client_->Put(std::move(builder).BuildRequest(),
                                 {absl::MakeConstSpan(request.contents())});
   if (!response.ok()) return std::move(response).status();
-  if ((*response)->StatusCode() >= rest::kMinNotSuccess) {
-    return rest::AsStatus(std::move(**response));
-  }
+  if (IsHttpError(**response)) return rest::AsStatus(std::move(**response));
   auto payload = rest::ReadAll(std::move(**response).ExtractPayload());
   if (!payload.ok()) return std::move(payload).status();
   return internal::ObjectMetadataParser::FromJson(nlohmann::json{
@@ -634,9 +628,7 @@ StatusOr<std::unique_ptr<ObjectReadSource>> RestClient::ReadObjectXml(
 
   auto response = storage_rest_client_->Get(std::move(builder).BuildRequest());
   if (!response.ok()) return std::move(response).status();
-  if (IsHttpError((*response)->StatusCode())) {
-    return rest::AsStatus(std::move(**response));
-  }
+  if (IsHttpError(**response)) return rest::AsStatus(std::move(**response));
   return std::unique_ptr<ObjectReadSource>(
       new RestObjectReadSource(*std::move(response)));
 }
@@ -669,10 +661,7 @@ StatusOr<std::unique_ptr<ObjectReadSource>> RestClient::ReadObject(
 
   auto response = storage_rest_client_->Get(std::move(builder).BuildRequest());
   if (!response.ok()) return response.status();
-  if (IsHttpError((*response)->StatusCode())) {
-    return rest::AsStatus(std::move(**response));
-  }
-
+  if (IsHttpError(**response)) return rest::AsStatus(std::move(**response));
   return std::unique_ptr<ObjectReadSource>(
       new RestObjectReadSource(*std::move(response)));
 }