Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Bandit.HTTP1.Adapter.inform/3 return value #268

Merged
merged 2 commits into from Jan 16, 2024

Conversation

wojtekmach
Copy link
Contributor

Per https://hexdocs.pm/plug/Plug.Conn.Adapter.html#c:inform/3, the callback should return:

:ok | {:error, :not_supported}

The way Plug.Conn.inform! is implemented at the moment, https://github.com/elixir-plug/plug/blob/v1.15.2/lib/plug/conn.ex#L1335, it looked like Bandit did not implement inform but it sure does!

Per https://hexdocs.pm/plug/Plug.Conn.Adapter.html#c:inform/3, the
callback should return:

    :ok | {:error, :not_supported}

The way Plug.Conn.inform! is implemented at the moment,
https://github.com/elixir-plug/plug/blob/v1.15.2/lib/plug/conn.ex#L1335,
it looked like Bandit did _not_ implement inform but it sure does!
@mtrudel
Copy link
Owner

mtrudel commented Nov 22, 2023

Looks good! I'm going to add a small note to the bandit telemetry docs now that we're losing tracking of inform responses but otherwise lgtm!

@wojtekmach
Copy link
Contributor Author

would it help if the adapter allowed you to send {:ok, payload} or whatever makes the most sense? (I believe payload is a name used for arbitrary data, at least in adapter.send_resp/4)

If so that would be a compatible change in Plug and I can help pursuing it.

@mtrudel
Copy link
Owner

mtrudel commented Nov 22, 2023

would it help if the adapter allowed you to send {:ok, payload} or whatever makes the most sense?

Ideally, yeah. Returning bare :ok's is a bit of an anti pattern IMO. For example, Plug's chunk api also expects a bare :ok back from the adapter, which in addition to precluding tracking state for e.g. metrics, was also the cause of a recent oddball bug as well. Ideally everything in the adapter behaviour would return the a full tuple that took an updated adapter back, but that's not super realistic.

Happy to hew whichever way the upstream Plug issue wants to go here. Let's keep the discussion going over there.

@wojtekmach
Copy link
Contributor Author

wojtekmach commented Nov 23, 2023

We need to wait for elixir-plug/plug#1193 in case there's any change in the contract but otherwise I think this is ready. And when there's a new Plug version I'll send a PR that uses the stricter inform!, 18e12ed#diff-6a1dfdf6de8e0ba81a6ae4eef3f45cf79b7d4a55d7130e228463f9f77a0344e3R1339.

@mtrudel
Copy link
Owner

mtrudel commented Jan 16, 2024

New plug version is here!

@mtrudel mtrudel merged commit bfffb65 into mtrudel:main Jan 16, 2024
27 checks passed
@mtrudel
Copy link
Owner

mtrudel commented Jan 16, 2024

Merged this since it's erroring out on a couple of other PRs without this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants