Distinguish errors by Exception class #47
Conversation
|
Hi, thanks for the PR! For an "Unavailable" situation it's generally good to consider any 5xx code. I usually use number comparison for this in other languages, but given that Ruby apparently gives the status as a string, maybe The change in #46 is probably still relevant I think; and I can rebase it separately from this PR since your patchset doesn't include it. |
|
@danopia Thank you for your review. From your suggestion, I went and checked the library documentation: So rather than check the error code directly, I compare against the base class for 5XX errors. I then changed the if sequences to be a switch statement. I did this in f0ba154 If you prefer to check against strings, I can change to that. I added some tests to validate the new behaviour in d42cc3a And I merged in the changes from #46 (well, I merged with Please let me know what you think. Thank you! |
|
Excellent, the subclass handling looks like the correct way of handling these HTTP statuses! I'll do a CI run of this in a branch soon |
There was a problem hiding this comment.
Test run (merged w/ master) visible here: https://github.com/relrod/bing_translator-gem/runs/3185131454?check_suite_focus=true
lib/bing_translator.rb
Outdated
| when Net::HTTPServerError | ||
| raise UnavailableException.new("#{response.code}: Credentials server unavailable") | ||
| else | ||
| raise Exception.new("Unsuccessful Access Token call: Code: #{response.code} (Invalid credentials?)") |
There was a problem hiding this comment.
I believe the specialized error class got lost here when merging master.
The new spec seems to expect AuthenticationException
There was a problem hiding this comment.
Sorry for the sloppy merge. I've fixed this in 1f71d42 and added tests to ensure the correct exception is being raised. Thanks!
|
I'll go ahead and merge this, but I don't have the access to cut a release so that will still be pending. I will update here with a version number when it's done. |
|
Thanks for your review and taking care of the merge! |
|
🎉 This PR is included in version 6.2.0 🎉 The release is available on: Your friendly neighborhood human 📦🚀 |
|
Upgrading now! Thank you! |
Recently Microsoft had wide outages across their various products. The translate API was unable to obtain the right credentials and failed with error
Invalid credentials. The real issue was a503 service unavailable.This PR does 2 things:
The first point is done in a backwards compatible way. Existing code that rescues on
BingTranslator::Exceptionwill still run as before. With the important exception of catching 503 errors, which have a new unique string associated with them.I think the second point is redundant with #46 and the solution in the other PR is more generally useful.
https://status.azure.com/status/history/
Between approximately 07:00 UTC and 13:55 UTC on 21 Jul 2021, customers using Cognitive Services may have experienced difficulties connecting to resources. Impacted customers may have experienced timeouts or 401 authorization errors.I personally saw 503 errors for the few cases I debugged but it could be that during the service downtime interval, various error codes were returned. Maybe catching only 503 is too specific?