New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make adverse status codes not throw an exception #193

Closed
bitemyapp opened this Issue Apr 29, 2016 · 25 comments

Comments

Projects
None yet
4 participants
@bitemyapp

bitemyapp commented Apr 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Yuras

Yuras Apr 29, 2016

Collaborator

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.

Collaborator

Yuras commented Apr 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bitemyapp

bitemyapp Apr 29, 2016

@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.

bitemyapp commented Apr 29, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg May 1, 2016

Owner

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
#193 (comment)

Owner

snoyberg commented May 1, 2016

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
#193 (comment)

@bitemyapp

This comment has been minimized.

Show comment
Hide comment
@bitemyapp

bitemyapp May 1, 2016

@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.

bitemyapp commented May 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 May 24, 2016

Contributor

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.

Contributor

winterland1989 commented May 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg May 24, 2016

Owner

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 #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
#193 (comment)

Owner

snoyberg commented May 24, 2016

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 #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
#193 (comment)

@winterland1989

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 May 24, 2016

Contributor

Will take a stab on this later ; )

Contributor

winterland1989 commented May 24, 2016

Will take a stab on this later ; )

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 17, 2016

Owner

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

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

Owner

snoyberg commented Jun 17, 2016

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

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

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 20, 2016

Owner

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

Owner

snoyberg commented Jun 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bitemyapp

bitemyapp Jun 20, 2016

@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?

bitemyapp commented Jun 20, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 20, 2016

Owner

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?

Owner

snoyberg commented Jun 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bitemyapp

bitemyapp Jun 20, 2016

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

bitemyapp commented Jun 20, 2016

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

@winterland1989

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 Jun 21, 2016

Contributor

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.

Contributor

winterland1989 commented Jun 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 21, 2016

Owner

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
#193 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADBB-lwrkugqKxN4HgZ6Nq9T_nZuzIHks5qN0ingaJpZM4ISgHf
.

Owner

snoyberg commented Jun 21, 2016

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
#193 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADBB-lwrkugqKxN4HgZ6Nq9T_nZuzIHks5qN0ingaJpZM4ISgHf
.

@winterland1989

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 Jun 21, 2016

Contributor

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

Contributor

winterland1989 commented Jun 21, 2016

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

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 21, 2016

Owner

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
#193 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADBBx0eitC6xMA_faJWNqgu3CaYcwxTks5qN2nbgaJpZM4ISgHf
.

Owner

snoyberg commented Jun 21, 2016

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
#193 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADBBx0eitC6xMA_faJWNqgu3CaYcwxTks5qN2nbgaJpZM4ISgHf
.

@winterland1989

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 Jun 21, 2016

Contributor

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

Contributor

winterland1989 commented Jun 21, 2016

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

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 21, 2016

Owner

First stab at this is available in:

573a28a

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.

Owner

snoyberg commented Jun 21, 2016

First stab at this is available in:

573a28a

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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 27, 2016

Owner

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

Owner

snoyberg commented Jun 27, 2016

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

@bitemyapp

This comment has been minimized.

Show comment
Hide comment
@bitemyapp

bitemyapp Jun 27, 2016

@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.

bitemyapp commented Jun 27, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 Jun 28, 2016

Contributor

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?

Contributor

winterland1989 commented Jun 28, 2016

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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 28, 2016

Owner

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
#193 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADBB1QUF5Ls0ebe-yqJ9odc8MyRMOYcks5qQHsIgaJpZM4ISgHf
.

Owner

snoyberg commented Jun 28, 2016

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
#193 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADBB1QUF5Ls0ebe-yqJ9odc8MyRMOYcks5qQHsIgaJpZM4ISgHf
.

@winterland1989

This comment has been minimized.

Show comment
Hide comment
@winterland1989

winterland1989 Jun 28, 2016

Contributor

Fine, i cant find other problems now.

Contributor

winterland1989 commented Jun 28, 2016

Fine, i cant find other problems now.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 30, 2016

Owner

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.)

Owner

snoyberg commented Jun 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jun 30, 2016

Owner

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.

Owner

snoyberg commented Jun 30, 2016

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.

@snoyberg snoyberg closed this Jun 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment