Derive HasClient good response status from Verb status#1469
Derive HasClient good response status from Verb status#1469tchoutri merged 9 commits intohaskell-servant:masterfrom
Conversation
548a80b to
3c6a2b5
Compare
alpmestan
left a comment
There was a problem hiding this comment.
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.
|
You got it boss |
3c6a2b5 to
50bc905
Compare
| let p = Person "Clara" 42 | ||
| left show <$> runClient (getBody p) baseUrl `shouldReturn` Right p | ||
|
|
||
| it "Servant.API.Get redirection" $ \(_, baseUrl) -> do |
There was a problem hiding this comment.
The idea of this test being that before your patch, we'd get a ClientError because we don't get a 2xx status code?
There was a problem hiding this comment.
Yes, that's the idea. I also checked the same test in master and it returns, as expected, a FailureResponse.
There was a problem hiding this comment.
Btw, this is similar to the "Redirects when appropriate" test, that uses uverbGetSuccessOrRedirect True, but for Verb.
There was a problem hiding this comment.
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.
I had a look at how to add this test, and I was not sure how to proceed. type ServerAPI = Verb GET 201 '[JSON] ()
type ClientAPI = Verb GET 200 '[JSON] ()I tried to use also the free monad client in the |
|
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, |
I added the |
Hi Everyone,
There's a limitation/bug in the
HasClientinstances for theVerbdatatype.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 ofrunRequestin theclientWithRoutedefinitions.This PR brings the following changes:
runRequestwithrunRequestAcceptStatusin theVerbinstances for theHasClientclass (as suggested by @alpmestan).Verbstatus.statusIsSuccessfulfromNetwork.HTTP.Types.Statusto lighten the code (not required to solve the issue).