Skip to content

impl(rest): map http code 504 to kUnavailable #16077

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

Merged
scotthart merged 1 commit into from
Apr 13, 2026
+14 −3
Merged

impl(rest): map http code 504 to kUnavailable #16077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 9 additions & 2 deletions google/cloud/internal/rest_response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ StatusCode MapHttpCodeToStatus5xx(std::int32_t code) {
if (code == HttpStatusCode::kServiceUnavailable) {
return StatusCode::kUnavailable;
}
if (code == HttpStatusCode::kGatewayTimeout) {
return StatusCode::kUnavailable;
}
Comment on lines 120 to +125
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 10, 2026

Choose a reason for hiding this comment

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

medium

These checks can be combined for better readability. Additionally, it appears kBadGateway (502) is also expected to be mapped to kUnavailable based on the test cases (see line 83 of google/cloud/internal/rest_response_test.cc), but it is currently missing from this function. Consider adding it here.

  if (code == HttpStatusCode::kServiceUnavailable ||
      code == HttpStatusCode::kGatewayTimeout ||
      code == HttpStatusCode::kBadGateway) {
    return StatusCode::kUnavailable;
  }

// 5XX - server errors are mapped to kInternal.
return StatusCode::kInternal;
}
Expand Down Expand Up @@ -153,8 +156,12 @@ Status AsStatus(HttpStatusCode http_status_code, std::string payload) {
if (payload.empty()) {
// If there's no payload, create one to make sure the original http status
// code received is available.
return Status(status_code, "Received HTTP status code: " +
std::to_string(http_status_code));
ErrorInfo error_info{
{}, {}, {{"http_status_code", std::to_string(http_status_code)}}};
return Status(
status_code,
"Received HTTP status code: " + std::to_string(http_status_code),
std::move(error_info));
Comment on lines +159 to +164
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 10, 2026

Choose a reason for hiding this comment

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

medium

The HTTP status code is converted to a string twice. Consider converting it once to improve efficiency. Additionally, ensure to use std::move when passing the local ErrorInfo object to the Status constructor to avoid unnecessary copies, as per repository best practices.

    auto code_str = std::to_string(http_status_code);
    ErrorInfo error_info{{}, {}, {{"http_status_code", code_str}}};
    return Status(status_code, "Received HTTP status code: " + code_str,
                  std::move(error_info));
References
  1. When passing a local object to a method that takes it by value, use std::move to avoid unnecessary copies.

}
auto p =
ParseJsonError(static_cast<int>(http_status_code), std::move(payload));
Expand Down
1 change: 1 addition & 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 @@ -67,6 +67,7 @@ enum HttpStatusCode : std::int32_t {
kInternalServerError = 500,
kBadGateway = 502,
kServiceUnavailable = 503,
kGatewayTimeout = 504,
};

// This class contains the results of making a request to a RESTful service.
Expand Down
5 changes: 4 additions & 1 deletion google/cloud/internal/rest_response_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ INSTANTIATE_TEST_SUITE_P(
std::make_pair(HttpStatusCode::kBadGateway, StatusCode::kUnavailable),
std::make_pair(HttpStatusCode::kServiceUnavailable,
StatusCode::kUnavailable),
std::make_pair(static_cast<HttpStatusCode>(504), StatusCode::kInternal),
std::make_pair(HttpStatusCode::kGatewayTimeout,
StatusCode::kUnavailable),
std::make_pair(static_cast<HttpStatusCode>(601), StatusCode::kUnknown)),
[](testing::TestParamInfo<MapHttpCodeToStatusTest::ParamType> const& info) {
return std::to_string(std::get<0>(info.param));
Expand Down Expand Up @@ -135,6 +136,8 @@ TEST(AsStatus, RestResponseIsNotOkNoPayload) {
EXPECT_THAT(status, StatusIs(StatusCode::kNotFound));
EXPECT_THAT(status.message(), Eq("Received HTTP status code: 404"));
EXPECT_TRUE(status.error_info().reason().empty());
EXPECT_THAT(status.error_info().metadata(),
Contains(::testing::Pair("http_status_code", "404")));
}

TEST(AsStatus, RestResponseIsNotOkPayload) {
Expand Down
Loading