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.

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 error messages in exceptions from Artifactory #200

Open
BrukerJWD opened this issue Sep 30, 2024 · 0 comments
Open

Improve error messages in exceptions from Artifactory #200

BrukerJWD opened this issue Sep 30, 2024 · 0 comments

Comments

@BrukerJWD
Copy link

I was playing around with permissions and got an error 400. This is the output of the exception message:

requests.exceptions.HTTPError: 400 Client Error:  for url: https://myartifactory/artifactory/api/v2/security/permissions/all-test

400 is not much information about what went wrong, so I dug inside your code and the request library. Finally, I printed the correct variable and found that Artifactory returned this content:

{
  "errors" : [ {
    "status" : 400,
    "message" : "The user: \'username\'\' has admin privileges, and cannot be added to a Permission Target."
  } ]
}

This was a helpful error message :) (Although I disagree on the decision behind it).
Ignoring the error message returned by Artifactory in JSON seems not like a good idea to me. It would be better if the error message is also put into the exception which is thrown.

Currently I am wrapping calls to your library with my own exception handler to achieve this:

def get_errors_from_json_response(response: requests.Response) -> Optional[List[str]]:
    response_str: bytes = response.content
    try:
        response_dict: dict = json.loads(response_str)
    except:
        return None
    if not response_dict or not "errors" in response_dict:
        return None
    return [ f"{error['status']}: {error['message']}" for error in response_dict["errors"] ]

@contextlib.contextmanager
def exception_message(msg):
    # from https://stackoverflow.com/questions/17677680/how-can-i-add-context-to-an-exception-in-python
    def raise_with_new_msg(ex: Exception, new_msg: str):
        ex.args = (new_msg,) + ex.args[1:]
        raise
    def raise_and_add_message(ex: Exception, new_msg):
        msg = '{}: {}'.format(new_msg, ex.args[0]) if ex.args else str(new_msg)
        raise_with_new_msg(ex, msg)

    try:
        yield
    except HTTPError as ex:
        errors = get_errors_from_json_response(ex.response)
        if not errors:
            raise_and_add_message(msg)
        raise_with_new_msg(ex, f"{msg}: {'; '.join(errors)}")
    except Exception as ex:
        raise_and_add_message(ex, msg)

Of course, catching exceptions, modifying and re-throwing them is not the best idea. But otherwise I do not get the response object.
On the other hand, you could implement something like raise_for_status() yourself in _generic_http_method_request(), check if the response contains some json with errors and put that into the message, and otherwise use the default implementation:

    response: Response = http_method(
        f"{self._artifactory.url}/{route}",
        auth=self._auth,
        **kwargs,
        verify=self._verify,
        cert=self._cert,
    )
    if raise_for_status and response.status_code >= 400:
        errors = get_errors_from_json_response(response)
        if errors:
            raise ArtifactoryError("; ".join(errors))
        else:
            response.raise_for_status()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant