Including http_error as attribute in exception

[FCamborda]

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 the HTTPError of requests includes: https://github.com/psf/requests/blob/main/src/requests/exceptions.py#L22

[beliaev-maksim]

We raise from requests, you should be able to parse it

https://github.com/devopshq/artifactory/blob/master/dohq_artifactory/exception.py#L21

[FCamborda]

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.

[beliaev-maksim]

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

[FCamborda]

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.

     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.

[beliaev-maksim]

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 ?

[FCamborda]

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:

[allburov]

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...)

[FCamborda]

what do you think of my proposal with the base exception?