twilio / twilio-ruby

A Ruby gem for communicating with the Twilio API and generating TwiML
MIT License
1.35k stars 462 forks source link

fix: avoid JSON::ParserError for all server errors #582

Closed dan-jensen closed 2 years ago

dan-jensen commented 2 years ago

When there is a Twilio service outage that causes the Twilio API to return a 503 Service Unavailable error, a JSON::ParserError is raised. This fixes that bug by detecting all 5XX server error status codes, and handling them the same way 504 status codes were already being handled.

This is related to the existing bug issue and is basically an expansion of a previous pull request.

Fixes #544

A short description of what this PR does.

Checklist

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

dan-jensen commented 2 years ago

Note: I was tempted to implement the same change for client errors by changing elsif response.status == 400 to use Faraday::Response::RaiseError::ClientErrorStatuses. But that felt like too big a change for this PR.

Also, it makes me nervous that line appears after elsif response.body && !response.body.empty?. I would think we should check for both client errors and server errors before we return the response body, so I'm a bit wary of some special intent there. But if I had to guess, I would say it was an unintentional oversight in the original implementation. I would recommend a follow-up PR to handle client errors.

dan-jensen commented 2 years ago

@eshanholtz would you have time to review this? Pinging you because you merged a similar PR earlier this year so I'm guessing you have the most context awareness 🙂

doryphores commented 2 years ago

Hello. Any movement on this? Feels like a good fix to merge in.