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

feat: GCS parse "error info" from JSON when available #7654

Merged
merged 4 commits into from
Nov 23, 2021
Merged

feat: GCS parse "error info" from JSON when available #7654

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd0cb93
feat: gcs extracts ErrorInfo if present
devjgm Nov 22, 2021
c0c9895
fixed null json object crash
devjgm Nov 22, 2021
29d1ae7
tidy fixes
devjgm Nov 23, 2021
bdf3c1f
used auto constexpr
devjgm Nov 23, 2021
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
190 changes: 125 additions & 65 deletions google/cloud/storage/internal/http_response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "google/cloud/storage/internal/http_response.h"
#include "google/cloud/internal/absl_str_join_quiet.h"
#include <nlohmann/json.hpp>
#include <iostream>

namespace google {
Expand All @@ -22,26 +23,25 @@ namespace storage {
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
namespace internal {

Status AsStatus(HttpResponse const& http_response) {
// The code here is organized by increasing range (or value) of the errors,
// just to make it readable. There are probably shorter (and/or more
// efficient) ways to write this, but we went for readability.
namespace {

if (http_response.status_code < HttpStatusCode::kMinContinue) {
return Status(StatusCode::kUnknown, http_response.payload);
}
if (HttpStatusCode::kMinContinue <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinSuccess) {
// We treat the 100s (e.g. 100 Continue) as OK results. They normally are
// ignored by libcurl, so we do not really expect to see them.
return Status(StatusCode::kOk, std::string{});
}
if (HttpStatusCode::kMinSuccess <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinRedirects) {
// We treat the 200s as Okay results.
return Status(StatusCode::kOk, std::string{});
}
if (http_response.status_code == HttpStatusCode::kResumeIncomplete) {
// Maps HTTP status codes to enumerators in `StatusCode`. The code here is
// organized by increasing range (or value) of the errors, just to make it
// readable. There are probably shorter (and/or more efficient) ways to write
// this, but we went for readability.
StatusCode MapHttpCodeToStatus(long code) { // NOLINT(google-runtime-int)
// We treat the 100s (e.g. 100 Continue) as OK results. They normally are
// ignored by libcurl, so we do not really expect to see them.
if (HttpStatusCode::kMinContinue <= code &&
code < HttpStatusCode::kMinSuccess) {
return StatusCode::kOk;
}
// We treat the 200s as Okay results.
if (HttpStatusCode::kMinSuccess <= code &&
code < HttpStatusCode::kMinRedirects) {
return StatusCode::kOk;
}
if (code == HttpStatusCode::kResumeIncomplete) {
// 308 - Resume Incomplete: this one is terrible. When performing a PUT
// for a resumable upload this means "The client and server are out of sync
// in this resumable upload, please reset". Unfortunately, during a
Expand All @@ -50,87 +50,147 @@ Status AsStatus(HttpResponse const& http_response) {
// like a kOk, the first is more like a kFailedPrecondition.
// This level of complexity / detail is something that the caller should
// handle, i.e., the mapping depends on the operation.
return Status(StatusCode::kFailedPrecondition, http_response.payload);
return StatusCode::kFailedPrecondition;
}
if (http_response.status_code == HttpStatusCode::kNotModified) {
if (code == HttpStatusCode::kNotModified) {
// 304 - Not Modified: evidently GCS returns 304 for some failed
// pre-conditions. It is somewhat strange that it also returns this error
// code for downloads, which is always read-only and was not going to modify
// anything. In any case, it seems too confusing to return anything other
// than kFailedPrecondition here.
return Status(StatusCode::kFailedPrecondition, http_response.payload);
return StatusCode::kFailedPrecondition;
}
if (HttpStatusCode::kMinRedirects <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinRequestErrors) {
if (HttpStatusCode::kMinRedirects <= code &&
code < HttpStatusCode::kMinRequestErrors) {
// The 300s should be handled by libcurl, we should not get them, according
// to the Google Cloud Storage documentation these are:
// 302 - Found
// 303 - See Other
// 307 - Temporary Redirect
return Status(StatusCode::kUnknown, http_response.payload);
return StatusCode::kUnknown;
}
if (http_response.status_code == HttpStatusCode::kBadRequest) {
return Status(StatusCode::kInvalidArgument, http_response.payload);
if (code == HttpStatusCode::kBadRequest) {
return StatusCode::kInvalidArgument;
}
if (http_response.status_code == HttpStatusCode::kUnauthorized) {
return Status(StatusCode::kUnauthenticated, http_response.payload);
if (code == HttpStatusCode::kUnauthorized) {
return StatusCode::kUnauthenticated;
}
if (http_response.status_code == HttpStatusCode::kForbidden) {
return Status(StatusCode::kPermissionDenied, http_response.payload);
if (code == HttpStatusCode::kForbidden) {
return StatusCode::kPermissionDenied;
}
if (http_response.status_code == HttpStatusCode::kNotFound) {
return Status(StatusCode::kNotFound, http_response.payload);
if (code == HttpStatusCode::kNotFound) {
return StatusCode::kNotFound;
}
if (http_response.status_code == HttpStatusCode::kMethodNotAllowed) {
return Status(StatusCode::kPermissionDenied, http_response.payload);
if (code == HttpStatusCode::kMethodNotAllowed) {
return StatusCode::kPermissionDenied;
}
if (http_response.status_code == HttpStatusCode::kRequestTimeout) {
if (code == HttpStatusCode::kRequestTimeout) {
// GCS uses a 408 to signal that an upload has suffered a broken
// connection, and that the client should retry.
return Status(StatusCode::kUnavailable, http_response.payload);
return StatusCode::kUnavailable;
}
if (http_response.status_code == HttpStatusCode::kConflict) {
return Status(StatusCode::kAborted, http_response.payload);
if (code == HttpStatusCode::kConflict) {
return StatusCode::kAborted;
}
if (http_response.status_code == HttpStatusCode::kGone) {
return Status(StatusCode::kNotFound, http_response.payload);
if (code == HttpStatusCode::kGone) {
return StatusCode::kNotFound;
}
if (http_response.status_code == HttpStatusCode::kLengthRequired) {
return Status(StatusCode::kInvalidArgument, http_response.payload);
if (code == HttpStatusCode::kLengthRequired) {
return StatusCode::kInvalidArgument;
}
if (http_response.status_code == HttpStatusCode::kPreconditionFailed) {
return Status(StatusCode::kFailedPrecondition, http_response.payload);
if (code == HttpStatusCode::kPreconditionFailed) {
return StatusCode::kFailedPrecondition;
}
if (http_response.status_code == HttpStatusCode::kPayloadTooLarge) {
return Status(StatusCode::kOutOfRange, http_response.payload);
if (code == HttpStatusCode::kPayloadTooLarge) {
return StatusCode::kOutOfRange;
}
if (http_response.status_code ==
HttpStatusCode::kRequestRangeNotSatisfiable) {
return Status(StatusCode::kOutOfRange, http_response.payload);
if (code == HttpStatusCode::kRequestRangeNotSatisfiable) {
return StatusCode::kOutOfRange;
}
if (http_response.status_code == HttpStatusCode::kTooManyRequests) {
return Status(StatusCode::kUnavailable, http_response.payload);
if (code == HttpStatusCode::kTooManyRequests) {
return StatusCode::kUnavailable;
}
if (HttpStatusCode::kMinRequestErrors <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinInternalErrors) {
if (HttpStatusCode::kMinRequestErrors <= code &&
code < HttpStatusCode::kMinInternalErrors) {
// 4XX - A request error.
return Status(StatusCode::kInvalidArgument, http_response.payload);
return StatusCode::kInvalidArgument;
}
if (http_response.status_code == HttpStatusCode::kInternalServerError) {
return Status(StatusCode::kUnavailable, http_response.payload);
if (code == HttpStatusCode::kInternalServerError) {
return StatusCode::kUnavailable;
}
if (http_response.status_code == HttpStatusCode::kBadGateway) {
return Status(StatusCode::kUnavailable, http_response.payload);
if (code == HttpStatusCode::kBadGateway) {
return StatusCode::kUnavailable;
}
if (http_response.status_code == HttpStatusCode::kServiceUnavailable) {
return Status(StatusCode::kUnavailable, http_response.payload);
if (code == HttpStatusCode::kServiceUnavailable) {
return StatusCode::kUnavailable;
}
if (HttpStatusCode::kMinInternalErrors <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinInvalidCode) {
if (HttpStatusCode::kMinInternalErrors <= code &&
code < HttpStatusCode::kMinInvalidCode) {
// 5XX - server errors are mapped to kInternal.
return Status(StatusCode::kInternal, http_response.payload);
return StatusCode::kInternal;
}
return Status(StatusCode::kUnknown, http_response.payload);
return StatusCode::kUnknown;
}

// Makes an `ErrorInfo` from an `"error"` JSON object that looks like
// [
// {
// "@type": "type.googleapis.com/google.rpc.ErrorInfo",
// "reason": "..."
// "domain": "..."
// "metdata": {
// "key1": "value1"
// ...
// }
// }
// ]
// See also https://cloud.google.com/apis/design/errors#http_mapping
ErrorInfo MakeErrorInfo(nlohmann::json const& details) {
static auto const* const kErrorInfoType =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: auto constexpr kErrorInfoType = "..."; seems like enough?

"type.googleapis.com/google.rpc.ErrorInfo";
for (auto const& e : details.items()) {
auto const& v = e.value();
if (v.value("@type", "") != kErrorInfoType) continue;
auto reason = v.value("reason", "");
auto domain = v.value("domain", "");
auto metadata_json = v.value("metadata", nlohmann::json::object());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I do not worry about copying here, this should be the uncommon path. But if one was worried about copying, there is a copy of something like a small std::map<>. You could do something like:

  auto domain = v.value("domain", "");
  if (!v.contains("metadata")) return ErrorInfo{std::move(reason), std::move(domain), {});
  auto metadata = std::unordered_map<std::string, std::string>{};
  for (auto const& m : v["metadata"].items()) {
    metadata[m.key()] = m.value();
  }
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Due to the likely minimal copying that would save, I think I'll leave the code as is, because I think it's possibly slightly easier to read as is.

auto metadata = std::unordered_map<std::string, std::string>{};
for (auto const& m : metadata_json.items()) {
metadata[m.key()] = m.value();
}
return ErrorInfo{std::move(reason), std::move(domain), std::move(metadata)};
}
return ErrorInfo{};
}

} // namespace

Status AsStatus(HttpResponse const& http_response) {
auto const status_code = MapHttpCodeToStatus(http_response.status_code);
if (status_code == StatusCode::kOk) return Status{};

// We try to parse the payload as JSON, which may allow us to provide a more
// structured and useful error Status. If the payload fails to parse as JSON,
// we simply attach the full error payload as the Status's message string.
auto json = nlohmann::json::parse(http_response.payload, nullptr, false);
if (json.is_discarded()) return Status(status_code, http_response.payload);

// We expect JSON that looks like the following:
// {
// "error": {
// "message": "..."
// ...
// "details": [
// ...
// ]
// }
// }
// See https://cloud.google.com/apis/design/errors#http_mapping
auto error = json.value("error", nlohmann::json::object());
auto message = error.value("message", http_response.payload);
auto details = error.value("details", nlohmann::json::object());
auto error_info = MakeErrorInfo(details);
return Status(status_code, std::move(message), std::move(error_info));
}

std::ostream& operator<<(std::ostream& os, HttpResponse const& rhs) {
Expand Down
40 changes: 40 additions & 0 deletions google/cloud/storage/internal/http_response_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "google/cloud/storage/internal/http_response.h"
#include "google/cloud/testing_util/status_matchers.h"
#include <gmock/gmock.h>
#include <nlohmann/json.hpp>

namespace google {
namespace cloud {
Expand Down Expand Up @@ -92,6 +93,45 @@ TEST(HttpResponseTest, AsStatus) {
EXPECT_THAT(AsStatus(HttpResponse{600, "bad", {}}),
StatusIs(StatusCode::kUnknown));
}

TEST(HttpResponseTest, ErrorInfo) {
// This example payload comes from
// https://cloud.google.com/apis/design/errors#http_mapping
std::string const json_payload = R"(
{
"error": {
"code": 400,
"message": "API key not valid. Please pass a valid API key.",
"status": "INVALID_ARGUMENT",
"details": [
{
"@type": "type.googleapis.com/google.rpc.ErrorInfo",
"reason": "API_KEY_INVALID",
"domain": "googleapis.com",
"metadata": {
"service": "translate.googleapis.com"
}
}
]
}
}
)";

ErrorInfo error_info{"API_KEY_INVALID",
"googleapis.com",
{{"service", "translate.googleapis.com"}}};
std::string message = "API key not valid. Please pass a valid API key.";
Status expected{StatusCode::kInvalidArgument, message, error_info};
EXPECT_EQ(AsStatus(HttpResponse{400, json_payload, {}}), expected);
}

TEST(HttpResponseTest, ErrorInfoInvalidJson) {
// Some valid json, but not what we're looking for.
std::string const json_payload = R"({"code":123, "message":"some message" })";
Status expected{StatusCode::kInvalidArgument, json_payload};
EXPECT_EQ(AsStatus(HttpResponse{400, json_payload, {}}), expected);
}

} // namespace
} // namespace internal
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
Expand Down