diff --git a/google/cloud/internal/external_account_token_source_url.cc b/google/cloud/internal/external_account_token_source_url.cc index de49315b4e49..a84077b8392d 100644 --- a/google/cloud/internal/external_account_token_source_url.cc +++ b/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; -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 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(); diff --git a/google/cloud/internal/oauth2_authorized_user_credentials.cc b/google/cloud/internal/oauth2_authorized_user_credentials.cc index 35e2d4757569..58934d1789ff 100644 --- a/google/cloud/internal/oauth2_authorized_user_credentials.cc +++ b/google/cloud/internal/oauth2_authorized_user_credentials.cc @@ -108,9 +108,7 @@ StatusOr AuthorizedUserCredentials::GetToken( if (!response.ok()) return std::move(response).status(); std::unique_ptr 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); } diff --git a/google/cloud/internal/oauth2_compute_engine_credentials.cc b/google/cloud/internal/oauth2_compute_engine_credentials.cc index 0f1b3d42250d..192f53eddab9 100644 --- a/google/cloud/internal/oauth2_compute_engine_credentials.cc +++ b/google/cloud/internal/oauth2_compute_engine_credentials.cc @@ -110,10 +110,7 @@ StatusOr 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); diff --git a/google/cloud/internal/oauth2_external_account_credentials.cc b/google/cloud/internal/oauth2_external_account_credentials.cc index 4c4eb3716d47..f9a83b092300 100644 --- a/google/cloud/internal/oauth2_external_account_credentials.cc +++ b/google/cloud/internal/oauth2_external_account_credentials.cc @@ -113,9 +113,7 @@ StatusOr 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()); diff --git a/google/cloud/internal/oauth2_minimal_iam_credentials_rest.cc b/google/cloud/internal/oauth2_minimal_iam_credentials_rest.cc index dfad37824073..b26f3cf1b334 100644 --- a/google/cloud/internal/oauth2_minimal_iam_credentials_rest.cc +++ b/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(); diff --git a/google/cloud/internal/oauth2_service_account_credentials.cc b/google/cloud/internal/oauth2_service_account_credentials.cc index ee94b2f87436..2c5ae6b51689 100644 --- a/google/cloud/internal/oauth2_service_account_credentials.cc +++ b/google/cloud/internal/oauth2_service_account_credentials.cc @@ -357,8 +357,7 @@ StatusOr ServiceAccountCredentials::GetTokenOAuth( if (!response.ok()) return std::move(response).status(); std::unique_ptr 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); } diff --git a/google/cloud/internal/rest_response.cc b/google/cloud/internal/rest_response.cc index 171ff72a07e3..53383bfe2b77 100644 --- a/google/cloud/internal/rest_response.cc +++ b/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 {}; diff --git a/google/cloud/internal/rest_response.h b/google/cloud/internal/rest_response.h index d324e2e9a1f4..e34857d482e7 100644 --- a/google/cloud/internal/rest_response.h +++ b/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`. * diff --git a/google/cloud/internal/rest_response_test.cc b/google/cloud/internal/rest_response_test.cc index 9e6e96980b0c..0b8e5729f70a 100644 --- a/google/cloud/internal/rest_response_test.cc +++ b/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(); EXPECT_CALL(*response, StatusCode).WillOnce(Return(HttpStatusCode::kOk)); diff --git a/google/cloud/storage/benchmarks/benchmark_utils.cc b/google/cloud/storage/benchmarks/benchmark_utils.cc index eb4973c4e36e..e1898eff1720 100644 --- a/google/cloud/storage/benchmarks/benchmark_utils.cc +++ b/google/cloud/storage/benchmarks/benchmark_utils.cc @@ -266,7 +266,7 @@ absl::optional 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. diff --git a/google/cloud/storage/internal/rest_client.cc b/google/cloud/storage/internal/rest_client.cc index 38767984a1a9..c2aceaf0eae1 100644 --- a/google/cloud/storage/internal/rest_client.cc +++ b/google/cloud/storage/internal/rest_client.cc @@ -89,9 +89,7 @@ auto CheckedFromString(StatusOr> 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 StatusOr CreateFromJson( StatusOr> 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 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> 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( new RestObjectReadSource(*std::move(response))); } @@ -669,10 +661,7 @@ StatusOr> 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( new RestObjectReadSource(*std::move(response))); }