search

gaffneyc / usps

USPS Webtools API for Ruby
MIT License
63 stars 54 forks source link

Empty response body raises an error #34

Closed ozzyaaron closed 3 years ago

ozzyaaron commented 3 years ago

Hi there, I hope I did everything requested. I tried to follow the directions in the Readme as best I could.

When testing on my development machine I noticed that if you disable your network connection then the first request will timeout. Subsequent requests however are not attempted and thus are not a timeout but do return a blank body.

I'm not sure whether the fault lies with Typhoes or libcurl itself as they're difficult to tease apart.

Either way, I thought that if we get a blank body I this should be considered an exceptional case and hence the PR.

@gaffneyc as commented in the code it is up to you whether you think this should be a different type of error. I figured for consumers of this gem, like us, this type of error might either fall into the TimeoutError case or an AddressNotFound case. I picked TimeoutError as I thought it was more correct. I could also see you considering this a different type of error altogether 🤷

bensheldon commented 3 years ago

@ozzyaaron thank you for opening up an issue. I'm curious here, what the value of response.return_code is in this situation.

I'm a little reluctant to overload TimeoutError because I'm not sure if there are valid reasons for a response body to be empty and this might be a source of confusion. I think EmptyBodyError would be more descriptive, but I wonder if we can pry out of Typhoeus the underlying error itself.

ozzyaaron commented 3 years ago

Hi @bensheldon thanks for getting back!

I just checked and the response.return_code.

For the first request I get :operation_timedout then most times I get :couldnt_connect but once I got :couldnt_resolve_host.

I will say that from what I can see response.response_code was always 0 which I think could be considered exceptional.

I totally get your reluctance to overload the error, I'm happy to update it to anything you wish!

bensheldon commented 3 years ago

Ideally I think I would like the error hierarchy to be:

  # Error Hierarchy
  #
  # StandardError
  #   USPS::Error
  #     USPS::ConnectionError <= new error class
  #       USPS::TimeoutError
  #       USPS::AuthorizationError

And then in this case, raise a USPS::ConnectionError if response.response_code == 0.

ozzyaaron commented 3 years ago

@bensheldon I have updated the PR to reflect the changes you requested. I hope I got it right! :)

bensheldon commented 3 years ago

@ozzyaaron perfect! thank you! 🙌