Diversification of the exceptions classes

[brouberol]

Every possible error encountered when dealing with the Facebook API (HTTP error, token error, permission error, etc) is raised as a GraphAPIError. While it makes it easy to catch "all" errors in a single blow, it's also pretty hard to target a specific one.

I'll give you an example. I'm currently writing a celery task that would publish an event given an access_token and the event information. I would like to be able to retry to execute the task in the event of a network failure, but not in a token invalidity error (not susceptible to just "go away").

try:
    graph = facebook.GraphAPI(oauth_access_token)
    response = graph.put_object('me', 'event', data=data)
except facebook.GraphAPIError as exc:
    # retry (in 1 minute) if the API call failed
    raise self.retry(exc=exc, countdown=60)

However, with the single exception class, it's not possible to discriminate network related and permission/token related errors. In the case of an invalid token, I would then replay the same task several times, each time facing the exact same error.

I thus propose the following exception hierarchy, that would provide a finer grained control over the exception catching flow.

GraphAPIError
  +------ TokenError
  +------ PermissionError 

It would not break existing codebases, because all new exception classes inherit from GraphAPIError.

Note: my feeling is that a requests.HTTPError should be raised in case of a network error, and not wrapped into a GraphAPIError. However, would you do so, the current codebases might break.

What do you think?

[martey]

I think more granular errors would be a fine feature to add if you wanted to work on this.

Note: my feeling is that a requests.HTTPError should be raised in case of a network error, and not wrapped into a GraphAPIError. However, would you do so, the current codebases might break.

This should already be happening. Can you point to areas in the code or specific situations where it might not?

I think the reverse is possibly more problematic - where requesting invalid or too much data from the API results in a network timeout.

[brouberol]

Network errors - it seems you wrap them into GraphAPIError (see https://github.com/pythonforfacebook/facebook-sdk/blob/master/facebook/__init__.py#L202)

try:
    response = requests.request(method or "GET",
                                "https://graph.facebook.com/" + path,
                                timeout=self.timeout,
                                params=args,
                                data=post_args,
                                files=files)
except requests.HTTPError as e:
    response = json.loads(e.read())
    raise GraphAPIError(response)

I suggest raising a TokenError here: https://github.com/pythonforfacebook/facebook-sdk/blob/master/facebook/__init__.py#L228.

I guess the only way permission errors (eg: error received when the code tries to perform an action that has not been authorized by the account owner) would be introspect the error message and "guess" it it's permission related. Not very pretty... What do you think?

[lucasrangit]

Could you test the URL and schedule a retry if the request does not timeout?

try:
    graph = facebook.GraphAPI(oauth_access_token)
    response = graph.put_object('me', 'event', data=data)
except facebook.GraphAPIError as exc:
    try:
        request = requests.get("https://graph.facebook.com/", timeout=graph.timeout)
    except requests.exceptions.RequestException as re:
        # retry (in 1 minute) if the API call failed due to network issues
        raise self.retry(exc=exc, countdown=60)
    # server is reachable so this must be an API usage issue
    return False

The request.status_code will be 400 (Bad Request).

Instead of nesting exception handlers, I would move the reachable test to the Task.__init__() to ensure Facebook is reachable when each test process starts. Then the network retries would not count against number of retries in the test reports.