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

Add retry logic for status code - 503 #226

Merged
merged 4 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tap_facebook/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def raise_from(singer_error, fb_error):
error_message = '{}: {} Message: {}'.format(
http_method,
fb_error.http_status(),
fb_error.body().get('error', {}).get('message')
fb_error.body().get('error', {}).get('message') if isinstance(fb_error.body(), dict) else str(fb_error.body())
)
else:
# All other facebook errors are `FacebookError`s and we handle
Expand All @@ -154,7 +154,7 @@ def should_retry_api_error(exception):
elif isinstance(exception, FacebookRequestError):
return (exception.api_transient_error()
or exception.api_error_subcode() == 99
or exception.http_status() == 500
or exception.http_status() in (500, 503)
# This subcode corresponds to a race condition between AdsInsights job creation and polling
or exception.api_error_subcode() == 33
)
Expand Down
28 changes: 28 additions & 0 deletions tests/unittests/test_retry_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,34 @@ def test_retries_on_500(self):
# 5 is the max tries specified in the tap
self.assertEquals(5, mocked_account.get_ad_creatives.call_count )

def test_retries_on_503(self):
"""`AdCreative.sync.do_request()` calls a `facebook_business` method,
`get_ad_creatives()`, to make a request to the API. We mock this
method to raise a `FacebookRequestError` with an `http_status` of
`503`.

We expect the tap to retry this request up to 5 times, which is
the current hard coded `max_tries` value.
"""

# Create the mock and force the function to throw an error
mocked_account = Mock()
mocked_account.get_ad_creatives = Mock()
mocked_account.get_ad_creatives.side_effect = FacebookRequestError(
message='',
request_context={"":Mock()},
http_status=503,
http_headers=Mock(),
body={}
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To mock actual exception, can we assign string value to the body?

Suggested change
mocked_account.get_ad_creatives.side_effect = FacebookRequestError(
message='',
request_context={"":Mock()},
http_status=503,
http_headers=Mock(),
body={}
)
mocked_account.get_ad_creatives.side_effect = FacebookRequestError(
message='',
request_context={"":Mock()},
http_status=503,
http_headers=Mock(),
body="Service Unavailable"
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


# Initialize the object and call `sync()`
ad_creative_object = AdCreative('', mocked_account, '', '')
with self.assertRaises(FacebookRequestError):
ad_creative_object.sync()
# 5 is the max tries specified in the tap
self.assertEquals(5, mocked_account.get_ad_creatives.call_count )

def test_catch_a_type_error(self):
"""`AdCreative.sync.do_request()` calls a `facebook_business` method `get_ad_creatives()`.
We want to mock this to throw a `TypeError("string indices must be integers")` and assert
Expand Down