Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: success predicates for rest_internal::RestResponse #10370

Merged
merged 2 commits into from
Dec 5, 2022
Merged

refactor: success predicates for rest_internal::RestResponse #10370

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5980fff
refactor: success predicates for `rest_internal::RestResponse`
coryan Dec 5, 2022
53d306b
Address review comments
coryan Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions google/cloud/internal/external_account_token_source_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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();
Expand Down
4 changes: 1 addition & 3 deletions google/cloud/internal/oauth2_authorized_user_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
9 changes: 2 additions & 7 deletions google/cloud/internal/oauth2_compute_engine_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down
4 changes: 1 addition & 3 deletions google/cloud/internal/oauth2_external_account_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down
6 changes: 1 addition & 5 deletions google/cloud/internal/oauth2_minimal_iam_credentials_rest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
3 changes: 1 addition & 2 deletions google/cloud/internal/oauth2_service_account_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
11 changes: 11 additions & 0 deletions google/cloud/internal/rest_response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like either kMinNotSuccess is lying, or that should be `<'. That is, "== kMinNotSuccess" shouldn't be a success. (??)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

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 {};
Expand Down
6 changes: 6 additions & 0 deletions google/cloud/internal/rest_response.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
*
Expand Down
24 changes: 24 additions & 0 deletions google/cloud/internal/rest_response_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -88,6 +89,29 @@ 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::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));
Expand Down
2 changes: 1 addition & 1 deletion google/cloud/storage/benchmarks/benchmark_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 5 additions & 16 deletions google/cloud/storage/internal/rest_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)));
}
Expand Down Expand Up @@ -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)));
}
Expand Down