From e5eead0a520d67e654ca4434fb9dd3cf316f5350 Mon Sep 17 00:00:00 2001 From: Ben Weissmann Date: Sat, 11 Jul 2020 12:38:04 -0400 Subject: [PATCH] fix(exception-handling): Fix handling of network and other non-status-code errors when polling for datafile --- README.md | 2 +- optimizely/config_manager.py | 22 +++++--- tests/test_config_manager.py | 98 +++++++++++++++++++++++++++++++++++- 3 files changed, 113 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 5723501b..00ee22f1 100644 --- a/README.md +++ b/README.md @@ -176,7 +176,7 @@ Build and install the SDK with pip, using the following command: To get test dependencies installed, use a modified version of the install command: - pip install -e .[test] + pip install -e '.[test]' You can run all unit tests with: diff --git a/optimizely/config_manager.py b/optimizely/config_manager.py index 46a7dac3..b0f959bf 100644 --- a/optimizely/config_manager.py +++ b/optimizely/config_manager.py @@ -344,9 +344,14 @@ def fetch_datafile(self): if self.last_modified: request_headers[enums.HTTPHeaders.IF_MODIFIED_SINCE] = self.last_modified - response = requests.get( - self.datafile_url, headers=request_headers, timeout=enums.ConfigManager.REQUEST_TIMEOUT, - ) + try: + response = requests.get( + self.datafile_url, headers=request_headers, timeout=enums.ConfigManager.REQUEST_TIMEOUT, + ) + except requests_exceptions.RequestException as err: + self.logger.error('Fetching datafile from {} failed. Error: {}'.format(self.datafile_url, str(err))) + return + self._handle_response(response) @property @@ -411,7 +416,12 @@ def fetch_datafile(self): if self.last_modified: request_headers[enums.HTTPHeaders.IF_MODIFIED_SINCE] = self.last_modified - response = requests.get( - self.datafile_url, headers=request_headers, timeout=enums.ConfigManager.REQUEST_TIMEOUT, - ) + try: + response = requests.get( + self.datafile_url, headers=request_headers, timeout=enums.ConfigManager.REQUEST_TIMEOUT, + ) + except requests_exceptions.RequestException as err: + self.logger.error('Fetching datafile from {} failed. Error: {}'.format(self.datafile_url, str(err))) + return + self._handle_response(response) diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 42f5f76d..15c93245 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -394,8 +394,8 @@ def test_fetch_datafile(self, _): self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) self.assertTrue(project_config_manager.is_running) - def test_fetch_datafile__exception_raised(self, _): - """ Test that config_manager keeps running if exception is raised when fetching datafile. """ + def test_fetch_datafile__status_exception_raised(self, _): + """ Test that config_manager keeps running if status code exception is raised when fetching datafile. """ class MockExceptionResponse(object): def raise_for_status(self): raise requests.exceptions.RequestException('Error Error !!') @@ -434,6 +434,45 @@ def raise_for_status(self): # Confirm that config manager keeps running self.assertTrue(project_config_manager.is_running) + def test_fetch_datafile__request_exception_raised(self, _): + """ Test that config_manager keeps running if a request exception is raised when fetching datafile. """ + sdk_key = 'some_key' + mock_logger = mock.Mock() + with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'): + project_config_manager = config_manager.PollingConfigManager(sdk_key=sdk_key, logger=mock_logger) + expected_datafile_url = enums.ConfigManager.DATAFILE_URL_TEMPLATE.format(sdk_key=sdk_key) + test_headers = {'Last-Modified': 'New Time'} + test_datafile = json.dumps(self.config_dict_with_features) + test_response = requests.Response() + test_response.status_code = 200 + test_response.headers = test_headers + test_response._content = test_datafile + with mock.patch('requests.get', return_value=test_response): + project_config_manager.fetch_datafile() + + self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified) + self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) + + # Call fetch_datafile again, but raise exception this time + with mock.patch( + 'requests.get', + side_effect=requests.exceptions.RequestException('Error Error !!'), + ) as mock_requests: + project_config_manager.fetch_datafile() + + mock_requests.assert_called_once_with( + expected_datafile_url, + headers={'If-Modified-Since': test_headers['Last-Modified']}, + timeout=enums.ConfigManager.REQUEST_TIMEOUT, + ) + mock_logger.error.assert_called_once_with('Fetching datafile from {} failed. Error: Error Error !!'.format( + expected_datafile_url + )) + self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified) + self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) + # Confirm that config manager keeps running + self.assertTrue(project_config_manager.is_running) + def test_is_running(self, _): """ Test that polling thread is running after instance of PollingConfigManager is created. """ with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'): @@ -492,3 +531,58 @@ def test_fetch_datafile(self, _): ) self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) + + def test_fetch_datafile__request_exception_raised(self, _): + """ Test that config_manager keeps running if a request exception is raised when fetching datafile. """ + datafile_access_token = 'some_token' + sdk_key = 'some_key' + mock_logger = mock.Mock() + + with mock.patch('optimizely.config_manager.AuthDatafilePollingConfigManager.fetch_datafile'): + project_config_manager = config_manager.AuthDatafilePollingConfigManager( + datafile_access_token=datafile_access_token, sdk_key=sdk_key, logger=mock_logger) + expected_datafile_url = enums.ConfigManager.AUTHENTICATED_DATAFILE_URL_TEMPLATE.format(sdk_key=sdk_key) + test_headers = {'Last-Modified': 'New Time'} + test_datafile = json.dumps(self.config_dict_with_features) + test_response = requests.Response() + test_response.status_code = 200 + test_response.headers = test_headers + test_response._content = test_datafile + + # Call fetch_datafile and assert that request was sent with correct authorization header + with mock.patch('requests.get', + return_value=test_response) as mock_request: + project_config_manager.fetch_datafile() + + mock_request.assert_called_once_with( + expected_datafile_url, + headers={'Authorization': 'Bearer {datafile_access_token}'.format( + datafile_access_token=datafile_access_token)}, + timeout=enums.ConfigManager.REQUEST_TIMEOUT, + ) + + self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) + + # Call fetch_datafile again, but raise exception this time + with mock.patch( + 'requests.get', + side_effect=requests.exceptions.RequestException('Error Error !!'), + ) as mock_requests: + project_config_manager.fetch_datafile() + + mock_requests.assert_called_once_with( + expected_datafile_url, + headers={ + 'If-Modified-Since': test_headers['Last-Modified'], + 'Authorization': 'Bearer {datafile_access_token}'.format( + datafile_access_token=datafile_access_token), + }, + timeout=enums.ConfigManager.REQUEST_TIMEOUT, + ) + mock_logger.error.assert_called_once_with('Fetching datafile from {} failed. Error: Error Error !!'.format( + expected_datafile_url + )) + self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified) + self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig) + # Confirm that config manager keeps running + self.assertTrue(project_config_manager.is_running)