Allow collect_body/3 to be called just once

[josevalim]

Today if you call collect_body multiple times, you will get the body the first time and an empty body in the following ones. It would be nice if collect_body/3 instead returned {:error, :already_collected, conn} instead. In order for this to work, we will probably need to store this data or in the connect or in the adapter.

In the adapter is preferable although I am not sure if possible. Does cowboy gives us this information in the new API?

@scrogson, could you please take a look at this? :D After we get this in, we can release v0.5.0 with collect_body/3.

[scrogson]

@josevalim I'll look into that. Also, we need to s the non-deprecated functions from cowboy_req for reading the body. Currently we are using stream_body/2 which will be removed in 1.0. The new method for this is cowboy_req:body/2.

https://github.com/extend/cowboy/blob/0.10.0/src/cowboy_req.erl#L99

I'll work on updating this as well.

[ericmj]

@scrogson When you are adding support for cowboy 0.10.0 can you make the timeout configurable when collecting the body? Timeouts were added to the new functions for reading the body in cowboy. We really need that feature in hex :)

[scrogson]

After discussing with @josevalim in chat, we've decided to rename this to read_body/2. The signature is very simple:

def read_body(conn, opts \\ []), do: ... end

@ericmj the method will just delegate to cowboy_req:body/2 which will give you full control over the options passed.

Cowboy allows: length, read_length, read_timeout, transfer_decode (fn), and content_decode (fn) as options.

I'm currently working through a test failure and should have this in a PR in the next couple of days.

[ericmj]

Should we just pass the options through directly instead of making them part of the adapter interface?

[scrogson]

@ericmj not sure I understand what you mean. I'm passing the options directly into the adapter. As I understand it, we are reading the req_body from the adapters state.

def read_body(%Conn{adapter: {adapter, state}} = conn, opts \\ []) do
  case adapter.read_req_body(state, opts) do
    {:ok, data, state} ->
      {:ok, data, %{conn | adapter: {adapter, state}}}
    {:more, data, state} ->
      {:more, data, %{conn | adapter: {adapter, state}}}
  end
end

[ericmj]

@scrogson Plug is supposed to be generic for any adapter. Today we only have cowboy so the issue won't come up. But what if we add another webserver adapter that instead of :read_timeout uses :timeout as the name for the timeout option? We should define and document the options in Plug so that they work for all possible adapters.

[josevalim]

Yeah, I was talking to @scrogson and we have decided exactly that. We are going to use the same names as cowboy, but all supported options should be explicitly documented. We should never say "accepts the same options as cowboy".

[ericmj]

@josevalim @scrogson Awesome! 👍

[scrogson]

I've sent a PR for this work in #44

@josevalim and I have discussed that returning {:error, :already_collected, conn} is not really necessary.