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

Add Req.TransportError and Req.Test.transport_error/2 #323

Merged
merged 5 commits into from
Mar 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/req/checksum_mismatch_error.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
defmodule Req.ChecksumMismatchError do
@moduledoc """
Represents a checksum mismatch error returned by `Req.Steps.checksum/1`.
"""

defexception [:expected, :actual]

@impl true
def message(%{expected: expected, actual: actual}) do
"""
checksum mismatch
Expand Down
66 changes: 63 additions & 3 deletions lib/req/steps.ex
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,9 @@ defmodule Req.Steps do
["Adapter" section in the `Req.Request`](Req.Request.html#module-adapter) module documentation
for more information on adapters.

Finch returns `Mint.TransportError` exceptions on HTTP connection problems. These are automatically
converted to `Req.TransportError` exceptions.

## HTTP/1 Pools

On HTTP/1 connections, Finch creates a pool per `{scheme, host, port}` tuple. These pools
Expand Down Expand Up @@ -757,6 +760,11 @@ defmodule Req.Steps do

iex> Req.get!(url, connect_options: [transport_opts: [cacerts: :public_key.cacerts_get()]])

Transport errors are represented as `Req.TransportError` exceptions:

iex> Req.get("https://httpbin.org/delay/1", receive_timeout: 0, retry: false)
{:error, %Req.TransportError{reason: :timeout}}

Stream response body using `Finch.stream/5`:

fun = fn request, finch_request, finch_name, finch_options ->
Expand Down Expand Up @@ -876,6 +884,9 @@ defmodule Req.Steps do
{:ok, acc} ->
acc

{:error, %Mint.TransportError{reason: reason}} ->
{req, %Req.TransportError{reason: reason}}

{:error, exception} ->
{req, exception}
end
Expand Down Expand Up @@ -909,6 +920,9 @@ defmodule Req.Steps do
acc = collector.(acc, :done)
{req, %{resp | body: acc}}

{:error, %Mint.TransportError{reason: reason}} ->
{req, %Req.TransportError{reason: reason}}

{:error, exception} ->
{req, exception}
end
Expand Down Expand Up @@ -977,8 +991,14 @@ defmodule Req.Steps do

defp run_finch_request(finch_request, finch_name, finch_options) do
case Finch.request(finch_request, finch_name, finch_options) do
{:ok, response} -> Req.Response.new(response)
{:error, exception} -> exception
{:ok, response} ->
Req.Response.new(response)

{:error, %Mint.TransportError{reason: reason}} ->
%Req.TransportError{reason: reason}

{:error, exception} ->
exception
end
end

Expand Down Expand Up @@ -1156,6 +1176,18 @@ defmodule Req.Steps do

assert Req.get!(plug: plug, into: []).body == ["echoecho"]
end

You can simulate failure conditions by returning exception structs from your plugs.
For network related issues, use `Req.TransportError` exception:

test "network issues" do
plug = fn _conn ->
%Req.TransportError{reason: :econnrefused}
end
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @whatyouhide @josevalim I have a favour ask, what do you think about this API?

I initially thought about returning just an atom:

plug = fn _conn ->
  :econnrefused
end

or to stand out even more from the Plug contract (which obviously does not supports it) maybe throw(:econnrefused) but figured exception is extensible. I hope not but maybe people want to check for Mint.HTTPError, Finch.HTTPError, etc. Which I'm not standardising on btw, meaning, Finch adapter turns Mint.TransportError to Req.TransportError but other ones it might return are returned as is.

Any feedback appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this more than the atom.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is better than the atom indeed. You could also have some sort of Req.Plug.halt(conn, reason), if you want to keep the Plug contract, and then you store the reason somewhere in the connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a very good idea, because it allows you to keep the public API flexible!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reason being exception struct or atom and friends?

maybe Req.Test.transport_error(conn, reason)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think Req.Test.transport_error(conn, reason) is the same idea, but it reads better. I also like that it keeps it scoped to Req.Test. reason would be an atom, so that

Req.Test.transport_error(conn, :timeout)

essentially stores conn.private.req_transport_error = %Req.TransportError{reason: :timeout}, which then you can use to return the error. Thoughts?


assert Req.get(plug: plug, retry: false) ==
{:error, %Req.TransportError{reason: :econnrefused}}
end
"""
@doc step: :request
def put_plug(request) do
Expand Down Expand Up @@ -1195,8 +1227,36 @@ defmodule Req.Steps do
conn = Plug.Test.conn(request.method, request.url, req_body)

conn = put_in(conn.req_headers, req_headers)
conn = call_plug(conn, plug)

case call_plug(conn, plug) do
%Plug.Conn{} = conn ->
handle_plug_result(conn, request)

%Req.TransportError{} = exception ->
validate_transport_error!(exception.reason)
{request, exception}

%_{__exception__: true} = exception ->
{request, exception}
end
end

defp validate_transport_error!(:protocol_not_negotiated), do: :ok
defp validate_transport_error!({:bad_alpn_protocol, _}), do: :ok
defp validate_transport_error!(:closed), do: :ok
defp validate_transport_error!(:timeout), do: :ok

defp validate_transport_error!(reason) do
case :ssl.format_error(reason) do
~c"Unexpected error:" ++ _ ->
raise ArgumentError, "unexpected Req.Transport reason: #{inspect(reason)}"

_ ->
:ok
end
end

defp handle_plug_result(conn, request) do
# consume messages sent by Plug.Test adapter
{_, %{ref: ref}} = conn.adapter

Expand Down
5 changes: 5 additions & 0 deletions lib/req/too_many_redirects_error.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
defmodule Req.TooManyRedirectsError do
@moduledoc """
Represents an error when too many redirects occured, returned by `Req.Steps.redirect/1`.
"""

defexception [:max_redirects]

@impl true
def message(%{max_redirects: max_redirects}) do
"too many redirects (#{max_redirects})"
end
Expand Down
17 changes: 17 additions & 0 deletions lib/req/transport_error.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
defmodule Req.TransportError do
@moduledoc """
Represents an error with the transport used by an HTTP connection.

This is a standardised exception that all Req adapters should use for transport layer related
wojtekmach marked this conversation as resolved.
Show resolved Hide resolved
errors.

This exception is based on `Mint.TransportError`.
"""

defexception [:reason]

@impl true
def message(%__MODULE__{reason: reason}) do
Mint.TransportError.message(%Mint.TransportError{reason: reason})
end
end
33 changes: 30 additions & 3 deletions test/req/steps_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1777,6 +1777,33 @@ defmodule Req.StepsTest do
assert ["defmodule Req.MixProject do" <> _] = resp.body
refute_receive _
end

test "errors" do
req =
Req.new(
plug: fn _conn ->
%Req.TransportError{reason: :timeout}
end,
retry: false
)

assert Req.request(req) ==
{:error, %Req.TransportError{reason: :timeout}}
end

test "validate Req.TransportError reason" do
req =
Req.new(
plug: fn _conn ->
%Req.TransportError{reason: :bad}
end,
retry: false
)

assert_raise ArgumentError, "unexpected Req.Transport reason: :bad", fn ->
Req.request(req)
end
end
end

describe "run_finch" do
Expand Down Expand Up @@ -1849,7 +1876,7 @@ defmodule Req.StepsTest do
end)

req = Req.new(url: url, receive_timeout: 50, retry: false)
assert {:error, %Mint.TransportError{reason: :timeout}} = Req.request(req)
assert {:error, %Req.TransportError{reason: :timeout}} = Req.request(req)
assert_received :ping
end

Expand Down Expand Up @@ -2044,7 +2071,7 @@ defmodule Req.StepsTest do
test "into: fun handle error", %{bypass: bypass, url: url} do
Bypass.down(bypass)

assert {:error, %Mint.TransportError{reason: :econnrefused}} =
assert {:error, %Req.TransportError{reason: :econnrefused}} =
Req.get(
url: url,
retry: false,
Expand Down Expand Up @@ -2097,7 +2124,7 @@ defmodule Req.StepsTest do
test "into: collectable handle error", %{bypass: bypass, url: url} do
Bypass.down(bypass)

assert {:error, %Mint.TransportError{reason: :econnrefused}} =
assert {:error, %Req.TransportError{reason: :econnrefused}} =
Req.get(
url: url,
retry: false,
Expand Down
Loading