Add support for Unauthorized and Forbidden response codes, and handle non-JSON errors #55
Conversation
… non-JSON error responses
| @@ -180,6 +207,8 @@ def initialize(token, token_type: :Token, url: API_PREFIX, debug: false) | |||
| conn.use RaiseApiErrorOnNon200 | |||
| conn.use RaiseFileNotFoundOn404 | |||
| conn.use RaiseRateLimitOn429 | |||
| conn.use RaiseForbiddenOn403 | |||
| conn.use RaiseUnauthorizedOn401 | |||
There was a problem hiding this comment.
@technicalpickles I was wondering if it's easier to merge these error handlers into one handler, it'll make it easier to patch for consumers of the gem, and may clean up the stack a bit.
If you're OK with that, I'll make a new PR after this one
There was a problem hiding this comment.
Yeah, that is a good point. A follow up PR would be appreciated 👍🏻
|
I switched to this branch after seeing some non-JSON body errors in our logs and it much improved the exception output. 👍 on this. |
| # TODO May Need to check error.errors too | ||
| message += "\n#{JSON.parse(error)}" | ||
| begin | ||
| # TODO May Need to check error.errors too | ||
| message += "\n#{JSON.parse(error)}" | ||
| rescue JSON::ParserError | ||
| message += "\n#{error}" | ||
| end |
There was a problem hiding this comment.
Now that I am looking at it, it makes me wonder if it is even worth trying to parse the error? It has been awhile, but does JSON.parse end up pretty-printing the error?
If it doesn't, or if it's already reasonably formatted, maybe we just get rid of the parsing, and always just show the error
There was a problem hiding this comment.
I started this review long ago, but forgot to hit submit 😓 This is more of a note and a possible improvement, this looks good to get this behavior fixed.
| @@ -180,6 +207,8 @@ def initialize(token, token_type: :Token, url: API_PREFIX, debug: false) | |||
| conn.use RaiseApiErrorOnNon200 | |||
| conn.use RaiseFileNotFoundOn404 | |||
| conn.use RaiseRateLimitOn429 | |||
| conn.use RaiseForbiddenOn403 | |||
| conn.use RaiseUnauthorizedOn401 | |||
There was a problem hiding this comment.
Yeah, that is a good point. A follow up PR would be appreciated 👍🏻
This adds extra HTTP Status handling, for 401 ->
UnauthorizedError, and 403 ->ForbiddenErrorIt also adds better error parsing, sometimes the API will respond with a non-JSON body, and the
JSON.parseneeds a rescue in that case.