Derive HasClient good response status from Verb status

[marinelli]

Hi Everyone,
There's a limitation/bug in the HasClient instances for the Verb datatype.
If I try to perform a client request with runClientM, I'll obtain a successful response if and only if the backend endpoint returns a response with status code >=200 and <300. This is due to the use of runRequest in the clientWithRoute definitions.

This PR brings the following changes:

• Replace runRequest with runRequestAcceptStatus in the Verb instances for the HasClient class (as suggested by @alpmestan).
• Derive the good response status from the Verb status.
• Use statusIsSuccessful from Network.HTTP.Types.Status to lighten the code (not required to solve the issue).

[alpmestan]

Looks great thanks, and sorry for the delay!

Do you perhaps feel like adding a test case where the server returns e.g 201 but the client wants 200, to make sure this doesn't break in the future?

@tchoutri I suspect we'll want to be very loud and clear about this when announcing the release that ships this, including in the changelog. This might even upset some users ("but the response is a 2xx, it should be fine!"), perhaps? I don't know.

[tchoutri]

You got it boss

[alpmestan]

The idea of this test being that before your patch, we'd get a ClientError because we don't get a 2xx status code?

[marinelli]

Yes, that's the idea. I also checked the same test in master and it returns, as expected, a FailureResponse.

[marinelli]

Btw, this is similar to the "Redirects when appropriate" test, that uses uverbGetSuccessOrRedirect True, but for Verb.

[alpmestan]

OK, sounds good. Since it wasn't 100% obvious to me initially, that might be the case for other contributors, perhaps indicating the purpose in the test label or a comment nearby would help. But the test otherwise looks good.

[marinelli]

Do you perhaps feel like adding a test case where the server returns e.g 201 but the client wants 200, to make sure this doesn't break in the future?

I had a look at how to add this test, and I was not sure how to proceed.
To test that the proper status code for the client is the one derived from the public API signature of the server, I would change the status code in the server response before the client receives it. The only way I found to do that is to create a copy of the public API signature of the server and create another one specifically for the client. They would be identical except for the status code in the endpoint we want to test.
E.g.:

type ServerAPI = Verb GET 201 '[JSON] ()
type ClientAPI = Verb GET 200 '[JSON] ()

I tried to use also the free monad client in the Servant.Client.Free. At the end, that client is usefull to simulate a client, not to verify if the real client reports errors as expected.

[alpmestan]

For testing this precise thing, I'd do exactly what you said, no need to appeal to the big common API type with a bunch of constructs in it.

Just the two tiny almost-identical, differing-only-in-status-code endpoint types you gave seem perfect. They're even short enough to write them inline, client (Proxy :: Proxy (Verb GET 200 '[JSON] ())), if you want.

[marinelli]

For testing this precise thing, I'd do exactly what you said, no need to appeal to the big common API type with a bunch of constructs in it.

Just the two tiny almost-identical, differing-only-in-status-code endpoint types you gave seem perfect. They're even short enough to write them inline, client (Proxy :: Proxy (Verb GET 200 '[JSON] ())), if you want.

I added the Servant.BrokenSpec module for checking client/server api inconsitencies.

[alpmestan]

LGTM. Thanks!