Improve error handling when response is not JSON

[etscrivner]

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:

• Fix completely broken error response handling.
• 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.
• Skip unit-tests that should have been failing the entire time

[martey]

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 GraphAPIException 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.

It looks like you are skipping the test_valid_versions and test_fql tests. Your comment states that they have "user access token issues", but both tests don't use user access tokens.

Can you provide a Gist or some code to help me reproduce this problem?

[etscrivner]

@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:

GraphAPIError: An active access token must be used to query information about the current user.

The issue seems to be with the GraphAPI.get_version() method. This makes sense because both tests rely on querying the /me route, and it does not appear that apps have a /me. I was able to get the tests to pass by supplying a user access token from my own Facebook account to GraphAPI.

I may very well be missing something, and appreciate your quick reply. Let me know how I can help.

[martey]

Thanks to the additional information you provided, I think I have figured out what is happening.

get_version does call /me, but it never checks the result. The method looks at the "facebook-api-version" header to figure out the API version; this works even if an error is returned.

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.

[etscrivner]

@martey Ahh, that makes perfect sense. Thank you!

[keithhackbarth]

@martey - Any update to this being merged? Thanks!!!

[martey]

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.

[martey]

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.