Improve error handling when response is not JSON #248
Conversation
The GraphAPI class currently handles HTTP response errors by treating the response body as JSON and attempting to decode it. This is usually fine, but there are some cases where Facebook returns responses that do not contain valid JSON. In these cases a rather uninformative ValueError is raised indicating that JSON decoding failed. Since we have the HTTP response at the point where this error is raised, the status code and body can be extracted to produce a more helpful error message. This commit makes the following changes: - Factor out duplicate error handling code into new method GraphAPI.create_exception_for_error. - Add logic to catch JSON decoding error and produce a more informative message containing the contents of the HTTP response. - Add exception class GraphAPIResponseError that is raised when JSON decoding fails. - Add exception base class FacebookError and have both GraphAPIError and GraphAPIResponseError inherit from it.
- Use % formatting rather than .format method - User assertTrue and an isinstance check rather than assertIsInstance
The code for catching and handling HTTPError was originally written against urllib2. When the change was made to requests this code was not properly modified. Unfortunately, this had the effect of completely suppressing exceptions on error responses rather than handling them. Fixing this revealed 2 unit-tests that should actually have been failing the entire time. These tests have now been skipped pending a rewrite.
Use Response.json method to decode response data, and display content in GraphAPIResponseError using repr since content can be stream of bytes.
Update stub to have json() method
It looks like you are skipping the Can you provide a Gist or some code to help me reproduce this problem? |
|
@martey Of course! You can find a complete gist of the errors returned here: https://gist.github.com/etscrivner/c3d825537a6ba16b893b As some additional background - I ran these tests using the same app token and secret used for Travis CI. The tests were previously passing, but when I fixed the response error handling the GraphAPI errors returned from the tests were actually raised, causing them to fail. The error is the same for both: The issue seems to be with the I may very well be missing something, and appreciate your quick reply. Let me know how I can help. |
|
Thanks to the additional information you provided, I think I have figured out what is happening.
I should be able to refactor the test so it works with the new error handling (and also add better comments so it is clear what the method actually does). I also need to decide whether we still need to support Python 2.6 (since dropping it should remove the need to install unittest2). Either way, your pull request is accepted. I am adding it to the 1.0.0 milestone. Thanks for contributing. |
|
@martey Ahh, that makes perfect sense. Thank you! |
|
@martey - Any update to this being merged? Thanks!!! |
|
In order to release 1.0.0 as soon as possible, I am moving this to the 2.0.0 milestone. This isn't a reflection of the quality of your pull request, but of the lack of free time I currently have to devote to this project. |
|
I'm unassigning myself from this pull request because I don't think I will be able to get to it soon. I think the core idea of this pull request is still good, but some commits (those fixing Python 2.6 issues) should be dropped, and most of the others have multiple conflicts that need to be resolved. |
The GraphAPI class currently handles HTTP response errors by treating
the response body as JSON and attempting to decode it. This is usually
fine, but there are some cases where Facebook returns responses that do
not contain valid JSON. In these cases a rather uninformative ValueError
is raised indicating that JSON decoding failed. Since we have the HTTP
response at the point where this error is raised, the status code and
body can be extracted to produce a more helpful error message.
While fixing this an issue with HTTPError handling was found. The original
library was written using urllib2 rather than requests. When the change to
requests was made this error handling was not corrected. This had the
unfortunate side-effect of completely suppressing errors so that
GraphAPIError is never raised. This logic has been fixed, but fixing
it had the side-effect of revealing 2 unit-tests that should have been
breaking the entire time. These tests have been skipped pending a
rewrite.
This commit makes the following changes:
GraphAPI.create_exception_for_error.
message containing the contents of the HTTP response.
decoding fails.
GraphAPIResponseError inherit from it.