Repository: wojtekmach/req
Pull request #435 (2024-11-22 12:17:16)

Ignore :into collectable for non-200 responses
Currently when we pass `into: collectable`, the body is streamed into the collectable, regardless the status. This can be problematics for collectables with side effects. For example, I imagine a common use case may be `into: File.stream!(path)` to download a file, and in case of an error, we would save error message body instead.

There is one concern, that I'm not sure how to solve, hence a draft (details in separate comment).

wojtekmach: cc @acalejos, I think you run into this recently!
acalejos: > cc @acalejos, I think you run into this recently!

Yes I think I did! When a redirect occurred, if they body of the redirecting URL wasn't empty it would trigger the `:into` function. Expected behavior IMO would be for it to only trigger after all redirects, but Im not sure if it's possible for it to be aware of any redirects at the point when `:into` is called since it's called before response steps and redirects aren't always enabled
jonatanklosko: > it would trigger the :into function

You mean `into: fn {:data, data}, {req, resp} ->`, right? Since you have `resp` at that point, you could just pattern match on the resp status, and if it's not 200, you ignore the data, so a redirect would just go through. Though one could make an argument that it should only be called on 200 as well, and if needed we can have a option `into_on_any_status: true` applicable to both. But then the final question is what about `:self`, perhaps on non-200 req receives all messages itself and doesn't return async body? @wojtekmach I can send for any of these, just let me know :)

That said, this PR does fix the redirect case for `into: collectable`. Previously a redirect could result in the collectable invoked multiple times. Now, it will only happen for the final 200 response.
acalejos: @jonatanklosko 

Yes, it was `:into` a function for my case (seen here: https://github.com/acalejos/Reed/blob/64832bc789916e0b2a7ab5347328833846846113/lib/reed/req_plugin.ex#L44)

You're right I should pattern match on the status in the resp. I think for now Im just letting the XML parser error on the data and rescuing it and letting that be the way to ignore it, so I should change that 😅

Patch:
diff --git a/README.md b/README.md
index 015ecf0..8f7d27f 100644
--- a/README.md
+++ b/README.md
@@ -148,15 +148,9 @@ Req.get!(req, url: "/repos/wojtekmach/req").body["description"]
 #=> "Req is a batteries-included HTTP client for Elixir."
 ```
 
-Custom steps can be packaged into plugins so that they are even easier to use by others.
-Here are some examples:
+Custom steps can be packaged into plugins so that they are even easier to use by others. See [Related Packages](#related-packages).
 
-  * [`req_easyhtml`]
-  * [`req_s3`]
-  * [`req_hex`]
-  * [`req_github_oauth`]
-
-And here is how they can be used:
+Here is how they can be used:
 
 ```elixir
 Mix.install([
@@ -208,6 +202,16 @@ Req.get!(req, url: "https://api.github.com/user").body["login"]
 
 See [`Req.Request`] module documentation for more information on low-level API, request struct, and developing plugins.
 
+## Related Packages
+
+There are many packages that extend the Req library. To get yours listed here, send a PR.
+
+  * [`req_easyhtml`]
+  * [`req_s3`]
+  * [`req_hex`]
+  * [`req_github_oauth`]
+  * [`curl_req`]
+
 ## Presentations
 
   * [Req: A batteries-included HTTP client for Elixir - ElixirConf 2023, 2023-09-08](https://www.youtube.com/watch?v=owz2QacFuoQ "ElixirConf 2023 - Wojtek Mach - Req - a batteries-included HTTP client for Elixir")
@@ -270,3 +274,4 @@ limitations under the License.
 [`req_hex`]: https://github.com/wojtekmach/req_hex
 [`req_github_oauth`]: https://github.com/wojtekmach/req_github_oauth
 [`Mix.install/2`]: https://hexdocs.pm/mix/Mix.html#install/2
+[`curl_req`]: https://github.com/derekkraan/curl_req
diff --git a/lib/req.ex b/lib/req.ex
index b542c98..0f7198f 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 b931b4a..2cd7627 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
 


Test patch:
diff --git a/test/req/finch_test.exs b/test/req/finch_test.exs
index 54b193b..c8135dd 100644
--- a/test/req/finch_test.exs
+++ b/test/req/finch_test.exs
@@ -346,6 +346,24 @@ defmodule Req.FinchTest do
       assert resp.body == ["chunk1", "chunk2"]
     end
 
+    test "into: collectable non-200" do
+      # Ignores the collectable and returns body as usual
+
+      %{url: url} =
+        start_http_server(fn conn ->
+          Req.Test.json(%{conn | status: 404}, %{error: "not found"})
+        end)
+
+      resp =
+        Req.get!(
+          url: url,
+          into: :not_a_collectable
+        )
+
+      assert resp.status == 404
+      assert resp.body == %{"error" => "not found"}
+    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 6f095c7..e9565f6 100644
--- a/test/req/steps_test.exs
+++ b/test/req/steps_test.exs
@@ -1927,6 +1927,26 @@ defmodule Req.StepsTest do
       refute_receive _
     end
 
+    test "into: collectable non-200" do
+      # Ignores the collectable and returns body as usual
+
+      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: :not_a_collectable
+        )
+
+      resp = Req.request!(req)
+      assert resp.status == 404
+      assert resp.body == "foobar"
+      refute_receive _
+    end
+
     test "errors" do
       req =
         Req.new(