From b048f3307401d78315f4599182a6ddd6b82e5c66 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Tue, 28 Jan 2020 21:45:09 -0800 Subject: [PATCH 1/3] Using a default HTTP timeout in db module --- firebase_admin/_utils.py | 7 +++++++ firebase_admin/db.py | 7 +++---- tests/test_db.py | 31 +++++++++++++++++++++++++------ 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/firebase_admin/_utils.py b/firebase_admin/_utils.py index 2c4cec86..822a9aff 100644 --- a/firebase_admin/_utils.py +++ b/firebase_admin/_utils.py @@ -59,6 +59,9 @@ } +DEFAULT_HTTP_TIMEOUT_SECONDS = 120 + + def _get_initialized_app(app): """Returns a reference to an initialized App instance.""" if app is None: @@ -108,6 +111,10 @@ def handle_platform_error_from_requests(error, handle_func=None): return exc if exc else _handle_func_requests(error, message, error_dict) +def get_http_timeout(app): + return app.options.get('httpTimeout', DEFAULT_HTTP_TIMEOUT_SECONDS) + + def _handle_func_requests(error, message, error_dict): """Constructs a ``FirebaseError`` from the given GCP error. diff --git a/firebase_admin/db.py b/firebase_admin/db.py index 9092a955..b3832390 100644 --- a/firebase_admin/db.py +++ b/firebase_admin/db.py @@ -775,7 +775,7 @@ def __init__(self, app): self._auth_override = json.dumps(auth_override, separators=(',', ':')) else: self._auth_override = None - self._timeout = app.options.get('httpTimeout') + self._timeout = _utils.get_http_timeout(app) self._clients = {} emulator_host = os.environ.get(_EMULATOR_HOST_ENV_VAR) @@ -935,10 +935,9 @@ def request(self, method, url, **kwargs): query = extra_params + '&' + query else: query = extra_params - kwargs['params'] = query - if self.timeout: - kwargs['timeout'] = self.timeout + kwargs['params'] = query + kwargs['timeout'] = self.timeout try: return super(_Client, self).request(method, url, **kwargs) except requests.exceptions.RequestException as error: diff --git a/tests/test_db.py b/tests/test_db.py index b20f99cb..c0290618 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -24,6 +24,7 @@ from firebase_admin import db from firebase_admin import exceptions from firebase_admin import _sseclient +from firebase_admin import _utils from tests import testutils @@ -736,10 +737,11 @@ def test_valid_db_url(self, url): ref._client.session.mount(url, adapter) assert ref._client.base_url == 'https://test.firebaseio.com' assert 'auth_variable_override' not in ref._client.params - assert ref._client.timeout is None + assert ref._client.timeout == _utils.DEFAULT_HTTP_TIMEOUT_SECONDS assert ref.get() == {} assert len(recorder) == 1 - assert recorder[0]._extra_kwargs.get('timeout') is None + assert recorder[0]._extra_kwargs.get('timeout') == pytest.approx( + _utils.DEFAULT_HTTP_TIMEOUT_SECONDS, 0.001) @pytest.mark.parametrize('url', [ None, '', 'foo', 'http://test.firebaseio.com', 'https://google.com', @@ -761,7 +763,7 @@ def test_multi_db_support(self): ref = db.reference() assert ref._client.base_url == default_url assert 'auth_variable_override' not in ref._client.params - assert ref._client.timeout is None + assert ref._client.timeout == _utils.DEFAULT_HTTP_TIMEOUT_SECONDS assert ref._client is db.reference()._client assert ref._client is db.reference(url=default_url)._client @@ -769,7 +771,7 @@ def test_multi_db_support(self): other_ref = db.reference(url=other_url) assert other_ref._client.base_url == other_url assert 'auth_variable_override' not in ref._client.params - assert other_ref._client.timeout is None + assert other_ref._client.timeout == _utils.DEFAULT_HTTP_TIMEOUT_SECONDS assert other_ref._client is db.reference(url=other_url)._client assert other_ref._client is db.reference(url=other_url + '/')._client @@ -782,7 +784,7 @@ def test_valid_auth_override(self, override): default_ref = db.reference() other_ref = db.reference(url='https://other.firebaseio.com') for ref in [default_ref, other_ref]: - assert ref._client.timeout is None + assert ref._client.timeout == _utils.DEFAULT_HTTP_TIMEOUT_SECONDS if override == {}: assert 'auth_variable_override' not in ref._client.params else: @@ -804,7 +806,7 @@ def test_invalid_auth_override(self, override): with pytest.raises(ValueError): db.reference(app=other_app, url='https://other.firebaseio.com') - def test_http_timeout(self): + def test_custom_http_timeout(self): test_url = 'https://test.firebaseio.com' firebase_admin.initialize_app(testutils.MockCredential(), { 'databaseURL' : test_url, @@ -821,6 +823,23 @@ def test_http_timeout(self): assert len(recorder) == 1 assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(60, 0.001) + def test_no_http_timeout(self): + test_url = 'https://test.firebaseio.com' + firebase_admin.initialize_app(testutils.MockCredential(), { + 'databaseURL' : test_url, + 'httpTimeout': None + }) + default_ref = db.reference() + other_ref = db.reference(url='https://other.firebaseio.com') + for ref in [default_ref, other_ref]: + recorder = [] + adapter = MockAdapter('{}', 200, recorder) + ref._client.session.mount(ref._client.base_url, adapter) + assert ref._client.timeout is None + assert ref.get() == {} + assert len(recorder) == 1 + assert recorder[0]._extra_kwargs['timeout'] is None + def test_app_delete(self): app = firebase_admin.initialize_app( testutils.MockCredential(), {'databaseURL' : 'https://test.firebaseio.com'}) From e6d7ec80c6a75171b5b151c2b3d4371f7b17069c Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Tue, 28 Jan 2020 22:34:03 -0800 Subject: [PATCH 2/3] Using default timeout in FCM and project_mgt --- firebase_admin/messaging.py | 2 +- firebase_admin/project_management.py | 2 +- tests/test_messaging.py | 72 +++++++++++++++++----------- tests/test_project_management.py | 3 ++ 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/firebase_admin/messaging.py b/firebase_admin/messaging.py index 9262751a..88c82f24 100644 --- a/firebase_admin/messaging.py +++ b/firebase_admin/messaging.py @@ -331,7 +331,7 @@ def __init__(self, app): 'X-FIREBASE-CLIENT': 'fire-admin-python/{0}'.format(firebase_admin.__version__), } self._client = _http_client.JsonHttpClient(credential=app.credential.get_credential()) - self._timeout = app.options.get('httpTimeout') + self._timeout = _utils.get_http_timeout(app) self._transport = _auth.authorized_http(app.credential.get_credential()) @classmethod diff --git a/firebase_admin/project_management.py b/firebase_admin/project_management.py index 91aa1eeb..9addce1e 100644 --- a/firebase_admin/project_management.py +++ b/firebase_admin/project_management.py @@ -482,7 +482,7 @@ def __init__(self, app): credential=app.credential.get_credential(), base_url=_ProjectManagementService.BASE_URL, headers={'X-Client-Version': version_header}) - self._timeout = app.options.get('httpTimeout') + self._timeout = _utils.get_http_timeout(app) def get_android_app_metadata(self, app_id): return self._get_app_metadata( diff --git a/tests/test_messaging.py b/tests/test_messaging.py index 33c99445..36ca03e4 100644 --- a/tests/test_messaging.py +++ b/tests/test_messaging.py @@ -23,6 +23,7 @@ import firebase_admin from firebase_admin import exceptions from firebase_admin import messaging +from firebase_admin import _utils from tests import testutils @@ -1537,42 +1538,50 @@ def test_aps_alert_custom_data_override(self): } check_encoding(msg, expected) -class TestTimeout: - @classmethod - def setup_class(cls): - cred = testutils.MockCredential() - firebase_admin.initialize_app(cred, {'httpTimeout': 4, 'projectId': 'explicit-project-id'}) +class TestTimeout: - @classmethod - def teardown_class(cls): + def teardown(self): testutils.cleanup_apps() - def setup(self): - app = firebase_admin.get_app() - self.fcm_service = messaging._get_messaging_service(app) - self.recorder = [] - - def test_send(self): - self.fcm_service._client.session.mount( - 'https://fcm.googleapis.com', - testutils.MockAdapter(json.dumps({'name': 'message-id'}), 200, self.recorder)) + @pytest.mark.parametrize('timeout', [4, None]) + def test_send(self, timeout): + app = self._app_with_timeout(timeout) + recorder = self._instrument_service( + app, 'https://fcm.googleapis.com', {'name': 'message-id'}) msg = messaging.Message(topic='foo') messaging.send(msg) - assert len(self.recorder) == 1 - assert self.recorder[0]._extra_kwargs['timeout'] == pytest.approx(4, 0.001) + assert len(recorder) == 1 + if timeout is None: + assert recorder[0]._extra_kwargs['timeout'] is None + else: + assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(timeout, 0.001) - def test_topic_management_timeout(self): - self.fcm_service._client.session.mount( - 'https://iid.googleapis.com', - testutils.MockAdapter( - json.dumps({'results': [{}, {'error': 'error_reason'}]}), - 200, - self.recorder) - ) + @pytest.mark.parametrize('timeout', [4, None]) + def test_topic_management(self, timeout): + app = self._app_with_timeout(timeout) + recorder = self._instrument_service( + app, 'https://iid.googleapis.com',{'results': [{}, {'error': 'error_reason'}]}) messaging.subscribe_to_topic(['1'], 'a') - assert len(self.recorder) == 1 - assert self.recorder[0]._extra_kwargs['timeout'] == pytest.approx(4, 0.001) + assert len(recorder) == 1 + if timeout is None: + assert recorder[0]._extra_kwargs['timeout'] is None + else: + assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(timeout, 0.001) + + def _app_with_timeout(self, timeout): + cred = testutils.MockCredential() + return firebase_admin.initialize_app(cred, { + 'httpTimeout': timeout, + 'projectId': 'explicit-project-id' + }) + + def _instrument_service(self, app, url, response): + fcm_service = messaging._get_messaging_service(app) + recorder = [] + fcm_service._client.session.mount( + url, testutils.MockAdapter(json.dumps(response), 200, recorder)) + return recorder class TestSend: @@ -1641,7 +1650,8 @@ def test_send(self): assert recorder[0].url == self._get_url('explicit-project-id') assert recorder[0].headers['X-GOOG-API-FORMAT-VERSION'] == '2' assert recorder[0].headers['X-FIREBASE-CLIENT'] == self._CLIENT_VERSION - assert recorder[0]._extra_kwargs['timeout'] is None + assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( + _utils.DEFAULT_HTTP_TIMEOUT_SECONDS, 0.001) body = {'message': messaging._MessagingService.encode_message(msg)} assert json.loads(recorder[0].body.decode()) == body @@ -2224,6 +2234,8 @@ def test_subscribe_to_topic(self, args): assert recorder[0].method == 'POST' assert recorder[0].url == self._get_url('iid/v1:batchAdd') assert json.loads(recorder[0].body.decode()) == args[2] + assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( + _utils.DEFAULT_HTTP_TIMEOUT_SECONDS, 0.001) @pytest.mark.parametrize('status, exc_type', HTTP_ERROR_CODES.items()) def test_subscribe_to_topic_error(self, status, exc_type): @@ -2256,6 +2268,8 @@ def test_unsubscribe_from_topic(self, args): assert recorder[0].method == 'POST' assert recorder[0].url == self._get_url('iid/v1:batchRemove') assert json.loads(recorder[0].body.decode()) == args[2] + assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( + _utils.DEFAULT_HTTP_TIMEOUT_SECONDS, 0.001) @pytest.mark.parametrize('status, exc_type', HTTP_ERROR_CODES.items()) def test_unsubscribe_from_topic_error(self, status, exc_type): diff --git a/tests/test_project_management.py b/tests/test_project_management.py index aa717bbf..70392df4 100644 --- a/tests/test_project_management.py +++ b/tests/test_project_management.py @@ -22,6 +22,7 @@ import firebase_admin from firebase_admin import exceptions from firebase_admin import project_management +from firebase_admin import _utils from tests import testutils OPERATION_IN_PROGRESS_RESPONSE = json.dumps({ @@ -522,6 +523,8 @@ def _assert_request_is_correct( assert request.url == expected_url client_version = 'Python/Admin/{0}'.format(firebase_admin.__version__) assert request.headers['X-Client-Version'] == client_version + assert request._extra_kwargs['timeout'] == pytest.approx( + _utils.DEFAULT_HTTP_TIMEOUT_SECONDS, 0.001) if expected_body is None: assert request.body is None else: From f6b15d9447a00d63b8a1caeb70344f6eb2b43cc7 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Tue, 28 Jan 2020 22:37:18 -0800 Subject: [PATCH 3/3] Fixing lint error --- tests/test_messaging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_messaging.py b/tests/test_messaging.py index 36ca03e4..0d5a68e6 100644 --- a/tests/test_messaging.py +++ b/tests/test_messaging.py @@ -1561,7 +1561,7 @@ def test_send(self, timeout): def test_topic_management(self, timeout): app = self._app_with_timeout(timeout) recorder = self._instrument_service( - app, 'https://iid.googleapis.com',{'results': [{}, {'error': 'error_reason'}]}) + app, 'https://iid.googleapis.com', {'results': [{}, {'error': 'error_reason'}]}) messaging.subscribe_to_topic(['1'], 'a') assert len(recorder) == 1 if timeout is None: