wojtekmach/req

`decompress_body`: Fails with MatchError if error tuple is returned

Closed this issue · 5 comments

The step decompress_body throws a match error when Brotli (and it appears some other compression) fails. I suggest we create a new Error module to halt and return when decompression fails so developer can manage error, instead of the hard failure below. Happy to submit a PR if that's the case...

iex(1)> Req.get!("https://ministrygear.com/good-bible-verses-t-shirts/")
** (MatchError) no match of right hand side value: :error
    (req 0.4.14) lib/req/steps.ex:1661: Req.Steps.decompress_body/3
    (req 0.4.14) lib/req/steps.ex:1639: Req.Steps.decompress_body/1
    (req 0.4.14) lib/req/request.ex:1010: anonymous fn/2 in Req.Request.run_response/2
    (elixir 1.15.6) lib/enum.ex:4830: Enumerable.List.reduce/3
    (elixir 1.15.6) lib/enum.ex:2564: Enum.reduce_while/3
    (req 0.4.14) lib/req/request.ex:938: Req.Request.run/1
    (req 0.4.14) lib/req.ex:1000: Req.request!/2
    iex:1: (file)

Upon inspection, it appears the zstd decompression may fail similarly, by returning {:error, reason} instead of the assumed binary() return type in

decompress_body(rest, :ezstd.decompress(body), acc)
- although in this case it passes the error tuple to the calling function which may confuse the source.

Good catch. My proposal is to return %Req.DecompressError{reason: term()}. Could you send a PR?

Actually please hold off for a bit.

Sure, no problem. This is causing us issues in production so I'll work on a solution regardless, but will hold off on creating a PR until you decide which way you'd like to go with it.

Feel free to submit a PR that returns RuntimeError.exception("#{codec} decompression failed: #{reason}"). That is, if it's going to work for your use case. I'd like to ponder a proper exception struct, like that Req.DecompressError`, some more and how it relates to other errors we may want to return.

Closing in favour of #335.