shippo-ruby-client icon indicating copy to clipboard operation
shippo-ruby-client copied to clipboard

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

Open oehlschl opened this issue 8 years ago • 3 comments

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 avatar Sep 07 '17 07:09 oehlschl

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 avatar Sep 07 '17 08:09 oehlschl

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=

oehlschl avatar Sep 07 '17 08:09 oehlschl

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

jfriedr avatar Jul 07 '20 21:07 jfriedr