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

Improve the error handling for HTTP client so consumers can trigger appropriate behavior #793

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Improve the error handling for HTTP client so consumers can trigger appropriate behavior #793

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
18cc895
Improve the error handling for HTTP client so consumers can trigger a…
passuied Mar 14, 2025
4a61d21
PR feedback and __str__ overwrite
passuied Mar 16, 2025
4c4917f
cleanup
passuied Mar 16, 2025
18f06ef
keep code DRY
passuied Mar 19, 2025
6e2c885
encapsulate logic in DaprHttpError constructor
passuied Mar 19, 2025
775779e
format
passuied Mar 19, 2025
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
35 changes: 35 additions & 0 deletions dapr/clients/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ def as_json_safe_dict(self):

return error_dict

def __str__(self):
if self._error_code != ERROR_CODE_UNKNOWN:
return f"('{self._message}', '{self._error_code}')"
return self._message or 'Unknown Dapr Error.'


class StatusDetails:
def __init__(self):
Expand All @@ -74,6 +79,36 @@ def as_dict(self):
return {attr: getattr(self, attr) for attr in self.__dict__}


class DaprHttpError(DaprInternalError):
"""DaprHttpError encapsulates all Dapr HTTP exceptions"""
def __init__(
self,
message: Optional[str] = None,
error_code: Optional[str] = ERROR_CODE_UNKNOWN,
raw_response_bytes: Optional[bytes] = None,
status_code: Optional[int] = None,
reason: Optional[str] = None,
):
self._status_code = status_code
self._reason = reason
super(__class__, self).__init__(
message or f'HTTP status code: {status_code}',
error_code,
raw_response_bytes)

def as_dict(self):
error_dict = super(__class__, self).as_dict()
error_dict['status_code'] = self._status_code
error_dict['reason'] = self._reason
return error_dict

def __str__(self):
if self._error_code != ERROR_CODE_UNKNOWN:
return super(__class__, self).__str__()
else:
return f"Unknown Dapr Error. HTTP status code: {self._status_code}"


class DaprGrpcError(RpcError):
def __init__(self, err: RpcError):
self._status_code = err.code()
Expand Down
28 changes: 21 additions & 7 deletions dapr/clients/http/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

from dapr.conf import settings
from dapr.clients.base import DEFAULT_JSON_CONTENT_TYPE
from dapr.clients.exceptions import DaprInternalError, ERROR_CODE_DOES_NOT_EXIST, ERROR_CODE_UNKNOWN
from dapr.clients.exceptions import DaprHttpError, DaprInternalError, ERROR_CODE_DOES_NOT_EXIST, ERROR_CODE_UNKNOWN


class DaprHttpClient:
Expand Down Expand Up @@ -106,22 +106,36 @@ async def convert_to_error(self, response: aiohttp.ClientResponse) -> DaprIntern
try:
error_body = await response.read()
if (error_body is None or len(error_body) == 0) and response.status == 404:
return DaprInternalError('Not Found', ERROR_CODE_DOES_NOT_EXIST)
return DaprHttpError(
error_code=ERROR_CODE_DOES_NOT_EXIST,
status_code=response.status,
reason=response.reason,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic should be encapsulated in the error class itself. The error class would only receive the response object and it would create the error code, message and everything else based on it.

Copy link
Author

@passuied passuied Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, calling response.read() is async so I'm not sure we can use it in a constructor... copilot is recommending using asyncio.run() but I'm not sure that's a good idea... I'm worried about the negative performance side effects. There's also a dependency on self._serializer...
All in all, I'm not sure if the encapsulation is worth it...

This is the best I could do below but I don't like the dependency to DaprHttpClient instance.

async def from_response(http_client: DaprHttpClient, response: aiohttp.ClientResponse):
        error_info = None
        try:
            error_body = await response.read()
            if (error_body is None or len(error_body) == 0) and response.status == 404:
                return DaprHttpError(
                    error_code=ERROR_CODE_DOES_NOT_EXIST,
                    status_code=response.status,
                    reason=response.reason,
                )
            error_info = http_client._serializer.deserialize(error_body)
        except Exception:
            return DaprHttpError(
                error_code=ERROR_CODE_UNKNOWN,
                raw_response_bytes=error_body,
                status_code=response.status,
                reason=response.reason,
            )

        if error_info and isinstance(error_info, dict):
            message = error_info.get('message')
            error_code = error_info.get('errorCode') or ERROR_CODE_UNKNOWN
            return DaprHttpError(
                message=message,
                error_code=error_code,
                raw_response_bytes=error_body,
                status_code=response.status,
                reason=response.reason,
            )

        return DaprHttpError(
            error_code=ERROR_CODE_UNKNOWN,
            raw_response_bytes=error_body,
            status_code=response.status,
            reason=response.reason,
        )

Copy link
Author

Choose a reason for hiding this comment

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

I have refactored the code and I have come up with a good compromise (I hope). Please let me know

error_info = self._serializer.deserialize(error_body)
except Exception:
return DaprInternalError(
f'Unknown Dapr Error. HTTP status code: {response.status}',
return DaprHttpError(
error_code=ERROR_CODE_UNKNOWN,
raw_response_bytes=error_body,
status_code=response.status,
reason=response.reason,
)

if error_info and isinstance(error_info, dict):
message = error_info.get('message')
error_code = error_info.get('errorCode') or ERROR_CODE_UNKNOWN
return DaprInternalError(message, error_code, raw_response_bytes=error_body)
return DaprHttpError(
message=message,
error_code=error_code,
raw_response_bytes=error_body,
status_code=response.status,
reason=response.reason,
)

return DaprInternalError(
f'Unknown Dapr Error. HTTP status code: {response.status}',
return DaprHttpError(
error_code=ERROR_CODE_UNKNOWN,
raw_response_bytes=error_body,
status_code=response.status,
reason=response.reason,
)

def get_ssl_context(self):
Expand Down
49 changes: 49 additions & 0 deletions tests/clients/test_secure_http_service_invocation_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from opentelemetry.trace.propagation.tracecontext import TraceContextTextMapPropagator

from dapr.clients import DaprClient, DaprGrpcClient
from dapr.clients.exceptions import DaprInternalError
from dapr.clients.health import DaprHealth
from dapr.clients.http.client import DaprHttpClient
from dapr.conf import settings
Expand Down Expand Up @@ -139,3 +140,51 @@ def test_timeout_exception_thrown_when_timeout_reached(self):
self.server.set_server_delay(1.5)
with self.assertRaises(TimeoutError):
new_client.invoke_method(self.app_id, self.method_name, '')

def test_notfound_json_body_exception_thrown_with_status_code_and_reason(self):
self.server.set_response(b'{"error": "Not found"}', code=404)
with self.assertRaises(DaprInternalError) as context:
self.client.invoke_method(self.app_id, self.method_name, '')

error_dict = context.exception.as_dict()
self.assertEqual("HTTP status code: 404", error_dict.get('message'))
self.assertEqual("UNKNOWN", error_dict.get('errorCode'))
self.assertEqual(b'{"error": "Not found"}', error_dict.get('raw_response_bytes'))
self.assertEqual(404, error_dict.get('status_code'))
self.assertEqual('Not Found', error_dict.get('reason'))

def test_notfound_no_body_exception_thrown_with_status_code_and_reason(self):
self.server.set_response(b'', code=404)
with self.assertRaises(DaprInternalError) as context:
self.client.invoke_method(self.app_id, self.method_name, '')

error_dict = context.exception.as_dict()
self.assertEqual("HTTP status code: 404", error_dict.get('message'))
self.assertEqual("ERR_DOES_NOT_EXIST", error_dict.get('errorCode'))
self.assertEqual(None, error_dict.get('raw_response_bytes'))
self.assertEqual(404, error_dict.get('status_code'))
self.assertEqual('Not Found', error_dict.get('reason'))

def test_notfound_no_json_body_exception_thrown_with_status_code_and_reason(self):
self.server.set_response(b"Not found", code=404)
with self.assertRaises(DaprInternalError) as context:
self.client.invoke_method(self.app_id, self.method_name, '')

error_dict = context.exception.as_dict()
self.assertEqual("HTTP status code: 404", error_dict.get('message'))
self.assertEqual("UNKNOWN", error_dict.get('errorCode'))
self.assertEqual(b"Not found", error_dict.get('raw_response_bytes'))
self.assertEqual(404, error_dict.get('status_code'))
self.assertEqual('Not Found', error_dict.get('reason'))

def test_notfound_json_body_w_message_exception_thrown_with_status_code_and_reason(self):
self.server.set_response(b'{"message": "My message", "errorCode": "MY_ERROR_CODE"}', code=404)
with self.assertRaises(DaprInternalError) as context:
self.client.invoke_method(self.app_id, self.method_name, '')

error_dict = context.exception.as_dict()
self.assertEqual("My message", error_dict.get('message'))
self.assertEqual("MY_ERROR_CODE", error_dict.get('errorCode'))
self.assertEqual(b'{"message": "My message", "errorCode": "MY_ERROR_CODE"}', error_dict.get('raw_response_bytes'))
self.assertEqual(404, error_dict.get('status_code'))
self.assertEqual('Not Found', error_dict.get('reason'))