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.

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

Add ErrorInfo support to GCS #7644

Closed
devjgm opened this issue Nov 20, 2021 · 6 comments · Fixed by #7654
Closed

Add ErrorInfo support to GCS #7644

devjgm opened this issue Nov 20, 2021 · 6 comments · Fixed by #7654
Labels
api: storage Issues related to the Cloud Storage API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@devjgm
Copy link
Contributor

devjgm commented Nov 20, 2021

This is a follow on issue related to #7429. In that issue, we added ErrorInfo support to g::c::Status, per https://google.aip.dev/193. That's now done for gRPC based services.

GCS is not gRPC, and so it needs the ability to fill in the ErrorInfo class that's (optionally) passed to g::c::Status. Per @coryan 's comment in #7640

For a future PR: I think we should do the same thing for GCS, at least here:

Status AsStatus(HttpResponse const& http_response) {

Something like:

Status AsStatus(HttpResponse const& http_response) {
  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{};
  }
  if (HttpStatusCode::kMinSuccess <= http_response.status_code &&
      http_response.status_code < HttpStatusCode::kMinRedirects) {
    // We treat the 200s as Okay results.
    return Status{};
  }
  // Parse the payload as a JSON object, see https://cloud.google.com/apis/design/errors#http_mapping
  auto parsed = nlohmann::json::parse(http_response.payload, nullptr, false);
  ErrorInfo error_info = ExtractErrorInfo(parsed);
  return Status(/* map HTTP code */, parsed.value("message", ""), error_info);
@devjgm devjgm added api: storage Issues related to the Cloud Storage API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Nov 20, 2021
@devjgm
Copy link
Contributor Author

devjgm commented Nov 21, 2021

From https://cloud.google.com/apis/design/errors#http_mapping, an example response with "error info" data is:

{
  "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"
        }
      }
    ]
  }
}

@devjgm
Copy link
Contributor Author

devjgm commented Nov 21, 2021

Question: Within AsStatus(), is the HttpResponse::payload field always JSON? I see sometimes the payload is used directly as the Status's "message" field, so I'm not sure what format the payload field is.

@coryan
Copy link
Contributor

coryan commented Nov 21, 2021

Question: Within AsStatus(), is the HttpResponse::payload field always JSON? I see sometimes the payload is used directly as the Status's "message" field, so I'm not sure what format the payload field is.

I do not think we can guarantee that it is always JSON. Some error messages arrive from the load balancer, where they are not necessarily in JSON format. But when the error message is from the JSON API, then they are in the format outlined above.

We can try to parse them as JSON, and if that fails we just stick the full message in the message string (as we do today). If the parsing succeeds, then we can extract the ErrorInfo and use it in Status.

devjgm added a commit to devjgm/google-cloud-cpp that referenced this issue Nov 22, 2021
Fixes: googleapis#7644

Please review this nlohmann code closely, because I've never used it and
I may be doing things the least optimal way.
@devjgm
Copy link
Contributor Author

devjgm commented Nov 22, 2021

@devjgm
Copy link
Contributor Author

devjgm commented Nov 22, 2021

One problem currently is that GCS (or maybe it's the emulator) is returning some valid JSON that I'm not expecting. According to https://cloud.google.com/apis/design/errors#http_mapping, my code expects error JSON of the form:

     {
       "error": {
         "code": ...,
         "message": "..."
         ...
         "details": [
           ...
         ]
       }
  }

But I'm sometimes seeing JSON of the form

  {
     "code": ...,
     "message": ...
  }

That is, I see JSON data that looks like the contents of the "error" object. So I'm not sure which of these formats I should actually expect.

@coryan
Copy link
Contributor

coryan commented Nov 22, 2021

Ugh... That might be the emulator, specifically the code around here:

https://github.com/googleapis/storage-testbench/blob/main/testbench/error.py#L37

devjgm added a commit to devjgm/google-cloud-cpp that referenced this issue Nov 23, 2021
Fixes: googleapis#7644

Please review this nlohmann code closely, because I've never used it and
I may be doing things the least optimal way.
devjgm added a commit that referenced this issue Nov 23, 2021
Fixes: #7644

Follows https://cloud.google.com/apis/design/errors#http_mapping and parses the "error info" json object from responses when it's available, and uses it to set the fields of the returned `g::c::Status`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants