From d77b5b6a6a1979b4f34b838d93ee10cc7e334535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Fri, 22 Nov 2024 20:13:00 +0800 Subject: [PATCH 1/4] Ignore :into collectable for non-200 responses --- lib/req.ex | 3 +++ lib/req/finch.ex | 18 +++++++++++------- lib/req/request.ex | 3 +++ lib/req/steps.ex | 13 ++++++++----- test/req/finch_test.exs | 36 ++++++++++++++++++++++++++++++++++++ test/req/steps_test.exs | 26 ++++++++++++++++++++++++++ 6 files changed, 87 insertions(+), 12 deletions(-) diff --git a/lib/req.ex b/lib/req.ex index 2f7d4de..faebd0f 100644 --- a/lib/req.ex +++ b/lib/req.ex @@ -285,6 +285,9 @@ defmodule Req do into: File.stream!("path") + Note that the collectable is only used, if the response status is 200. In other cases, + the body is accumulated and processed as usual. + * `:self` - stream response body into the current process mailbox. Received messages should be parsed with `Req.parse_message/2`. diff --git a/lib/req/finch.ex b/lib/req/finch.ex index 633d4f0..4e86af3 100644 --- a/lib/req/finch.ex +++ b/lib/req/finch.ex @@ -131,12 +131,16 @@ defmodule Req.Finch do end defp finch_stream_into_collectable(req, finch_req, finch_name, finch_options, collectable) do - {acc, collector} = Collectable.into(collectable) resp = Req.Response.new() fun = fn - {:status, status}, {acc, req, resp} -> - {acc, req, %{resp | status: status}} + {:status, 200}, {nil, req, resp} -> + {acc, collector} = Collectable.into(collectable) + {{acc, collector}, req, %{resp | status: 200}} + + {:status, status}, {nil, req, resp} -> + {acc, collector} = Collectable.into("") + {{acc, collector}, req, %{resp | status: status}} {:headers, fields}, {acc, req, resp} -> resp = @@ -146,9 +150,9 @@ defmodule Req.Finch do {acc, req, resp} - {:data, data}, {acc, req, resp} -> + {:data, data}, {{acc, collector}, req, resp} -> acc = collector.(acc, {:cont, data}) - {acc, req, resp} + {{acc, collector}, req, resp} {:trailers, fields}, {acc, req, resp} -> fields = finch_fields_to_map(fields) @@ -156,8 +160,8 @@ defmodule Req.Finch do {acc, req, resp} end - case Finch.stream(finch_req, finch_name, {acc, req, resp}, fun, finch_options) do - {:ok, {acc, req, resp}} -> + case Finch.stream(finch_req, finch_name, {nil, req, resp}, fun, finch_options) do + {:ok, {{acc, collector}, req, resp}} -> acc = collector.(acc, :done) {req, %{resp | body: acc}} diff --git a/lib/req/request.ex b/lib/req/request.ex index 4aa08a5..6e50b96 100644 --- a/lib/req/request.ex +++ b/lib/req/request.ex @@ -92,6 +92,9 @@ defmodule Req.Request do into: File.stream!("path") + Note that the collectable is only used, if the response status is 200. In other cases, + the body is accumulated and processed as usual. + * `:options` - the options to be used by steps. The exact representation of options is private. Calling `request.options[key]`, `put_in(request.options[key], value)`, and `update_in(request.options[key], fun)` is allowed. `get_option/3` and `delete_option/2` diff --git a/lib/req/steps.ex b/lib/req/steps.ex index 33e332e..cea4483 100644 --- a/lib/req/steps.ex +++ b/lib/req/steps.ex @@ -1032,17 +1032,20 @@ defmodule Req.Steps do end collectable -> - {acc, collector} = Collectable.into(collectable) - response = Req.Response.new( status: conn.status, headers: conn.resp_headers ) - acc = collector.(acc, {:cont, conn.resp_body}) - acc = collector.(acc, :done) - {request, %{response | body: acc}} + if conn.status == 200 do + {acc, collector} = Collectable.into(collectable) + acc = collector.(acc, {:cont, conn.resp_body}) + acc = collector.(acc, :done) + {request, %{response | body: acc}} + else + {request, %{response | body: conn.resp_body}} + end end end diff --git a/test/req/finch_test.exs b/test/req/finch_test.exs index 54b193b..1297162 100644 --- a/test/req/finch_test.exs +++ b/test/req/finch_test.exs @@ -346,6 +346,42 @@ defmodule Req.FinchTest do assert resp.body == ["chunk1", "chunk2"] end + @tag :tmp_dir + test "into: collectable non-200", %{tmp_dir: tmp_dir} do + # Ignores the collectable and returns body as usual + + File.mkdir_p!(tmp_dir) + file = Path.join(tmp_dir, "result.bin") + + %{url: url} = + start_tcp_server(fn socket -> + {:ok, "GET / HTTP/1.1\r\n" <> _} = :gen_tcp.recv(socket, 0) + + body = ~s|{"error": "not found"}| + + data = """ + HTTP/1.1 404 OK + content-length: #{byte_size(body)} + content-type: application/json + + #{body} + """ + + :ok = :gen_tcp.send(socket, data) + end) + + resp = + Req.get!( + url: url, + into: File.stream!(file) + ) + + assert resp.status == 404 + assert resp.body == %{"error" => "not found"} + + refute File.exists?(file) + end + test "into: collectable handle error" do assert {:error, %Req.TransportError{reason: :econnrefused}} = Req.get( diff --git a/test/req/steps_test.exs b/test/req/steps_test.exs index 7774dd7..1dd92c9 100644 --- a/test/req/steps_test.exs +++ b/test/req/steps_test.exs @@ -1927,6 +1927,32 @@ defmodule Req.StepsTest do refute_receive _ end + @tag :tmp_dir + test "into: collectable non-200", %{tmp_dir: tmp_dir} do + # Ignores the collectable and returns body as usual + + File.mkdir_p!(tmp_dir) + file = Path.join(tmp_dir, "result.bin") + + req = + Req.new( + plug: fn conn -> + conn = Plug.Conn.send_chunked(conn, 404) + {:ok, conn} = Plug.Conn.chunk(conn, "foo") + {:ok, conn} = Plug.Conn.chunk(conn, "bar") + conn + end, + into: [] + ) + + resp = Req.request!(req) + assert resp.status == 404 + assert resp.body == "foobar" + refute_receive _ + + refute File.exists?(file) + end + test "errors" do req = Req.new( From c2abf50c22ebc3b8f7ac72a939568b10215fe86f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Fri, 22 Nov 2024 20:22:57 +0800 Subject: [PATCH 2/4] Fix test --- test/req/steps_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/req/steps_test.exs b/test/req/steps_test.exs index 1dd92c9..f183b27 100644 --- a/test/req/steps_test.exs +++ b/test/req/steps_test.exs @@ -1942,7 +1942,7 @@ defmodule Req.StepsTest do {:ok, conn} = Plug.Conn.chunk(conn, "bar") conn end, - into: [] + into: File.stream!(file) ) resp = Req.request!(req) From f05c939dcc1965e4ee683cd8f660056e45c73dbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Sat, 23 Nov 2024 04:13:33 +0100 Subject: [PATCH 3/4] Update test/req/finch_test.exs Co-authored-by: Wojtek Mach --- test/req/finch_test.exs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/test/req/finch_test.exs b/test/req/finch_test.exs index 1297162..801c805 100644 --- a/test/req/finch_test.exs +++ b/test/req/finch_test.exs @@ -354,20 +354,8 @@ defmodule Req.FinchTest do file = Path.join(tmp_dir, "result.bin") %{url: url} = - start_tcp_server(fn socket -> - {:ok, "GET / HTTP/1.1\r\n" <> _} = :gen_tcp.recv(socket, 0) - - body = ~s|{"error": "not found"}| - - data = """ - HTTP/1.1 404 OK - content-length: #{byte_size(body)} - content-type: application/json - - #{body} - """ - - :ok = :gen_tcp.send(socket, data) + start_http_server(fn conn -> + Req.Test.json(%{conn | status: 404}, %{error: :not_found}) end) resp = From 7c017278d98c94f40ede0ee3e2f836be36bce449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Sat, 23 Nov 2024 11:14:40 +0800 Subject: [PATCH 4/4] Up --- test/req/finch_test.exs | 12 +++--------- test/req/steps_test.exs | 10 ++-------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/test/req/finch_test.exs b/test/req/finch_test.exs index 801c805..c8135dd 100644 --- a/test/req/finch_test.exs +++ b/test/req/finch_test.exs @@ -346,28 +346,22 @@ defmodule Req.FinchTest do assert resp.body == ["chunk1", "chunk2"] end - @tag :tmp_dir - test "into: collectable non-200", %{tmp_dir: tmp_dir} do + test "into: collectable non-200" do # Ignores the collectable and returns body as usual - File.mkdir_p!(tmp_dir) - file = Path.join(tmp_dir, "result.bin") - %{url: url} = start_http_server(fn conn -> - Req.Test.json(%{conn | status: 404}, %{error: :not_found}) + Req.Test.json(%{conn | status: 404}, %{error: "not found"}) end) resp = Req.get!( url: url, - into: File.stream!(file) + into: :not_a_collectable ) assert resp.status == 404 assert resp.body == %{"error" => "not found"} - - refute File.exists?(file) end test "into: collectable handle error" do diff --git a/test/req/steps_test.exs b/test/req/steps_test.exs index f183b27..94882fa 100644 --- a/test/req/steps_test.exs +++ b/test/req/steps_test.exs @@ -1927,13 +1927,9 @@ defmodule Req.StepsTest do refute_receive _ end - @tag :tmp_dir - test "into: collectable non-200", %{tmp_dir: tmp_dir} do + test "into: collectable non-200" do # Ignores the collectable and returns body as usual - File.mkdir_p!(tmp_dir) - file = Path.join(tmp_dir, "result.bin") - req = Req.new( plug: fn conn -> @@ -1942,15 +1938,13 @@ defmodule Req.StepsTest do {:ok, conn} = Plug.Conn.chunk(conn, "bar") conn end, - into: File.stream!(file) + into: :not_a_collectable ) resp = Req.request!(req) assert resp.status == 404 assert resp.body == "foobar" refute_receive _ - - refute File.exists?(file) end test "errors" do