Open FCamborda opened 4 months ago
We raise from requests, you should be able to parse it
https://github.com/devopshq/artifactory/blob/master/dohq_artifactory/exception.py#L21
You mean by accessing __cause__
?
In that case, that means that we still have to import requests
and declare it as a direct dependency, which feels kind of artificial.
import artifactory
import requests
def should_retry(exception) -> bool:
recoverable_errors = {429}
dohq_exception = isinstance(exception, artifactory.ArtifactoryException)
raised_from = dohq_exception.__cause__
if dohq_exception and raised_from is not None:
requests_exception = isinstance(raised_from, requests.exceptions.HTTPError)
if requests_exception and requests_exception.response is not None:
if requests_exception.response.status_code in {recoverable_errors}:
return True
return False
Or is there a way to access response.status_code
directly from ArtifactoryException
? I thought raise ArtifactoryException(str(exception)) from exception
means that the Exception Object only has a messsage attribute.
it feels somehow wrong
first of all, you can directly catch HTTP exceptions if you know that you expect 429. No need to catch artifactory exception. then just use some lib like: https://github.com/jd/tenacity
We're already using Tenacity. The snippet I posted is a variation of what we run in our codebase. I wanted to abstract that implementation detail.
Correct me if I'm wrong, but in general your code is comparable to:
# requests.py
class RequestsException(Exception):
message = "bar" # Not a direct dependency of any user of `dohq-artifactory`
...
# dohq-artifactory.py
class ArtifactoryException(Exception):
pass # Custom exception
def raise_exception():
"""Mimics code in dohq-artifactory that would raise exception."""
try:
raise RequestsException
except RequestsException as exc:
raise ArtifactoryException("Copied message") from exc # Not hiding the original exception is a good idea IMO
Which means that your users have the following possibilities:
Catch ArtifactoryException
. In this case the error code is only accessible via __cause__
. Which in the best form I know leads to type checking if exc.__cause__
is not None and is a requests.exception.HTTPError
. The problem here is that this is not really part of your public API, so it might break in a future release without notice.
def main():
"""Mimics dependents of Artifactory."""
try:
raise_exception() # In real production code this would not be called directly, obviously
except ArtifactoryException as exc:
print(exc) # Checks against unknown library and __cause__ are needed
* Trying to catch `requests.exception.HTTPError` will not work as the exception you're raising is a custom one `artifactory.exceptions.ArtifactoryError`. The `from exc` means that the original Requests exception is only accessible via `.__cause__`, which again should be checked for correctness.
```python
def main():
"""Mimics dependents of Artifactory."""
try:
raise_exception()
except requests.exception.HTTPError as exc: # Awkward for users, might change at any time.
print(exc) # Also does not work, will never hit this line
Catching any Exception is even worse as it's an anti pattern and does not get rid of the check against requests.exception.HTTPError
def main():
"""Mimics dependents of Artifactory."""
try:
raise_exception()
except Exception: # Antipattern in this case
print(exc) # Checks against unknown library and __cause__ are needed
What I'm proposing in this issue is that the public API dohq.exceptions.ArtifactoryException
includes an optional http_error
, since you're the only ones who know which lower-level libraries (and exceptions) you're accessing (i.e. Requests in this case). This is to leverage these checks from the users and avoid artificial dependencies.
If somebody can guide me through that part of your code, I would gladly prepare a PR.
hmm, I see what you mean.
At the same time, where to draw the line of what should be attached to ArtifactoryException
?
only http code? http msg ?
the simplest would be to attach the original response, but that will not solve the issue, since you anyway will depend on the implementation detail then
@allburov thoughts on this ?
I don't know the complexity of your project, but perhaps in this case it's worth having a Base exception and different exceptions types inheriting?
class ArtifactoryException(Exception):
"""Base exception for all custom exceptions in dohq-artifactory."""
pass
class ArtifactoryHTTPError(ArtifactoryException): # PEP-8 recommends ending exception names with 'Error'
def __init__(self):
# ... perhaps inherit from requests.exceptions.HTTPError?
class ArtifactoryConfigError(ArtifactoryException):
"""Probably worth adding docstrings."""
I'm not a huge fan of custom exceptions, but I think that your dependency to requests
leads you this way...
Iff you only use ArtifactoryException
for HTTP errors and reraised ArtifactoryExceptions are always/only raised from exceptions.requests.HTTPError
. then I'd be ok by inspecting __cause__
. You'd have to update your documentation to mention this, as this is your public API. I'd then live by this contract, avoid checks ala isinstance(exception, requests.exceptions.HTTPError)
and have some peace of mind.
From https://github.com/devopshq/artifactory?tab=readme-ov-file#exception-handling:
Exceptions in this library are represented by dohq_artifactory.exception.ArtifactoryException or by OSError. Exceptions including __cause__ are always a facade of `requests.exceptions.HTTPError`, so you can always drill down the root cause like in the following example:
I agree on the point that we shouldn't even show the underlying library (requests) we use to make http requests and we should allow to switch that library by providing some client
(in the ideal word...)
what do you think of my proposal with the base exception?
It looks to me like the current implementation of
ArtifactoryException
currently only includes the http error code as part of the string message (e.g.dohq_artifactory.exception.ArtifactoryException: 502 Server Error: Bad Gateway for url: https://myserver.com/artifactory/api/storage/my-repo/path/4242/directory
Unfortunately our infrastructure is sometimes unstable so we need to retry some operations in case of some specific errors (e.g. 429).We prefer not to parse the exception message string, as there are some numbers (IDs) in the URL and we would create a dependency to the format of your exception message.
Instead, we think that including an optional (i.e. None on initialization)
http_error_code
would greatly help us. That is anyway what theHTTPError
ofrequests
includes: https://github.com/psf/requests/blob/main/src/requests/exceptions.py#L22