feat: convert grpc errors to api core exceptions (#457)

This brings NDB into line with other API libraries by calling
`google.api_core.exceptions.from_grpc_error` to convert grpc errors to
distinct exceptions from `google.api_core.exceptions`.

Closes #416

Author: Chris Rossi

google/cloud/ndb/_datastore_api.py

@@ -14,9 +14,11 @@
 
 """Functions that interact with Datastore backend."""
 
+import grpc
 import itertools
 import logging
 
+from google.api_core import exceptions as core_exceptions
 from google.cloud.datastore import helpers
 from google.cloud.datastore_v1.proto import datastore_pb2
 from google.cloud.datastore_v1.proto import entity_pb2
@@ -85,7 +87,13 @@ def rpc_call():
         log.debug(rpc)
         log.debug("timeout={}".format(timeout))
 
-        result = yield rpc
+        try:
+            result = yield rpc
+        except Exception as error:
+            if isinstance(error, grpc.Call):
+                error = core_exceptions.from_grpc_error(error)
+            raise error
+
         raise tasklets.Return(result)
 
     if retries:

google/cloud/ndb/_retry.py

@@ -15,7 +15,6 @@
 """Retry functions."""
 
 import functools
-import grpc
 import itertools
 
 from google.api_core import retry as core_retry
@@ -91,18 +90,18 @@ def retry_wrapper(*args, **kwargs):
     return retry_wrapper
 
 
-# Possibly we should include DEADLINE_EXCEEDED. The caveat is that I think the
+# Possibly we should include DeadlineExceeded. The caveat is that I think the
 # timeout is enforced on the client side, so it might be possible that a Commit
 # request times out on the client side, but still writes data on the server
 # side, in which case we don't want to retry, since we can't commit the same
 # transaction more than once. Some more research is needed here. If we discover
-# that a DEADLINE_EXCEEDED status code guarantees the operation was cancelled,
-# then we can add DEADLINE_EXCEEDED to our retryable status codes. Not knowing
-# the answer, it's best not to take that risk.
-TRANSIENT_CODES = (
-    grpc.StatusCode.UNAVAILABLE,
-    grpc.StatusCode.INTERNAL,
-    grpc.StatusCode.ABORTED,
+# that a DeadlineExceeded error guarantees the operation was cancelled, then we
+# can add DeadlineExceeded to our retryable errors. Not knowing the answer,
+# it's best not to take that risk.
+TRANSIENT_ERRORS = (
+    core_exceptions.ServiceUnavailable,
+    core_exceptions.InternalServerError,
+    core_exceptions.Aborted,
 )
 
 
@@ -115,10 +114,4 @@ def is_transient_error(error):
     if core_retry.if_transient_error(error):
         return True
 
-    if isinstance(error, grpc.Call):
-        method = getattr(error, "code", None)
-        if callable(method):
-            code = method()
-            return code in TRANSIENT_CODES
-
-    return False
+    return isinstance(error, TRANSIENT_ERRORS)

tests/system/test_query.py

@@ -19,12 +19,12 @@
 import datetime
 import operator
 
-import grpc
 import pytest
 import pytz
 
 import test_utils.system
 
+from google.api_core import exceptions as core_exceptions
 from google.cloud import ndb
 from google.cloud.datastore import key as ds_key_module
 
@@ -61,7 +61,7 @@ class SomeKind(ndb.Model):
     with pytest.raises(Exception) as error_context:
         query.fetch(timeout=timeout)
 
-    assert error_context.value.code() == grpc.StatusCode.DEADLINE_EXCEEDED
+    assert isinstance(error_context.value, core_exceptions.DeadlineExceeded)
 
 
 @pytest.mark.usefixtures("client_context")

tests/unit/test__datastore_api.py

@@ -17,9 +17,11 @@
 except ImportError:  # pragma: NO PY3 COVER
     import mock
 
+import grpc
 import pytest
 
 from google.api_core import client_info
+from google.api_core import exceptions as core_exceptions
 from google.cloud.datastore import entity
 from google.cloud.datastore import helpers
 from google.cloud.datastore import key as ds_key_module
@@ -132,6 +134,50 @@ def test_explicit_timeout(stub, _retry):
         assert call.result() == "bar"
         api.foo.future.assert_called_once_with(request, timeout=20)
 
+    @staticmethod
+    @pytest.mark.usefixtures("in_context")
+    @mock.patch("google.cloud.ndb._datastore_api.stub")
+    def test_grpc_error(stub):
+        api = stub.return_value
+        future = tasklets.Future()
+        api.foo.future.return_value = future
+
+        class DummyError(grpc.Call, Exception):
+            def code(self):
+                return grpc.StatusCode.UNAVAILABLE
+
+            def details(self):
+                return "Where is the devil in?"
+
+        try:
+            raise DummyError("Have to raise in order to get traceback")
+        except Exception as error:
+            future.set_exception(error)
+
+        request = object()
+        with pytest.raises(core_exceptions.ServiceUnavailable):
+            _api.make_call("foo", request, retries=0).result()
+
+    @staticmethod
+    @pytest.mark.usefixtures("in_context")
+    @mock.patch("google.cloud.ndb._datastore_api.stub")
+    def test_other_error(stub):
+        api = stub.return_value
+        future = tasklets.Future()
+        api.foo.future.return_value = future
+
+        class DummyException(Exception):
+            pass
+
+        try:
+            raise DummyException("Have to raise in order to get traceback")
+        except Exception as error:
+            future.set_exception(error)
+
+        request = object()
+        with pytest.raises(DummyException):
+            _api.make_call("foo", request, retries=0).result()
+
 
 def _mock_key(key_str):
     key = mock.Mock(kind="SomeKind", spec=("to_protobuf", "kind"))

tests/unit/test__retry.py

@@ -19,7 +19,6 @@
 except ImportError:  # pragma: NO PY3 COVER
     import mock
 
-import grpc
 import pytest
 
 from google.api_core import exceptions as core_exceptions
@@ -142,68 +141,40 @@ def test_core_says_yes(core_retry):
 
     @staticmethod
     @mock.patch("google.cloud.ndb._retry.core_retry")
-    def test_not_a_grpc_call(core_retry):
-        error = object()
-        core_retry.if_transient_error.return_value = False
-        assert _retry.is_transient_error(error) is False
-        core_retry.if_transient_error.assert_called_once_with(error)
-
-    @staticmethod
-    @mock.patch("google.cloud.ndb._retry.core_retry")
-    def test_code_is_not_callable(core_retry):
-        error = mock.Mock(spec=grpc.Call, code=404)
-        core_retry.if_transient_error.return_value = False
-        assert _retry.is_transient_error(error) is False
-        core_retry.if_transient_error.assert_called_once_with(error)
-
-    @staticmethod
-    @mock.patch("google.cloud.ndb._retry.core_retry")
-    def test_code_is_not_transient(core_retry):
-        error = mock.Mock(spec=grpc.Call, code=mock.Mock(return_value=42))
+    def test_error_is_not_transient(core_retry):
+        error = Exception("whatever")
         core_retry.if_transient_error.return_value = False
         assert _retry.is_transient_error(error) is False
         core_retry.if_transient_error.assert_called_once_with(error)
 
     @staticmethod
     @mock.patch("google.cloud.ndb._retry.core_retry")
     def test_unavailable(core_retry):
-        error = mock.Mock(
-            spec=grpc.Call,
-            code=mock.Mock(return_value=grpc.StatusCode.UNAVAILABLE),
-        )
+        error = core_exceptions.ServiceUnavailable("testing")
         core_retry.if_transient_error.return_value = False
         assert _retry.is_transient_error(error) is True
         core_retry.if_transient_error.assert_called_once_with(error)
 
     @staticmethod
     @mock.patch("google.cloud.ndb._retry.core_retry")
     def test_internal(core_retry):
-        error = mock.Mock(
-            spec=grpc.Call,
-            code=mock.Mock(return_value=grpc.StatusCode.INTERNAL),
-        )
+        error = core_exceptions.InternalServerError("testing")
         core_retry.if_transient_error.return_value = False
         assert _retry.is_transient_error(error) is True
         core_retry.if_transient_error.assert_called_once_with(error)
 
     @staticmethod
     @mock.patch("google.cloud.ndb._retry.core_retry")
     def test_unauthenticated(core_retry):
-        error = mock.Mock(
-            spec=grpc.Call,
-            code=mock.Mock(return_value=grpc.StatusCode.UNAUTHENTICATED),
-        )
+        error = core_exceptions.Unauthenticated("testing")
         core_retry.if_transient_error.return_value = False
         assert _retry.is_transient_error(error) is False
         core_retry.if_transient_error.assert_called_once_with(error)
 
     @staticmethod
     @mock.patch("google.cloud.ndb._retry.core_retry")
     def test_aborted(core_retry):
-        error = mock.Mock(
-            spec=grpc.Call,
-            code=mock.Mock(return_value=grpc.StatusCode.ABORTED),
-        )
+        error = core_exceptions.Aborted("testing")
         core_retry.if_transient_error.return_value = False
         assert _retry.is_transient_error(error) is True
         core_retry.if_transient_error.assert_called_once_with(error)