Conditions under which Maxwell.Error are raised

[bitwalker]

@zhongwencool I think there is another important change we should make. Currently, if you call get!/1, it raises if the status code is not in the range 200..299 or if an error is returned. I think raising in this situation isn't a good idea. If the request succeeded, but the status code wasn't successful, say 404, we should still return Maxwell.Conn.t instead of raising, and let the user check the status code to determine what to do with the request, and only raise Maxwell.Error if an actual adapter/middleware error occurs.

Thoughts?

[zhongwencool]

def user_repos(username) do
new()
|> put_path("/users/" <> username <> "/repos")
|> put_req_header("user-agent", "hackney")
|> get!()
|> get_resp_body()

The original purpose is for get_resp_body/1 don't need check status code.
But, you make well reasoned points, 2xx ~ 4xx should not raise, But maybe 5xx should be raise? it's not user's fault lead to 5xx.

[bitwalker]

I think code like that is quite error prone unfortunately, for example, status 206 means it's only partial content, and not all 200 status codes return a response body (e.g. 204). Some status codes in the 300 range are not a problem (like 304), but others are (like 300, or 303). Anything in the 400-500 ranges will potentially have a body, but not what the user expects, which will break their pipeline as well.

My point being, blindly asking for the response body and then piping into something else without checking status codes explicitly is a bad idea. Building up the connection and then executing it at the end is a perfect use case for pipelining, but I think one should bind the result and check it after that in order to do the correct thing.

I know I was already bitten by this behaviour since all other HTTP clients I've used only raise errors when actual exceptional conditions occur, such as the adapter failing to establish a connection. I consider getting a response of any kind successful, and I think most other users will expect that as well. This is also how Tesla works, and if we're interested in getting the two projects aligned, I think this might be a good change to make in that direction.

I'll respect your decision either way here, but I would strongly recommend reconsidering the current behaviour, as I think it's an error-prone API to expose to users.

[zhongwencool]

how about:

def unquote(method_exception)(conn, expect_status \\ nil ) do
          case unquote(method)(conn) do
            {:ok, %Maxwell.Conn{status: status} = new_conn} -> 
             case expect_status  do:
                  nil ->  ok;
                  _  is_integer(expect_status) -> 
                        unless status == expect_statuses do
                            raise Maxwell.Error, {__MODULE__, :response_status_not_match, new_conn}
                        end
                  _  is_list(expect_status) -> 
                      unless status in expect_statuses do
                         raise Maxwell.Error, {__MODULE__, :response_status_not_match, new_conn}
                     end
            end;
            {:error, reason, new_conn}  -> raise Maxwell.Error, {__MODULE__, reason, new_conn}
          end
       new_conn
end

I want to keep get!/2.

[secretworry]

@bitwalker @zhongwencool In general, when to raise an error should be determined by the expectation of the invoker.
Accessing a URL/URI should be considered as accessing a resource, just like a file or an io device. You expect it to return the valid resource, otherwise, it should raise an error. So when you read a non-exist file with File.read! you get a FileError(404), when you read from a scrapped device, you get a FileError(5xx).
i.e. we should raise an error for 4xx & 5xx status codes, cuz when you use the bang functions they are not what we expect to return from a URL.

Even we consider a remote endpoint's failure to be expected ( return normal result as usual), we should at least raise an error for 4xx, cuz, 4xx indicates a client error and invoker could not fix an error caused by itself ( you should not expect you code to live through an ArgumentError, or a UndefinedFunctionError).

As for the argument for 206, 204, I think all the 2xx indicate a success, as the Status Code Definitions defined, so the clients are expected to handle all the possible status codes, cuz it should be part of the protocol.

If users need to handle the content from a 4xx or 5xx response, they should turn to the functions without bang(!), cuz this function take all kinds of status as expected.

[bitwalker]

I think you make good points @secretworry. I've converted over to using the non-bang versions anyway due to this behavior, though I think it still potentially could throw people off coming from other HTTP clients, but as long as it's clearly documented with attention drawn to the difference, it's not a big deal. I'll change this to a documentation issue.

[doughsay]

Sorry to jump in late, but I want to make the opposite case about error expectations in http clients.

The comparison with file reading is interesting, but I don't think it applies here. Http services vary immensely with how they are implemented and how they are meant to be consumed. In some cases, 404s are expected responses, not unexpected. Even 500s, in my opinion, are still not worthy of raising an error, because the request/response cycle completed successfully, and the response may even have a body.

I've always lived by one rule when it comes to general purpose http clients: You only raise on connection errors. i.e. the connection couldn't be established, the host does not exist, connection dropped half way through, server responded with non-http response, etc. If you got a valid http response from a server after making a request, you shouldn't raise an error. It's the client implementors job to interpret http response codes the way they need for their particular situation.

Anyway, just my 2 cents!