New step: expect

[wojtekmach]

This is a replacement for https://hexdocs.pm/req/Req.Steps.html#handle_http_errors/1 which will be deprecated and removed in Req 1.0.

Some more context here:

• feat: add ok property to response and to_tuple func #188
• handle_http_errors: Support :response and :exception #193
• Allow to define what successful/unsuccessful response statuses are #467

In a nutshell:

- case Req.request(req) do
-   {:ok, %{status: 200}} = result ->
-     result
-
-   {:ok, resp} ->
-     exception =
-       RuntimeError.exception("""
-       unexpected status #{resp.status}
-
-       #{inspect(resp.body, pretty: true)}
-       """)
-
-     {:error, exception}
-
-   {:error, _} = error ->
-     error
- end
+ Req.request(req, expect: 200)

[wojtekmach]

maybe instead of storing the full response we keep just:

Suggested change

	defexception [:expected, :response]
	defexception [:expected_status, :status, :headers, :body]

i.e. no resp.private (nor resp.assigns, #492)

[wojtekmach]

Maybe we put it after decompress but before decode so the exception message just prints the string

[yordis]

I still feel ok literally how Fetch Web API works is a good idea, along with expect thingy, and familiar to other ecosystems.

expect still extremely valuable to control the flow from the domain perspective, while ok is useful in the infra (steps) perspective for most use cases.

At least for most OpenAPI, non-redirecting APIs; which are the most commons anyway.

I think it is worth it to keep it around, and trivial enough where it doesn't add much complexity, if any.

[wojtekmach]

Thanks for the feedback about expect. I concede response.ok is ubiquitous due to being part of the Web platform.

I think in pattern matches, by the way of falling into another code branch that is for transport errors, expect "wins":

  case resp do
-   {:ok, %{ok?: true} = resp -> ...
-   {:ok, resp} -> ...
-   {:error, e} -> ...
+   {:ok, resp} ->
+   {:error, e} -> ...
  end

In other cases

- if resp.status in 200..299 do
+ if resp.ok? do

yeah, that looks really clean. My personal opinion though is it doesn't justify adding a new field to the struct even if that's some false scarcity argument.

[yordis]

ok will always be 200..299 as the specs says, it is up the implementors to understand that behaviour. To be honest, the reason I keep coming back to this one (and I rather aligned here) is that it is such common case due to the popularity of JS and fetch.

Most likely useful for default steps behaviour and things like that, purely a semantic meaning that sometimes it is useful.

I have come across that 200..299 far too many times.

[yordis]

Suggested change

	iex> Req.get!("https://httpbin.org/status/404", expect: 200..299)
	iex> Req.get!("https://httpbin.org/status/404", expect: :ok)

could be an atom representing a given config as well? 😅

[wojtekmach]

Good question! Is :ok the only accepted atom?

If yes, I could maybe see that.

If not, I guess we support :not_found and friends? I think this reads well expect: [:ok, :not_found] and we could totally find typos (and Did you mean it) in atom names. But then again:

iex> Plug.Conn.Status.code(:ok)
200
iex> Plug.Conn.Status.code(:not_found)
404

so we probably shouldn't call it :ok because it's not clear whether it's the 200 OK or all 2XXs. The RFC groups statuses into:

• 1xx (Informational)
• 2xx (Successful)
• 3xx (Redirection)
• 4xx (Client Error)
• 5xx (Server Error)

but expect: :successful doesn't quite have the same ring to it.

So yeah, not sure. WDYT?

[whatyouhide]

expect: :success looks pretty good to me!

[yordis]

I would use RFC naming, althou somewhat annoying, it is the RFC so we avoid ambiguous language.

[jozuas]

Is there anything I can do to help move this forward? I've been waiting for this feature for a long time now 🫠

[wojtekmach]

I'm merging this as is. If someone wants to send a PR with expect: :informational | :successful | :redirection | :client_error | :server_error, that's very welcome!

[wojtekmach]

anyone has any objections to calling the step and option a more explicit:

expect_status: 200..299

?

I think it plays nicer which how I ended up calling the exception: Req.UnexpectedResponseStatusError. Could see shorter Req.UnexpectedStausError too, btw.

The main driver is there’s an expect header—-https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Expect

so :expect_status would totally disambiguate even if slightly more verbose.

thoughts?

[yordis]

@wojtekmach I rather it call it expect_status typing has become, effectively, 0 cost, but proper context has become 100% of the risk in the AI era.

Expect is already a thing in HTTP so ...