top | item 16898741

(no title)

llccbb | 7 years ago

I am confused about the test used here. The `requests` docs very plainly state that `Response.ok` only checks the status code. Looking into the codebase proves that as well. Is there a status code for "I am going to send bad-length content"?

This might be a UX problem and not a technical one. The author seems to think that `Response.ok` should be an all-purpose check.

discuss

order

setr|7 years ago

Its completely a UX issue, but it seems reasonable to me. I agree itS not surprising behavior given the docs, but I believe it should be added.

Its been a while since i used Requests, but iirc response.ok is basically syntax sugar; but it seems to me that in most valid usecases where you'd like this sugar (over being explicit in your actions), is when you'd like to verify the communications was correct. And malformed http is not correct. I imagine if you implemented a wrapper ok2, of correctness check + response.ok, you'd see 90% of response.ok become ok2

It seems to me to be a sensible check (validate that the http message meets the standard), that should exist in any http library at request's level. And response.ok seems like a wasted api slot, if its not meeting the full needs of its sugaring

Twirrim|7 years ago

Honestly? I'm with the author here. "Response.ok" should tell me that the response is ok. It's the most natural expectation. If it's only going to check status code, Response.status_code.ok would be more reasonable.

ubernostrum|7 years ago

It's the most natural expectation. If it's only going to check status code, Response.status_code.ok would be more reasonable.

Except that the status line is literally "HTTP 200 OK", and the 'ok' check is simply using the domain terminology. This is not a bad thing. And having 'request.ok' be a general "I have examined every possible aspect of this response for problems and found none" is probably impossible, despite being what you apparently expect.

The solution is not to rename the method, it's to also warn or error when there's something fishy in the response. Which is apparently what requests 3.x will do.

There's also the issue that the Content-Length header, while generally well-adopted, is optional (sending either Content-Length or Transfer-Encoding is a SHOULD in RFC 7230, not a MUST), and sending an incorrect Content-Length, while annoying, is not actually against the RFC (the only MUST NOT prohibitions on an incorrect Content-Length value in the response are when the request was a HEAD or a conditional GET).

llccbb|7 years ago

I agree with your point that there should be some baked in capability for a more comprehensive check on the HTTP response. There are probably some edge cases around this regarding stream=True (load header only, not content) and iter_content() which pulls chunks from a buffer so the entire content isn't loaded into memory at once. Probably surmountable.

FWIW Response.status_code is an int, so it might need to be a member function on Response.

luhn|7 years ago

If a HTTP response is malformed, the most natural expectation for an exception to be thrown.

I'm glad to see the requests 3.0 does this by default for invalid Content-Length.

Latty|7 years ago

To be fair, basically the whole reason requests exists is to try and have good UX for devs.