-
Notifications
You must be signed in to change notification settings - Fork 134
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
Improve the error handling for HTTP client so consumers can trigger appropriate behavior #793
base: main
Are you sure you want to change the base?
Improve the error handling for HTTP client so consumers can trigger appropriate behavior #793
Conversation
…ppropriate behavior Signed-off-by: Patrick Assuied <[email protected]>
7457f5a
to
18cc895
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like instead of modifying the genericDaprInternalError
class with http properties that are not always relevant, we should extend it to a http specific class, something like:
class DaprHttpError(DaprInternalError):
I like it. What about including the response object in the exception then? I was hesitating about it but didn't want to couple it. |
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
dapr/clients/http/client.py
Outdated
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, | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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,
)
There was a problem hiding this comment.
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
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Description
Improve the error handling for HTTP client so consumers can trigger appropriate behavior
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #794
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: