chargebee-ruby icon indicating copy to clipboard operation
chargebee-ruby copied to clipboard

only rescue RestClient exceptions

Open igneus opened this issue 9 years ago • 4 comments

Currently all errors coming from the depths of rest-client are rescued and translated to IOError. I suggest to change it:

  1. It's a bad practice to rescue Exception. If you want to rescue all exceptions, rescue StandardError. Here's why.
  2. As in the scenario described in #15, an error may occasionally happen somewhere deep in rest-client, that has nothing to do with network communication, but results solely from gems incompatibility or other client-side software misfortune. It is really misleading to mask such errors as IOError.

I believe the right thing to do is to only handle exceptions raised by rest-client (which fortunately have a common ancestor RestClient::Exception). rest-client already does an awesome job handling various exceptions from the layers below and translating them to instances of it's own exception types, so you can be assured that exceptions of other types aren't "mere IO errors" and signal deeper problems.

igneus avatar Jul 19 '16 20:07 igneus

One line code change to improve error handling. Looks good to me. Ship it!

evolve2k avatar Jun 16 '20 04:06 evolve2k

@cb-navaneedhan - can this get merged?

jongbeau avatar Jan 20 '21 00:01 jongbeau

@cb-nithins can we merge this one?

sobrinho avatar Jan 08 '24 12:01 sobrinho

Ping

sobrinho avatar May 28 '25 17:05 sobrinho

@cb-alish would you review this one as well?

sobrinho avatar Jun 06 '25 13:06 sobrinho

Hi @sobrinho, this is no longer relevant since we've moved away from rest-client. Are you seeing a similar issue with the new HTTP client? Let me review and add support if it's still applicable. Thanks.

cb-alish avatar Jun 06 '25 14:06 cb-alish

@cb-alish lib/chargebee/rest.rb still contains several occurrences of the rescue Exception antipattern, which is discussed in my first post above.

Personally I'm not involved in any ChargeBee integration anymore, so I don't really care, but if you care about your users/integrators/customers, it's definitely something worth fixing.

igneus avatar Jun 06 '25 19:06 igneus

... but it's true that this PR as such is now outdated and makes no more sense. Closing.

igneus avatar Jun 26 '25 14:06 igneus