Make adverse status codes not throw an exception

[bitemyapp]

I get a lot of people complaining about http-client (usually via http-conduit and wreq) throwing exceptions for error-y status codes.

I think what Python's requests library is going to be less surprising to people:

>>> import requests
>>> r = requests.get('http://httpstat.us/401')
>>> r
<Response [401]>
>>> r.status_code
401
>>> r = requests.get('http://httpstat.us/500')
>>> r
<Response [500]>

I realize this is a major change, but as usual I think it's something where we're better off the sooner we get it fixed.

[Yuras]

I'm sure you are aware of checkStatus. Setting it to always return Nothing gives the behavior you want, doesn't it?

Not sure my opinion counts, but I think the default behavior (throwing an exception on non-2xx status) makes perfect sense, and I have a lot of code that relies on that.

[bitemyapp]

@Yuras my point is about the default path. People coming to Haskell expecting "safety" find exceptions for a status code being non-100/200/300 very surprising.

[snoyberg]

As the docs mention, this is definitely a contentious issue, with people having strong reasons on both sides. Breaking the API in a silent way in such a situation is probably a bad idea.

On Fri, Apr 29, 2016, 11:29 PM Chris Allen notifications@github.com wrote:

@Yuras https://github.com/Yuras my point is about the default path. People coming to Haskell expecting "safety" find exceptions for a status code being non-100/200/300 very surprising.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/snoyberg/http-client/issues/193#issuecomment-215872442

[bitemyapp]

@snoyberg no doubt. I do see the benefits of using exceptions for this, but we don't want surprise either way. Long as it's on your radar. I'll chew on it.

[winterland1989]

Also mentioned in #23 , throwing on non-100/200/300 is okay depending on how you model HTTP protocol, the problem is that one may still want to access the respond body...etc inside the exception handler.

The way to do it with current API is to provide your own checkStatus, turn off throwing on non-100/200/300 , and check status code afterwards. so i suggest to provide a helper like:

-- | turn off throwing on non-100/200/300 respond. 
silentBadStatus :: Request -> Request
silentBadStatus req = req{ checkStatus = \_ _ _ _ -> Nothing}

I also ask to move checkStatus from request level to manager level, i can't see a particular use case that per-requset granularity checkStatus is necessary, but i may miss something here, please discuss on this.

[snoyberg]

I'm fine with making this a manager level setting as well, like we do with timeouts. Want to make a PR?

On Tue, May 24, 2016, 9:02 AM winterland notifications@github.com wrote:

Also mentioned in #23 https://github.com/snoyberg/http-client/issues/23 , throwing on non-100/200/300 is okay depending on how you model HTTP protocol, the problem is that one may still want to access the respond body...etc inside the exception handler.

The way to do it with current API is to provide your own checkStatus, turn off throwing on non-100/200/300 , and check status code afterwards. so i suggest to provide a helper like:

-- | turn off throwing on non-100/200/300 respond. silentBadStatus :: Request -> Request silentBadStatus req = req{ checkStatus = -> Nothing}

I also ask to move checkStatus from request level to manager level, i can't see a particular use case that per-requset granularity checkStatus is necessary, but i may miss something here, please discuss on this.

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/snoyberg/http-client/issues/193#issuecomment-221174410

[winterland1989]

Will take a stab on this later ; )

[snoyberg]

I've posted a Google Form on Twitter and Reddit about this topic:

https://docs.google.com/forms/d/172VCI-9ikFSXfa1hRu8WYsLBu2F6270IqAv44rVsLhQ/viewform

[snoyberg]

I think this is the best idea for moving forward I've seen, so I'll be making the relevant changes some time this week: https://www.reddit.com/r/haskell/comments/4oi5us/feedback_requested_how_to_deal_with_exceptions/d4cydjc

[bitemyapp]

@snoyberg problem is, "parseUrlAllStatus" is not going to sound like what a new person wants when they first use the library. They won't know to use something like that. Most don't even understand the idea of parsing a URL to produce a request which is then executed.

We could have something this easy to use (this is from the most popular Python HTTP client library):

>>> r = requests.get('https://api.github.com/user', auth=('user', 'pass'))
>>> r.status_code
200
>>> r.headers['content-type']
'application/json; charset=utf8'
>>> r.encoding
'utf-8'
>>> r.text
u'{"type":"User"...'
>>> r.json()
{u'private_gists': 419, u'total_private_repos': 77, ...}

The request manager parts I am not as concerned about as that isn't the sucking chest wound scaring people off. Is there some reason we cannot have something roughly as simple/intuitive as the above for the simple cases not concerned with reusing connections?

[snoyberg]

Let me clarify exactly what I'm thinking:

• Remove the Default and IsString instance for Request
• Mark parseUrl as deprecated, and have it point to parseRequest
• Add a new parseRequest function with the non-throwing behavior (both in a new breaking version and the current major version)
• Possibly: add a new parseUrlAndThrow function (or something like that) with the current parseUrl behavior
• Update all docs to recommend using parseRequest
• Expose a new defaultRequest value which is like def but without the throwing behavior (both current and next major version)

Does that set aside your concerns?

As far as your example: I believe the Network.HTTP.Simple module does give something basically this simple. The syntax is different and somewhat more verbose since there's not built-in assoc lookup syntax or optional named arguments, but otherwise I feel comfortable with saying that that API is good for end users. Do you have specific improvements there that you'd like to see?

[bitemyapp]

@snoyberg I'd say go ahead for now and I'll chainsaw it after you have something up.

[winterland1989]

How about add a quoter for parseUrl, so that we can write [HTTP.url|GET foo:bar@www.qux.com/a|]. We get both compile time check and easy to read/write code.

[snoyberg]

I'd rather not pull in template Haskell in this library, it negatively impacts some systems without full GHC support.

On Tue, Jun 21, 2016, 5:12 AM winterland notifications@github.com wrote:

How about add a quoter for parseUrl, so that we can write [HTTP.url|GET foo:bar@www.qux.com/a| http://foo:bar@www.qux.com/a%7C]. We get both compile time check and easy to read/write code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/snoyberg/http-client/issues/193#issuecomment-227322389, or mute the thread https://github.com/notifications/unsubscribe/AADBB-lwrkugqKxN4HgZ6Nq9T_nZuzIHks5qN0ingaJpZM4ISgHf .

[winterland1989]

But that can be CPPed anyway, i offer to make a PR if you permit ; )

[snoyberg]

How about adding it to Network.HTTP.Simple? I get less grief about dependencies there

On Tue, Jun 21, 2016, 7:34 AM winterland notifications@github.com wrote:

But that can be CPPed anyway, i offer to make a PR if you permit ; )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/snoyberg/http-client/issues/193#issuecomment-227338283, or mute the thread https://github.com/notifications/unsubscribe/AADBBx0eitC6xMA_faJWNqgu3CaYcwxTks5qN2nbgaJpZM4ISgHf .

[winterland1989]

That's not so obvious IMHO, maybe discuss it later, after we migrate to parseRequest. Overall the migration story is good, Thanks!

[snoyberg]

First stab at this is available in:

https://github.com/snoyberg/http-client/commit/573a28a6e54b46b2adff7cfec571cfbf9bcc42cb

Ideally, I would have deprecated the IsString and Default instances for Request, but that's apparently not possible. Instead, we'll need to have a major version bump which removes these instances, and then at some point in the future we can add them back with the new behavior. I'll hold off on the removing-instances commit until after we nail down what the next 0.4-series release will look like.

[snoyberg]

One more call for feedback before I move ahead with this change.

[bitemyapp]

@snoyberg I don't have any objections, but I didn't ask for code to support a QQ/TH for URL parsing.

I'm not sold on making users parse URIs upfront to begin with.

[winterland1989]

the name of parseUrlThrow is a little implicit IMO cause we have a MonadThrow m on return type, but it's not related with the 'throw' behavior we want to express. I prefer something like parseRequest' with document, what do you think?

[snoyberg]

I want to be as implicit as possible in that name. The goal is that we're trying to get people to cycle off of that function towards parseRequest, and we want it to be obvious in the name (and the DEPRECATION warning) why it's not as good.

On Tue, Jun 28, 2016 at 4:38 AM, winterland notifications@github.com wrote:

the name of parseUrlThrow is a little implicit IMO cause we have a MonadThrow m on return type, but it's not related with the 'throw' behavior we want to express. I prefer something like parseRequest' with document, what do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/snoyberg/http-client/issues/193#issuecomment-228925120, or mute the thread https://github.com/notifications/unsubscribe/AADBB1QUF5Ls0ebe-yqJ9odc8MyRMOYcks5qQHsIgaJpZM4ISgHf .

[winterland1989]

Fine, i cant find other problems now.

[snoyberg]

I've uploaded http-client-0.4.30 to Hackage, which introduces these changes. I'm leaning towards just making a break now and releasing 0.5.0 and, instead of removing the IsString instance, changing its behavior. (I do want to get rid of the Default instance though, I've been moving away from data-default for a while now.)

[snoyberg]

I've created pull request #206 for the http-client 0.5 release. I'm going to close this issue, and ask for feedback on that PR instead from here out.