elixir-plug / plug

Compose web applications with functions
https://hex.pm/packages/plug
Other
2.88k stars 587 forks source link

Support returning {:ok, payload} from adapter.inform/3 #1193

Closed wojtekmach closed 1 year ago

wojtekmach commented 1 year ago

There is a minor Bandit bug where it looks like it does not support inform but it definitely does. Turns out using Plug.Conn.inform/3 worked but inform!/3 did not. The bug in Bandit was wrong return value which went unchecked in inform/3 but got incorrectly interpreted in inform!/3.

This is not a breaking change for inform!/3 as we were already raising but it could be considered a breaking change for inform/3 but in this particular case it seemed warranted to me.

cc @mtrudel

mtrudel commented 1 year ago

It'd actually be real nice if we could instead add a match on {:ok, adapter} and return that back to the caller. As it is this makes it impossible to track state (namely metrics) past an inform call. WDYT?

wojtekmach commented 1 year ago

@mtrudel to be a bit more precise, do you have something like this in mind?

  def inform(%Conn{} = conn, status, headers \\ []) do
    status_code = Plug.Conn.Status.code(status)
    adapter_inform(conn, status_code, headers)
-   conn
  end

  # simplified for brevity
  defp adapter_inform(%Conn{adapter: {adapter, payload}}, status, headers) do
-   :ok = adapter.inform(payload, status, headers)
+   {:ok, payload} = adapter.inform(payload, status, headers)
+   put_in(conn.adapter, {adapter, payload})
  end

With almost zero knowledge about this side of Plug, this looks sensible to me. 😅

mtrudel commented 1 year ago

Conceptually, yeah. Though in an additive sense (with a case statement) so as to be backwards compatible. Like here (and without the change to the private function further down)

wojtekmach commented 1 year ago

I believe this is ready. I renamed the PR to "Support returning {:ok, payload} from adapter.inform/3".

josevalim commented 1 year ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: