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 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 60 additions & 4 deletions google/api_core/exceptions.py
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 @@ -116,15 +119,16 @@ 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)
return "{} {} {}".format(self.code, self.message, self.error_details)
tseaver marked this conversation as resolved.
Show resolved Hide resolved

@property
def errors(self):
Expand All @@ -135,6 +139,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 +425,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 +480,41 @@ def _is_informative_grpc_error(rpc_exc):
return hasattr(rpc_exc, "code") and hasattr(rpc_exc, "details")


def _parse_grpc_error_details(rpc_exc):
if not rpc_status:
return []
tseaver marked this conversation as resolved.
Show resolved Hide resolved
if not isinstance(rpc_exc, grpc.Call):
return []
tseaver marked this conversation as resolved.
Show resolved Hide resolved
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,9 @@ 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
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
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

2 changes: 2 additions & 0 deletions tests/asyncio/test_grpc_helpers_async.py
Expand Up @@ -33,6 +33,8 @@ 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
79 changes: 71 additions & 8 deletions tests/unit/test_exceptions.py
Expand Up @@ -14,35 +14,37 @@

import http.client
import json
from google.rpc import status_pb2
from google.rpc.status_pb2 import Status

import grpc
import mock
import requests

from google.rpc import error_details_pb2
tseaver marked this conversation as resolved.
Show resolved Hide resolved
from google.rpc import code_pb2
from google.protobuf import any_pb2, json_format
from grpc_status import rpc_status
tseaver marked this conversation as resolved.
Show resolved Hide resolved
from google.api_core import exceptions

from google.protobuf import text_format

def test_create_google_cloud_error():
exception = exceptions.GoogleAPICallError("Testing")
exception.code = 600
assert str(exception) == "600 Testing"
assert str(exception) == "600 Testing []"
tseaver marked this conversation as resolved.
Show resolved Hide resolved
assert exception.message == "Testing"
assert exception.errors == []
assert exception.response is None


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)
exception.code = 600
assert str(exception) == "600 Testing"
assert str(exception) == "600 Testing []"
assert exception.message == "Testing"
assert exception.errors == [error]
assert exception.response == response
Expand Down Expand Up @@ -224,3 +226,64 @@ 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 = 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(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


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]
3 changes: 3 additions & 0 deletions tests/unit/test_grpc_helpers.py
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