Fix Coflux.Handlers.Utils.read_json_body/1 and handlers that used it

[mediremi]

Coflux.Handlers.Utils.read_json_body/1 had two issues:

1. It didn't return req when Jason.decode/1 failed
2. It didn't handle {:more, data, req} returned by :cowboy_req.read_body/1 when the body is
   larger than the default 8MB chunk length

Also, handlers that used read_json_body assumed that it would return {:error, :invalid_json} when in fact it returned {:error, %Jason.DecodeError{...}}.

Not returning req

From Cowboy's documentation:

Some of the cowboy_req functions return an updated Req object. They are the read, reply, set and delete functions. While ignoring the returned Req will not cause incorrect behavior for some of them, it is highly recommended to always keep and use the last returned Req object.

Ignoring the returned req can sometimes lead to transport errors and other similar transient errors -- I've personally observed this with my own Elixir services as a result of a buggy read_full_body helper that dropped req from a {:more, data, req}.

Tests

Here's some unit tests to exercise both fixes:

defmodule Coflux.Handlers.UtilsTest do
  use ExUnit.Case, async: false

  alias Coflux.Handlers.Utils

  # Tiny Cowboy handler that exercises Utils.read_json_body/1 and reports
  # the *shape* of the value it returned, so tests can assert on whether
  # the req was threaded through correctly.
  defmodule ProbeHandler do
    @moduledoc false

    def init(req, _opts) do
      orig_req = req
      result = Utils.read_json_body(req)

      {status, payload, resp_req} =
        case result do
          {:ok, body, new_req} ->
            {200, %{"shape" => "ok_3", "body" => body}, new_req}

          {:error, reason, new_req} ->
            {400, %{"shape" => "error_3", "reason" => inspect(reason)}, new_req}

          {:error, reason} ->
            # Bug path: read_json_body lost the updated req. We have no
            # post-read_body req to reply with, so we fall back to the
            # stale one captured at entry.
            {400, %{"shape" => "error_2_DROPPED_REQ", "reason" => inspect(reason)}, orig_req}

          other ->
            {500, %{"shape" => "unexpected", "value" => inspect(other)}, orig_req}
        end

      resp_req =
        :cowboy_req.reply(
          status,
          %{"content-type" => "application/json"},
          Jason.encode!(payload),
          resp_req
        )

      {:ok, resp_req, []}
    end
  end

  setup_all do
    {:ok, _} = Application.ensure_all_started(:cowboy)
    {:ok, _} = Application.ensure_all_started(:req)

    dispatch = :cowboy_router.compile([{:_, [{"/", ProbeHandler, []}]}])

    {:ok, _} =
      :cowboy.start_clear(
        :utils_test_listener,
        [{:port, 0}],
        %{env: %{dispatch: dispatch}}
      )

    port = :ranch.get_port(:utils_test_listener)
    on_exit(fn -> :cowboy.stop_listener(:utils_test_listener) end)

    {:ok, base_url: "http://127.0.0.1:#{port}"}
  end

  defp post(url, body) do
    Req.post(url,
      body: body,
      headers: [{"content-type", "application/json"}],
      decode_body: false,
      retry: false
    )
  end

  defp post_and_decode(url, body) do
    case post(url, body) do
      {:ok, resp} -> {:ok, resp.status, Jason.decode!(resp.body)}
      {:error, reason} -> {:error, reason}
    end
  end

  describe "read_json_body/1 — happy path (sanity)" do
    test "returns {:ok, body, req} for a valid JSON body", %{base_url: url} do
      assert {:ok, status, body} =
               post_and_decode(url, Jason.encode!(%{"hello" => "world"}))

      assert status == 200
      assert body["shape"] == "ok_3"
      assert body["body"] == %{"hello" => "world"}
    end
  end

  describe "read_json_body/1 — bug: drops req on JSON decode error" do
    # Demonstrates the bug in server/lib/coflux/handlers/utils.ex:191-198.
    #
    #   def read_json_body(req) do
    #     case :cowboy_req.read_body(req) do
    #       {:ok, data, req} ->
    #         with {:ok, result} <- Jason.decode(data) do
    #           {:ok, result, req}
    #         end
    #     end
    #   end
    #
    # When Jason.decode/1 fails, the `with` returns the bare
    # {:error, %Jason.DecodeError{}} tuple — the updated req from
    # :cowboy_req.read_body/1 is silently discarded.
    #
    # The ProbeHandler above will fall into its `{:error, reason}` clause
    # and label the shape "error_2_DROPPED_REQ". A correct implementation
    # would return a 3-tuple including the threaded req, producing
    # "error_3".
    test "returns a 3-tuple including the threaded req for invalid JSON", %{base_url: url} do
      case post_and_decode(url, "not json") do
        {:ok, _status, body} ->
          assert body["shape"] == "error_3",
                 """
                 read_json_body/1 dropped the req on a JSON decode error.

                 ProbeHandler observed: #{inspect(body)}

                 Expected the function to return {:error, reason, req},
                 but it returned a bare 2-tuple {:error, reason}, forcing
                 the caller to reply with the stale pre-read_body req.
                 """

        {:error, reason} ->
          flunk("""
          read_json_body/1 dropped the req on a JSON decode error, and the
          fallback reply (using the stale pre-read_body req) left Cowboy in
          a state where the response never reached the client.

          Transport error: #{inspect(reason)}

          This is the textbook symptom of dropping a Cowboy req: the
          response is lost / the connection is reset.
          """)
      end
    end
  end

  describe "read_json_body/1 — bug: doesn't handle :more chunks" do
    # Demonstrates the second bug in the same function. :cowboy_req.read_body/1
    # may return {:more, data, req} for partial reads — the default chunk
    # length is 8_000_000 bytes. The current implementation only matches
    # {:ok, data, req}, so a large body crashes the handler with a
    # CaseClauseError (observable as a 500 response / dropped connection).
    #
    # Tagged :slow because sending 8MB+ over the loopback is noticeably
    # slower than the other tests; skip with `mix test --exclude slow`.
    @tag :slow
    @tag timeout: 60_000
    test "accepts a body larger than the default read_body chunk size", %{base_url: url} do
      # 9MB of JSON — exceeds the cowboy read_body/1 default length of 8MB,
      # which forces at least one {:more, _, _} return before {:ok, _, _}.
      large_value = String.duplicate("x", 9 * 1024 * 1024)
      payload = Jason.encode!(%{"data" => large_value})

      case post(url, payload) do
        {:ok, resp} ->
          assert resp.status == 200,
                 """
                 read_json_body/1 failed on a body larger than 8MB.

                 Status: #{resp.status}
                 Body:   #{inspect(resp.body)}

                 The function only pattern-matches {:ok, data, req} from
                 :cowboy_req.read_body/1, so {:more, data, req} crashes the
                 handler with a CaseClauseError.
                 """

        {:error, reason} ->
          flunk("transport error on >8MB body: #{inspect(reason)}")
      end
    end
  end
end

These tests spin up a Cowboy server as that seemed to be the simplest way of testing cowboy handlers - the alternative being mocking with a library like meck. The main downside to spinning up a Cowboy server is that the tests can't be async.

I'd strongly recommend using Plug instead of directly using Cowboy, as it makes testing handlers much easier and does not require spinning up an actual web server.

(I didn't include the above tests as part of this PR as I think they would be clearer and shorter if/once coflux migrates to Plug)

[mediremi]

The diff for this file is much easier to read if you select the 'hide whitespace' PR diff view.

[joefreeman]

Thanks!