Should error responses raise an HTTPError by default?

[florimondmanca]

I raised this question on the chat, but I think it's worth a proper issue to discuss options…

Current situation and pain points

Currently, to be notified of an error response we need to either:

• Check the .status_code manually:

if response.status_code == 404:
    print("Not found")

if response.is_error:
    print("Error")

• Call .raise_for_status():

response.raise_for_status()  # Raises an HTTPError

This is mostly in line with what Requests does, but I'd argue we can do better.

In particular, there's a very basic use case we could address better: make a request, then handle any 4xx/5xx.

There are mostly two ways to do this currently:

• Inspect the .status_code:

import httpx

r = httpx.get("https://httpbin.org/status/500")
if r.status_code == 500:
    ...

• Call .raise_for_status() to get a proper HTTPError:

import httpx

def fetch(url):
    r = httpx.get(url)
    r.raise_for_status()
    return r

try:
    r = fetch("https://httpbin.org/status/500")
except httpx.HTTPError as exc:
    if exc.response.status_code == 500:
        ...

I think we can argue this looks somewhat clunky. It forces the user to add a .raise_for_status() line basically everywhere — probably as a wrapper around an HTTPX call — otherwise they'd start processing responses they think are valid, when they're actually errors.

Possible future situation

What if request methods automatically raised an HTTPError if we get an error response? Sounds sensible?

import httpx

try:
    r = httpx.get("https://httpbin.org/status/500")
except httpx.HTTPError as exc:
    print(exc.response.status_code)
    # Deal with 500...

This would allow users to have to think about error handling, as they'd be getting HTTPError exceptions thrown at them when hosts aren't able to satisfy the request. We can also easily add a "manual mode" without breaking backwards compat after 1.0…

Planning

If we think this is reasonable, the way I'd see us implement this is:

• Switch to raise an error as the default behavior before 1.0.
• This would be done via a raise_for_status=True flag, probably on the client constructor at first.
• Switch the docs to document the new default behavior, and document raise_for_status=False + response.raise_for_status/response.is_error as a more low-level/manual way of handling things.
• Later on (i.e. after 1.0), we could extend this to allow a Callable[[int], bool] too, allowing to do things like raise_for_status=lambda s: 500 <= s < 600 so that 4xx error responses don't raise an HTTPError. (Sounds fancy, but I can see this feature request turning up at some point.)

[StephenBrown2]

I was only halfway on board, until you mentioned being able to pass a Callable. Now that sold me! (Thinking of the potential for retry functionality...)

[florimondmanca]

Thinking of the potential for retry functionality...

I don’t think this is a critical point for implementing retries, is it? Checking for error responses would happen at the outermost layer of the client (in .send()), while I assume retries would be implement below that (similar to redirects and auth). Retries can (should?) check the status code manually — I mostly see this issue as a UX improvement, really.

[florimondmanca]

The part I’m not too sure about yet is where to put the raise_for_status flag.

Although this issue mentions putting it on the client, #754 puts it on request methods. That’s because I think it makes sense to be able to override the default behavior on a per-request basis, instead of having to build another client just because eg a specific endpoint returns a 403 you’d like to handle more manually.

But defining the behavior across a client also makes sense I suppose (especially to disable automatic raising of responses, eg if we porting from Requests).

So I guess the request level could be treated as an incremental improvement, and I should maybe update #754 to add a client-level flag first?

---

To be honest though, I’m only halfway convinced we should keep (read: advertise) a manual API at all...

There’s nothing we can do with a manual inspection of status codes and .raise_for_status that we can’t do with an API that raises an HTTPError containing a .response attribute. We can still inspect the status code and act accordingly, ie handle the exception or re-raise it.

The only benefit I can see of keeping the manual API available by the reach of a flag is to help with an incremental migration from Requests. But even then our manual API already isn’t 100% compatible (eg we don’t have .is_ok).

This doesn’t mean we won’t need a parameter at all in the future, if only to customize what it means for a response to be an error. But I think this can also be addressed via proper exception handling on the user side. After all, semantically a 403 doesn’t cease to be an error response just because you plan to maybe handle it somewhere in your error handling code. (What if you don’t/forget to?)

I think we could get away with no flag/callable parameter at all, for the sake of keeping the scope tight.

[lovelydinosaur]

I'm not sure about this. My initial reaction would be that it feels a bit blunt, and that exceptions should default to network failures etc. only.

Not absolutely against it, but it'd be a huge change in behaviour over requests, and we'd need to think long and hard about it.

Starting with a different question here:

If we wanted to introduce an optional flag on the client to auto raise on 4xx, 5xx, then:

1. What would we call that flag? Client(raise_on_status=True)? Client(auto_raise=True)?
2. What would we use want to name the flag on methods in cases where we want to disable it? .get(..., raise_on_status=False), .get(..., raise_on_status=True)?

(I'm not absolutely convinced about that one either, but it's a smaller point of design discussion to start with.)

[thebigmunch]

I'm not sure about this. My initial reaction would be that it feels a bit blunt, and that exceptions should default to network failures etc. only.

Not absolutely against it, but it'd be a huge change in behaviour over requests, and we'd need to think long and hard about it.

Same.

If we wanted to introduce an optional flag on the client to auto raise on 4xx, 5xx, then:

1. What would we call that flag? Client(raise_on_status=True)? Client(auto_raise=True)?
2. What would we use want to name the flag on methods in cases where we want to disable it? .get(..., raise_on_status=False), .get(..., raise_on_status=True)?

(I'm not absolutely convinced about that one either, but it's a smaller point of design discussion to start with.)

I always wondered why it wasn't named raise_on_error rather than raise_on_status. I could also see raise_if_error. Though I'm not sure HTTPX should deviate from requests with the naming if it's going to make this big of a behavior change.

[StephenBrown2]

I don’t think this is a critical point for implementing retries, is it?

It's not critical, unless the Callable was handed the request and response object (which the exception already contains: an API that raises an HTTPError containing a .response attribute.), in which case the callable could retry or not, just removing a try/except block really.

it'd be a huge change in behaviour over requests, and we'd need to think long and hard about it.

Seconded.

I always wondered why it wasn't named raise_on_error rather than raise_on_status.

Also seconded. raise_for_status means... what exactly? Especially since I can't specify the status code(s) to raise on if I wanted to: https://requests.readthedocs.io/en/master/api/#requests.Response.raise_for_status only raises stored HTTPError, if one occurred, which means that it could/should be folded into the Client/request easily as raise_on_error.

[florimondmanca]

Okay, I'd agree with down-sizing this a bit. Let's switch from "raise errors on error HTTP responses by default" to "add an optional flag to raise errors by default".

If we wanted to introduce an optional flag on the client to auto raise on 4xx, 5xx, then:

• What would we call that flag?

Options (showing the default value):

• raise_for_status: in line with response.raise_for_status(). Probably the most sane option unless we decide to change the response method too.
• raise_on_status: slightly more meaningful IMO.
• raise_on_error: not a good option IMO, because it might lead to thinking that passing False would not raise any error (which isn't true: it would still raise NetworkError, timeout exceptions, etc).

So, I guess the most sane option is raise_for_status. It would be disabled by default, and we could enabled it using…

r = httpx.get(..., raise_for_status=True)
httpx.Client(raise_for_status=True)

Also, another data point: aiohttp.ClientSession() provides a raise_for_status=False flag.

@StephenBrown2 About retries: I still don't think it's necessarily related to this issue. Any auto-raise-on-status functionality would happen right at the edge of the Client.send(), and retries would probably be implemented as an inner method. Something like…

async def send(self, request, ...):
    ...
    response = await self.send_handling_retries(request, ...)
    if self.raise_for_status:
        response.raise_for_status()
    ...
    return response

As long as we guarantee that an HTTPError raised in .send_handling_retries() corresponds an actual error while trying to get a response (as above), I really think retries and auto-raise-on-error-responses can be treated as separate, orthogonal features.

[florimondmanca]

Note that if we're going for an off-by-default optional flag (instead of on-by-default), it also means we can treat this as a possible post-1.0 enhancement…

[hugovk]

It would be good for migration too. For example, change this:

r = httpx.get("https://httpbin.org/status/404")
r.raise_for_status()

Into:

r = requests.get("https://httpbin.org/status/404")
r.raise_for_status()

Or:

r = httpx.get("https://httpbin.org/status/404", raise_for_status=True)