Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling when response is not JSON #248

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

etscrivner
Copy link

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

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
@martey
Copy link
Member

martey commented Nov 17, 2015

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
Copy link
Author

@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
Copy link
Member

martey commented Nov 17, 2015

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.

@martey martey self-assigned this Nov 17, 2015
@martey martey added this to the v1.0.0 milestone Nov 17, 2015
@etscrivner
Copy link
Author

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

@keithhackbarth
Copy link

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

@martey
Copy link
Member

martey commented Feb 22, 2016

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 martey modified the milestones: v2.0.0, v1.0.0 Feb 22, 2016
@martey martey modified the milestones: v3.0.0, v2.0.0 Aug 8, 2016
@martey
Copy link
Member

martey commented Nov 5, 2016

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.

@martey martey removed their assignment Nov 5, 2016
@martey martey modified the milestones: v3.0.0, v4.0.0 Jul 19, 2018
@martey martey modified the milestones: v4.0.0, v5.0.0 Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants