facebook / facebook-python-business-sdk

Python SDK for Meta Marketing APIs
https://developers.facebook.com/docs/business-sdk
Other
1.27k stars 629 forks source link

Call to Ads API doesn't throw error on failure. #675

Open JBarti opened 1 week ago

JBarti commented 1 week ago

While trying to gather all adcreatives from a customer ad account I managed to trigger an unexpected error in the Ads API. The library handles this response incorrectly by not raising an error even though the response status is 400.

Details

  1. Try to retrieve all adcreatives from a single ad account, together with all the existing fields, with the response limit set to 100 (the account has more than a thousand adcreatives).
  2. After around 10 pages are retrieved facebook returns the body in HTML format with a status code of 400:

    <!DOCTYPE html>
    <html lang="en" id="facebook">
    <head>
    <title>Facebook | Error</title>
    <meta charset="utf-8">
    <meta http-equiv="cache-control" content="no-cache">
    <meta http-equiv="cache-control" content="no-store">
    <meta http-equiv="cache-control" content="max-age=0">
    <meta http-equiv="expires" content="-1">
    <meta http-equiv="pragma" content="no-cache">
    <meta name="robots" content="noindex,nofollow">
    <style>
      html, body {
        color: #141823;
        background-color: #e9eaed;
        font-family: Helvetica, Lucida Grande, Arial,
                     Tahoma, Verdana, sans-serif;
        margin: 0;
        padding: 0;
        text-align: center;
      }
    
      #header {
        height: 30px;
        padding-bottom: 10px;
        padding-top: 10px;
        text-align: center;
      }
    
      #icon {
        width: 30px;
      }
    
      h1 {
        font-size: 18px;
      }
    
      p {
        font-size: 13px;
      }
    
      #footer {
        border-top: 1px solid #ddd;
        color: #9197a3;
        font-size: 12px;
        padding: 5px 8px 6px 0;
      }
    </style>
    </head>
    <body>
    <div id="header">
      <a href="//www.facebook.com/">
        <img id="icon" src="//static.facebook.com/images/logos/facebook_2x.png" />
      </a>
    </div>
    <div id="core">
      <h1 id="sorry">Sorry, something went wrong.</h1>
      <p id="promise">
        We're working on it and we'll get it fixed as soon as we can.
      </p>
      <p id="back-link">
        <a id="back" href="//www.facebook.com/">Go Back</a>
      </p>
    … [message truncated due to size]
  3. The is_success method in the facebook_business.api module fails to identify this response as a failure and returns True.

Code

The code provided encounters this issue when accessing the /act_<account_id>/adcreatives endpoint. Here's a breakdown of what the code does:

  1. API Call: It makes a request to a specified path of the Facebook Ads API.
  2. Response Parsing: It parses the API response using a Pydantic model.
  3. Pagination Handling: If the response is paginated, it repeats the entire process using the paging.next URL. This approach ensures that the full data for a single edge is retrieved.
def _call_fb_api_with_retry(api: FacebookAdsApi, method: str, path: str, params: dict, max_retries: int = 3) -> FacebookResponse:
    """Call the Facebook API with retries."""
    for i in range(max_retries):
        try:
            return api.call(method=method, path=path, params=params)
        except Exception as e:
            logger.error(e)
            if i == max_retries - 1:
                raise e
            logger.info(f"Retrying {method} {path} with params {params} (attempt {i + 1}/{max_retries})")

    raise Exception("This should never happen")

def _get_fb_data(
    api: FacebookAdsApi,
    validation_model: Type[BaseModel],
    endpoint: str,
    params: dict,
    limit: int = 1000
) -> Generator[list[BaseModel], Any, Any]:
    """Get data from the Facebook API and yield the data in chunks."""
    base_url = "https://graph.facebook.com/v20.0"
    path = f"{base_url}{endpoint}"

    while path:
        resp = _call_fb_api_with_retry(
            api=api,
            method="GET",
            path=path,
            params={
                **params,
                "limit": limit,
                "time_increment": 1,
            },
        )

        raw_data = resp.json()

        if not raw_data:
            logger.info("No data for endpoint ", endpoint)
            return

        try:
            fb_body_model = validation_model(**raw_data)
        except Exception as e:
            logger.error(e)
            logging.error("Status Code: %s" % resp.status())
            logging.error("Error: %s" % resp.error())
            logging.error("Body: %s" % resp.body())
            raise e

        if isinstance(fb_body_model, EdgeRequestResponseBody):
            path = fb_body_model.paging.next if fb_body_model.paging else None
            logger.info("Next page: %s" % path)
            yield fb_body_model.data
        else:
            path = None
            yield [fb_body_model]
PacificGilly commented 1 week ago

We've experienced the exact same issue on occasion as the status code isn't being acted on, even 5xx codes.

Think the simplest and cleanest solution might be to implement a proper Retry strategy directly at the low-level API request call being made: https://github.com/facebook/facebook-python-business-sdk/blob/0a8f34741eee83fbce4426b48312f7dc52a1265e/facebook_business/api.py#L301-L317

Potential Fix

from urllib3 import Retry
from requests import Session()
from requests.adapters import HTTPAdapter

DEFAULT_RETRIES = 5  # Allows configuration.
DEFAULT_BACKOFF_FACTOR = 0.5  # Time doubles between API calls.

session = Session()
retry = Retry(
    total=DEFAULT_RETRIES,
    # Matches `settings.RETRY_HTTP_CODES` in lyst/checkout.
    status_forcelist=[
        HTTP_408_REQUEST_TIMEOUT,
        HTTP_429_TOO_MANY_REQUESTS,
        HTTP_500_INTERNAL_SERVER_ERROR,
        HTTP_503_SERVICE_UNAVAILABLE,
        HTTP_504_GATEWAY_TIMEOUT,
    ],
    backoff_factor=DEFAULT_BACKOFF_FACTOR,
    respect_retry_after_header=True,
)
adapter = HTTPAdapter(max_retries=retry)
# Multiple `mount`s shouldn't be an issue since it's just updating a dict key.
# Preferably declare them by descending prefix length as per below.
session.mount("https://", adapter)
session.mount("http://", adapter)

Happy to raise a PR if useful