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

feat: Add error_details property to GoogleAPICallError based on google.rpc.status.details. #286

Merged
merged 15 commits into from
Oct 19, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
66 changes: 62 additions & 4 deletions google/api_core/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@
from __future__ import unicode_literals

import http.client
from google.rpc import error_details_pb2
tseaver marked this conversation as resolved.
Show resolved Hide resolved

try:
import grpc
from grpc_status import rpc_status
tseaver marked this conversation as resolved.
Show resolved Hide resolved

except ImportError: # pragma: NO COVER
grpc = None
rpc_status = None

# Lookup tables for mapping exceptions from HTTP and gRPC transports.
# Populated by _GoogleAPICallErrorMeta
Expand Down Expand Up @@ -96,6 +99,7 @@ class GoogleAPICallError(GoogleAPIError, metaclass=_GoogleAPICallErrorMeta):
Args:
message (str): The exception message.
errors (Sequence[Any]): An optional list of error details.
details (Sequence[Any]): An optional list of objects defined in google.rpc.error_details.
response (Union[requests.Request, grpc.Call]): The response or
gRPC call metadata.
"""
Expand All @@ -116,15 +120,19 @@ class GoogleAPICallError(GoogleAPIError, metaclass=_GoogleAPICallErrorMeta):
This may be ``None`` if the exception does not match up to a gRPC error.
"""

def __init__(self, message, errors=(), response=None):
def __init__(self, message, errors=(), details=(), response=None):
tseaver marked this conversation as resolved.
Show resolved Hide resolved
super(GoogleAPICallError, self).__init__(message)
self.message = message
"""str: The exception message."""
self._errors = errors
self._details = details
self._response = response

def __str__(self):
return "{} {}".format(self.code, self.message)
if self.error_details:
return "{} {} {}".format(self.code, self.message, self.error_details)
else:
return "{} {}".format(self.code, self.message)

@property
def errors(self):
Expand All @@ -135,6 +143,19 @@ def errors(self):
"""
return list(self._errors)

@property
def error_details(self):

Choose a reason for hiding this comment

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

Is details property has been occupied? If not, is that better to use details as name or status_details relate to google.rpc.Status.details.
Notice that we need to keep consistency between grpc and REST.
IMO, I prefer status_details. WDYT @busunkim96 @tseaver @atulep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the naming convention in design doc. I personally prefer details because it's a common denominator between google/rpc/error_details.proto and google.rpc.Status.details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name to "details".

"""Information contained in google.rpc.status.details.

Reference:
https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto
https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto

Returns:
Sequence[Any]: A list of structured objects from error_details.proto
"""
return list(self._details)

@property
def response(self):
"""Optional[Union[requests.Request, grpc.Call]]: The response or
Expand Down Expand Up @@ -408,13 +429,15 @@ def from_http_response(response):

error_message = payload.get("error", {}).get("message", "unknown error")
errors = payload.get("error", {}).get("errors", ())
# In JSON, details are already formatted in developer-friendly way.
details = payload.get("error", {}).get("details", ())

message = "{method} {url}: {error}".format(
method=response.request.method, url=response.request.url, error=error_message
)

exception = from_http_status(
response.status_code, message, errors=errors, response=response
response.status_code, message, errors=errors, details=details, response=response
)
return exception

Expand Down Expand Up @@ -461,6 +484,37 @@ def _is_informative_grpc_error(rpc_exc):
return hasattr(rpc_exc, "code") and hasattr(rpc_exc, "details")


def _parse_grpc_error_details(rpc_exc):
status = rpc_status.from_call(rpc_exc)
if not status:
return []
possible_errors = [
error_details_pb2.BadRequest,
error_details_pb2.PreconditionFailure,
error_details_pb2.QuotaFailure,
error_details_pb2.ErrorInfo,
error_details_pb2.RetryInfo,
error_details_pb2.ResourceInfo,
error_details_pb2.RequestInfo,
error_details_pb2.DebugInfo,
error_details_pb2.Help,
error_details_pb2.LocalizedMessage,
]
tseaver marked this conversation as resolved.
Show resolved Hide resolved
error_details = []
for detail in status.details:
matched_detail_cls = list(
filter(lambda x: detail.Is(x.DESCRIPTOR), possible_errors)
)
# If nothing matched, use detail directly.
if len(matched_detail_cls) == 0:
info = detail
tseaver marked this conversation as resolved.
Show resolved Hide resolved
else:
info = matched_detail_cls[0]()
detail.Unpack(info)
error_details.append(info)
return error_details


def from_grpc_error(rpc_exc):
"""Create a :class:`GoogleAPICallError` from a :class:`grpc.RpcError`.

Expand All @@ -475,7 +529,11 @@ def from_grpc_error(rpc_exc):
# However, check for grpc.RpcError breaks backward compatibility.
if isinstance(rpc_exc, grpc.Call) or _is_informative_grpc_error(rpc_exc):
return from_grpc_status(
rpc_exc.code(), rpc_exc.details(), errors=(rpc_exc,), response=rpc_exc
rpc_exc.code(),
rpc_exc.details(),
errors=(rpc_exc,),
details=_parse_grpc_error_details(rpc_exc),
response=rpc_exc,
)
else:
return GoogleAPICallError(str(rpc_exc), errors=(rpc_exc,), response=rpc_exc)
4 changes: 2 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@
# 'Development Status :: 5 - Production/Stable'
release_status = "Development Status :: 5 - Production/Stable"
dependencies = [
"googleapis-common-protos >= 1.6.0, < 2.0dev",
"googleapis-common-protos >= 1.52.0, < 2.0dev",
"protobuf >= 3.12.0",
"google-auth >= 1.25.0, < 3.0dev",
"requests >= 2.18.0, < 3.0.0dev",
"setuptools >= 40.3.0",
]
extras = {
"grpc": "grpcio >= 1.33.2, < 2.0dev",
"grpc": ["grpcio >= 1.33.2, < 2.0dev", "grpcio-status >= 1.0.0, < 2.0dev"],
tseaver marked this conversation as resolved.
Show resolved Hide resolved
"grpcgcp": "grpcio-gcp >= 0.2.2",
"grpcio-gcp": "grpcio-gcp >= 0.2.2",
}
Expand Down
3 changes: 2 additions & 1 deletion testing/constraints-3.6.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#
# e.g., if setup.py has "foo >= 1.14.0, < 2.0.0dev",
# Then this file should have foo==1.14.0
googleapis-common-protos==1.6.0
googleapis-common-protos==1.52.0
protobuf==3.12.0
google-auth==1.25.0
requests==2.18.0
Expand All @@ -14,3 +14,4 @@ packaging==14.3
grpcio==1.33.2
grpcio-gcp==0.2.2
grpcio-gcp==0.2.2
grpcio-status==1.33.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
grpcio-status==1.33.2
grpcio-status==1.0.0

(to match the lower bound in setup.py)

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the grpcio-status version supposed to be identical to the one for grpcio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit confusing. I initially put 1.0.0 lower bound in setup.py without giving it too much thought. As we found out later, grpcio-status requires the same version as grpc (see]). To avoid confusion, I say we should change lower bound in setup.py to match the bound for grpcio (ie 1.33.2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to 1.33.2

3 changes: 3 additions & 0 deletions tests/asyncio/test_grpc_helpers_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ def code(self):
def details(self):
return None

def trailing_metadata(self):
return None


@pytest.mark.asyncio
async def test_wrap_unary_errors():
Expand Down
103 changes: 97 additions & 6 deletions tests/unit/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,19 @@

import http.client
import json

import grpc
import mock
import requests

from google.rpc import error_details_pb2, status_pb2
from google.protobuf import any_pb2, json_format

try:
import grpc
from grpc_status import rpc_status
except ImportError:
grpc = None
grpc_status = None

from google.api_core import exceptions


Expand All @@ -33,11 +41,8 @@ def test_create_google_cloud_error():

def test_create_google_cloud_error_with_args():
error = {
"domain": "global",
"location": "test",
"locationType": "testing",
"code": 600,
"message": "Testing",
"reason": "test",
}
response = mock.sentinel.response
exception = exceptions.GoogleAPICallError("Testing", [error], response=response)
Expand Down Expand Up @@ -224,3 +229,89 @@ def test_from_grpc_error_non_call():
assert exception.message == message
assert exception.errors == [error]
assert exception.response == error


def create_bad_request_details():
bad_request_details = error_details_pb2.BadRequest()
field_violation = bad_request_details.field_violations.add()
field_violation.field = "document.content"
field_violation.description = "Must have some text content to annotate."
status_detail = any_pb2.Any()
status_detail.Pack(bad_request_details)
return status_detail


def test_error_details_from_rest_response():
bad_request_detail = create_bad_request_details()
status = status_pb2.Status()
status.code = 3
status.message = (
"3 INVALID_ARGUMENT: One of content, or gcs_content_uri must be set."
)
status.details.append(bad_request_detail)

# See JSON schema in https://cloud.google.com/apis/design/errors#http_mapping
http_response = make_response(
json.dumps({"error": json.loads(json_format.MessageToJson(status))}).encode(
"utf-8"
)
)
exception = exceptions.from_http_response(http_response)
want_error_details = [json.loads(json_format.MessageToJson(bad_request_detail))]
assert want_error_details == exception.error_details
# 404 POST comes from make_response.
assert str(exception) == (
"404 POST https://example.com/: 3 INVALID_ARGUMENT:"
" One of content, or gcs_content_uri must be set."
" [{'@type': 'type.googleapis.com/google.rpc.BadRequest',"
" 'fieldViolations': [{'field': 'document.content',"
" 'description': 'Must have some text content to annotate.'}]}]"
)


def test_error_details_from_v1_rest_response():
response = make_response(
json.dumps(
{"error": {"message": "\u2019 message", "errors": ["1", "2"]}}
).encode("utf-8")
)
exception = exceptions.from_http_response(response)
assert exception.error_details == []


def test_error_details_from_grpc_response():
atulep marked this conversation as resolved.
Show resolved Hide resolved
status = rpc_status.status_pb2.Status()
status.code = 3
status.message = (
"3 INVALID_ARGUMENT: One of content, or gcs_content_uri must be set."
)
status_detail = create_bad_request_details()
status.details.append(status_detail)

# Actualy error doesn't matter as long as its grpc.Call,
# because from_call is mocked.
error = mock.create_autospec(grpc.Call, instance=True)
with mock.patch("grpc_status.rpc_status.from_call") as m:
m.return_value = status
exception = exceptions.from_grpc_error(error)

bad_request_detail = error_details_pb2.BadRequest()
status_detail.Unpack(bad_request_detail)
assert exception.error_details == [bad_request_detail]


def test_error_details_from_grpc_response_unknown_error():
atulep marked this conversation as resolved.
Show resolved Hide resolved
status_detail = any_pb2.Any()

status = rpc_status.status_pb2.Status()
status.code = 3
status.message = (
"3 INVALID_ARGUMENT: One of content, or gcs_content_uri must be set."
)
status.details.append(status_detail)

error = mock.create_autospec(grpc.Call, instance=True)
with mock.patch("grpc_status.rpc_status.from_call") as m:
m.return_value = status
exception = exceptions.from_grpc_error(error)
assert exception.error_details == [status_detail]
3 changes: 3 additions & 0 deletions tests/unit/test_grpc_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ def code(self):
def details(self):
return None

def trailing_metadata(self):
return None


def test_wrap_unary_errors():
grpc_error = RpcErrorImpl(grpc.StatusCode.INVALID_ARGUMENT)
Expand Down