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 handling of API response with error message #89

Open
rsyring opened this issue Jan 21, 2023 · 1 comment
Open

Improve handling of API response with error message #89

rsyring opened this issue Jan 21, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@rsyring
Copy link

rsyring commented Jan 21, 2023

Enhancement description

The functions in http_requests should be improved so that error details returned in the API response are preserved.

The problem it solves

Currently, the functions in http_requests use response.raise_for_status() which throws a generic exception based on the HTTP response code. An error response from the API server can include additional details about why the request failed. For example, "At least one of supported fields should be set and non-empty" was stored in response.content for a recent API call I made that failed. But the exception was a generic 400 response exception and I had to make changes to the http_requests function to figure out what the real error was.

Alternatives

Catching the generic exceptions and logging the content. But since there is no one single entry point or method that can be overridden, it has to be done in multiple places, which is verbose. The best place to do it would be in http_requests.

@rsyring rsyring added the enhancement New feature or request label Jan 21, 2023
@rsyring
Copy link
Author

rsyring commented Jan 22, 2023

My workaround was to catch the exception earlier in the request/response lifecycle and add additional information:

class TimeoutSession(requests.Session):
    # Connect timeout and read timeout.  Request docs say to make it slightly
    # more than multiple of 3.  Use '_' in hope that if upstream adds a timeout to
    # the session in the future, it won't conflict.
    _timeout = (6.25, 30.25)

    def __init__(self, timeout=None):
        self._timeout = timeout or self._timeout
        super().__init__()

    def request(self, method, url, **kwargs):
        log.debug(kwargs)
        kwargs.setdefault('timeout', self._timeout)

        try:
            response = super().request(method, url, **kwargs)
            log.debug(response.content)
            response.raise_for_status()
            return response
        except requests.HTTPError as e:
            if text := e.response.text:
                raise requests.HTTPError(text, e.response) from e
            raise

The timeout stuff (#88) isn't related to this issue it just ended up in the same custom Session as this workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant