search

goshippo / shippo-ruby-client

Shipping API Ruby library (USPS, FedEx, UPS and more)
https://goshippo.com/docs/
76 stars 72 forks source link

Shippo::Manifest.create obscures invalid transaction error with Shippo::Exceptions::ConnectionError #73

Open oehlschl opened 7 years ago

oehlschl commented 7 years ago

We just encountered this regression after upgrading from version 1.0 to version 3.0 of the Shippo gem/API. It looks like this was really introduced in version 2.0* and we can work around it, but it's still worth noting that the behavior is unexpected and different than Shippo clients in other languages.

Previously, invalid transactions sent to Shippo::Manifest.create were returned in the message body of a Shippo::APIError object. Now, however, a manifest request with invalid transactions results yields an underlying RestClient::NotFound exception, which is wrapped by a Shippo::Exceptions::ConnectionError here: https://github.com/goshippo/shippo-ruby-client/blob/a36a024a302c83fc8d93166ec2c5817d6e40a28f/lib/shippo/api/request.rb#L75 . The original exception message containing the invalid transactions is obscured, and, in particular, the connection error is misleading. Ideally, the actual error message would still appear accessible to the end user.

It looks like at least the PHP and Java clients treat 404s as invalid requests; see https://github.com/goshippo/shippo-php-client/blob/master/lib/Shippo/ApiRequestor.php#L102 and https://github.com/goshippo/shippo-java-client/blob/5ee5e3068742640cc827d23ab709a5b364c8b613/src/main/java/com/shippo/net/APIResource.java#L522.

Can we we also include NotFound with the BadRequest logic here? https://github.com/goshippo/shippo-ruby-client/blob/master/lib/shippo/api/request.rb#L72

*The fallback exception class changed to ConnectionError in 2.0 (https://github.com/goshippo/shippo-ruby-client/blob/v2.0/lib/shippo/api/request.rb#L64), and the error message is no longer passed through as it was in 1.0. (https://github.com/goshippo/shippo-ruby-client/blob/v1.0.4/lib/shippo.rb#L88)

oehlschl commented 7 years ago

It looks like ::RestClient::BadRequest is also rescued twice (without being reraised), on both https://github.com/goshippo/shippo-ruby-client/blob/a36a024a302c83fc8d93166ec2c5817d6e40a28f/lib/shippo/api/request.rb#L62 and https://github.com/goshippo/shippo-ruby-client/blob/a36a024a302c83fc8d93166ec2c5817d6e40a28f/lib/shippo/api/request.rb#L72

oehlschl commented 7 years ago

It actually looks like InvalidInputError doesn't exist, possibly missed because the rescue code path is never hit. https://github.com/goshippo/shippo-ruby-client/search?utf8=%E2%9C%93&q=InvalidInputError&type=

jfriedr commented 4 years ago

@oehlschl I'm going to attempt to sift through all our outstanding issues and pull requests.

Are you still seeing this behavior or can this issue be closed out?