-
Notifications
You must be signed in to change notification settings - Fork 113
Expose rate limit headers into instance variables #202
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
Changes from all commits
9a65826
21a2963
9c5d8e7
f0a10be
94854a5
062e325
c72f124
469eb2c
cab4f6e
1bbf972
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import responses | ||
| import unittest | ||
|
|
||
| from tests.support import with_resource, with_fixture, characters | ||
|
|
||
| from twitter_ads.account import Account | ||
| from twitter_ads.campaign import Campaign | ||
| from twitter_ads.client import Client | ||
| from twitter_ads.cursor import Cursor | ||
| from twitter_ads.http import Request | ||
| from twitter_ads.resource import Resource | ||
| from twitter_ads import API_VERSION | ||
|
|
||
|
|
||
| @responses.activate | ||
| def test_rate_limit_cursor_class_access(): | ||
| responses.add(responses.GET, | ||
| with_resource('/' + API_VERSION + '/accounts/2iqph'), | ||
| body=with_fixture('accounts_load'), | ||
| content_type='application/json') | ||
|
|
||
| responses.add(responses.GET, | ||
| with_resource('/' + API_VERSION + '/accounts/2iqph/campaigns'), | ||
| body=with_fixture('campaigns_all'), | ||
| content_type='application/json', | ||
| headers={ | ||
| 'x-account-rate-limit-limit': '10000', | ||
| 'x-account-rate-limit-remaining': '9999', | ||
| 'x-account-rate-limit-reset': '1546300800' | ||
| }) | ||
|
|
||
| client = Client( | ||
| characters(40), | ||
| characters(40), | ||
| characters(40), | ||
| characters(40) | ||
| ) | ||
|
|
||
| account = Account.load(client, '2iqph') | ||
|
|
||
| cursor = Campaign.all(account) | ||
| assert cursor is not None | ||
| assert isinstance(cursor, Cursor) | ||
| assert cursor.rate_limit is None | ||
| assert cursor.account_rate_limit == '10000' | ||
| assert cursor.account_rate_limit_remaining == '9999' | ||
| assert cursor.account_rate_limit_reset == '1546300800' | ||
|
|
||
|
|
||
| @responses.activate | ||
| def test_rate_limit_resource_class_access(): | ||
| responses.add(responses.GET, | ||
| with_resource('/' + API_VERSION + '/accounts/2iqph'), | ||
| body=with_fixture('accounts_load'), | ||
| content_type='application/json') | ||
|
|
||
| responses.add(responses.GET, | ||
| with_resource('/' + API_VERSION + '/accounts/2iqph/campaigns/2wap7'), | ||
| body=with_fixture('campaigns_load'), | ||
| content_type='application/json', | ||
| headers={ | ||
| 'x-account-rate-limit-limit': '10000', | ||
| 'x-account-rate-limit-remaining': '9999', | ||
| 'x-account-rate-limit-reset': '1546300800' | ||
| }) | ||
|
|
||
| client = Client( | ||
| characters(40), | ||
| characters(40), | ||
| characters(40), | ||
| characters(40) | ||
| ) | ||
|
|
||
| account = Account.load(client, '2iqph') | ||
| campaign = Campaign.load(account, '2wap7') | ||
|
|
||
| resource = '/' + API_VERSION + '/accounts/2iqph/campaigns/2wap7' | ||
| params = {} | ||
|
|
||
| response = Request(client, 'get', resource, params=params).perform() | ||
| # from_response() is a staticmethod, so passing campaign instance as dummy. | ||
| # We can later change this test case to not call this manually | ||
| # once we changed existing classes to pass the header argument. | ||
| data = campaign.from_response(response.body['data'], response.headers) | ||
|
|
||
| assert data is not None | ||
| assert isinstance(data, Resource) | ||
| assert data.id == '2wap7' | ||
| assert data.entity_status == 'ACTIVE' | ||
| assert data.rate_limit is None | ||
| assert data.account_rate_limit == '10000' | ||
| assert data.account_rate_limit_remaining == '9999' | ||
| assert data.account_rate_limit_reset == '1546300800' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,8 +13,6 @@ | |
| else: | ||
| import http.client as httplib | ||
|
|
||
| import dateutil.parser | ||
| from datetime import datetime | ||
| from requests_oauthlib import OAuth1Session | ||
| from twitter_ads.utils import get_version | ||
| from twitter_ads.error import Error | ||
|
|
@@ -124,12 +122,9 @@ def __init__(self, code, headers, **kwargs): | |
| self._raw_body = kwargs.get('raw_body', None) | ||
|
|
||
| if headers.get('content-type') == 'application/gzip': | ||
| # hack because Twitter TON API doesn't return headers as it should | ||
| # and instead returns a gzipp'd file rather than a gzipp encoded response | ||
| # Content-Encoding: gzip | ||
| # Content-Type: application/json | ||
| # instead it returns: | ||
| # Content-Type: application/gzip | ||
| # Async analytics data arrives as a gzipped file so decompress it on-the-fly. | ||
| # Note: might need to consider using zlib.decompressobj() instead | ||
| # in case data streams gets large enough (data size doesn't fit into memory at once) | ||
| raw_response_body = zlib.decompress(self._raw_body, 16 + zlib.MAX_WBITS).decode('utf-8') | ||
| else: | ||
| raw_response_body = self._raw_body | ||
|
|
@@ -139,19 +134,6 @@ def __init__(self, code, headers, **kwargs): | |
| except ValueError: | ||
| self._body = raw_response_body | ||
|
|
||
| if 'x-rate-limit-reset' in headers: | ||
| self._rate_limit = int(headers['x-rate-limit-limit']) | ||
| self._rate_limit_remaining = int(headers['x-rate-limit-remaining']) | ||
| self._rate_limit_reset = datetime.fromtimestamp(int(headers['x-rate-limit-reset'])) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with not converting raw header values to a specific format like this. We can just leave it as is so that users can process as they want to. Plus, this doesn't necessarily be a breaking change as most of the users weren't able to access these values anyway unless they do a manual request approach. |
||
| elif 'x-cost-rate-limit-reset' in headers: | ||
| self._rate_limit = int(headers['x-cost-rate-limit-limit']) | ||
| self._rate_limit_remaining = int(headers['x-cost-rate-limit-remaining']) | ||
| self._rate_limit_reset = dateutil.parser.parse(headers['x-cost-rate-limit-reset'].first) | ||
| else: | ||
| self._rate_limit = None | ||
| self._rate_limit_remaining = None | ||
| self._rate_limit_reset = None | ||
|
|
||
| @property | ||
| def code(self): | ||
| return self._code | ||
|
|
@@ -168,18 +150,6 @@ def body(self): | |
| def raw_body(self): | ||
| return self._raw_body | ||
|
|
||
| @property | ||
| def rate_limit(self): | ||
| return self._rate_limit | ||
|
|
||
| @property | ||
| def rate_limit_remaining(self): | ||
| return self._rate_limit_remaining | ||
|
|
||
| @property | ||
| def rate_limit_reset(self): | ||
| return self._rate_limit_reset | ||
|
|
||
| @property | ||
| def error(self): | ||
| return True if (self._code >= 400 and self._code <= 599) else False | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| from twitter_ads.http import Request | ||
| from twitter_ads.cursor import Cursor | ||
| from twitter_ads import API_VERSION | ||
| from twitter_ads.utils import extract_response_headers | ||
|
|
||
|
|
||
| def resource_property(klass, name, **kwargs): | ||
|
|
@@ -42,12 +43,17 @@ def __init__(self, account): | |
| def account(self): | ||
| return self._account | ||
|
|
||
| def from_response(self, response): | ||
| def from_response(self, response, headers=None): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a new argument Currently, caller passes only response body: return self.from_response(response.body['data'])so adding response headers in order to process rate limit headers: return self.from_response(response.body['data'], response.headers)see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just reminder to follow up about debugging level headers and exposing those as well (X- headers)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, we can generalize the extract_rate_limit() method in the future to handle more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to John's point. I believe there are plans to include rate limit headers to the async stats create endpoint. Given the current setup, this should be fairly trivial to add. Good stuff!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I just changed the func name to |
||
| """ | ||
| Populates a given objects attributes from a parsed JSON API response. | ||
| This helper handles all necessary type coercions as it assigns | ||
| attribute values. | ||
| """ | ||
| if headers is not None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens for the case where headers is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tushdante Thanks! That's a good point... I removed explicit "if" conditions from the |
||
| limits = extract_response_headers(headers) | ||
| for k in limits: | ||
| setattr(self, k, limits[k]) | ||
|
|
||
| for name in self.PROPERTIES: | ||
| attr = '_{0}'.format(name) | ||
| transform = self.PROPERTIES[name].get('transform', None) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is not really correct I'm correcting this. re: PAEX-2015