servant-client doesn't allow HasClient instances to handle non-2xx responses

[mheinzel]

Hi,

When handling a response with a non-2xx status code, the RunClient instance currently directly throws a ClientError FailureResponse. This contains the raw response body as a ByteString, but since it's an error, it short-circuits and doesn't get parsed in the HasClient instance.
The same thing applies to other client libraries like servant-http-streams and servant-client-ghcjs.

This means that combinators can't specify how to handle non-2xx responses, severely limiting the usefulness of some libraries, see e.g. this issue in servant-checked-exceptions.

I know there are some related discussions and work being done that could move information about expected return codes to the type level, but I'm not sure what the expected timeline for that is.

Do you think it would be worth doing something simple now that allows users of existing solutions like servant-checked-exceptions to get access to the responses they care about and handle them as they want?

One possibility I see would be to adapt the ClientEnv configuration records to allow supplying the (currently hardcoded) function that decides which responses are considered successful.
I created a simple proof of concept or servant-client and could do the same for the other client libraries here (plus adding test cases and some docs) if you are interested.

Thanks!

[alpmestan]

This is an interesting idea. Thoughts, @phadej?

Note that there's servant-uverb which is an attempt at supplying a verb combinator that lets you declare all the outcomes of a route explicitly. See https://github.com/wireapp/servant-uverb

[phadej]

I don’t remember details unfortunately The status code check would be perfectly done already in servant-client-core machinery, which doesn’t have ClientEnv yet. But that’s probably impossible atm, so then status checks have to overridable per http-lib supported: i.e. In both servant-client and servant-http-streams, perfectly also servant-jssaddle.

…

On 19 Dec 2019, at 8.56, Alp Mestanogullari ***@***.***> wrote: This is an interesting idea. Thoughts, @phadej? Note that there's servant-uverb which is an attempt at supplying a verb combinator that lets you declare all the outcomes of a route explicitly. See https://github.com/wireapp/servant-uverb — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[mheinzel]

so then status checks have to overridable per http-lib supported: i.e. In both servant-client and servant-http-streams, perfectly also servant-jssaddle

Exactly, that's the idea. I agree that a solution in servant-client-core would be nicer, but it seems like that would require a breaking change of the RunClient typeclass definition itself.

If you think the per-client extension of ClientEnv is the way to go, I will go ahead and create a PR with changes for -client, -http-streams, -jssaddle and -client-ghcjs. Maybe another one in the servant-js repo.

[phadej]

PR would be great

[fisx]

related: #841

[mheinzel]

Thanks for the reminder. I still have an in-progress branch for this, almost finished. I can turn it into a PR next weekend.

[coderfromhere]

Hi, stumbled upon this thread while using the client for checking non-2xx payloads. Any progress with the aforementioned PR?

[mheinzel]

Yeah, I just forgot about it at some point. I'll try wrapping it up in the next few days.