Ignore :into collectable for non-200 responses (#435)

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`.

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,18 +150,18 @@ 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)
         resp = update_in(resp.trailers, &Map.merge(&1, fields))
         {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}}
 

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`

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/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(

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(