Merge branch 'master' into peter/datafile-accessor-json

aliabbasrizvi · web-flow · commit 495f8da7af0f · 2020-07-14T12:30:45.000-07:00
diff --git a/.travis.yml b/.travis.yml
@@ -19,6 +19,7 @@ stages:
   - 'Linting'
   - 'Integration tests'
   - 'Test'
+  - 'Source Clear'
 
 jobs:
   include:
@@ -31,18 +32,6 @@ jobs:
       notifications:
         email: false
 
-    - stage: 'Lint markdown files'
-      os: linux
-      language: generic
-      before_install: skip
-      install:
-        - npm i -g markdown-spellcheck
-      before_script:
-        - wget --quiet https://raw.githubusercontent.com/optimizely/mdspell-config/master/.spelling
-      script:
-        - mdspell -a -n -r --en-us '**/*.md'
-      after_success: skip
-
     - stage: 'Linting'
       language: python
       python: "2.7"
@@ -63,12 +52,18 @@ jobs:
         - $HOME/travisci-tools/trigger-script-with-status-update.sh
       after_success: travis_terminate 0
     - stage: 'Test'
-      addons:
-        srcclr: true
       dist: xenial
       python: "3.7"
     - stage: 'Test'
-      addons:
-        srcclr: true
       dist: xenial
       python: "3.8"
+
+    - stage: 'Source Clear'
+      if: type = cron
+      addons:
+        srcclr: true
+      before_install: skip
+      install: skip
+      before_script: skip
+      script: skip
+      after_success: skip
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Optimizely Python SDK Changelog
 
+## 3.5.1
+July 10th, 2020
+
+### Bug Fixes:
+* Fixed HTTP request exception handling in `PollingConfigManager`. ([#285](https://github.com/optimizely/python-sdk/pull/285))
+
 ## 3.5.0
 July 9th, 2020
 
diff --git 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
@@ -325,7 +325,7 @@ def _handle_response(self, response):
         """
         try:
             response.raise_for_status()
-        except requests_exceptions.HTTPError as err:
+        except requests_exceptions.RequestException as err:
             self.logger.error('Fetching datafile from {} failed. Error: {}'.format(self.datafile_url, str(err)))
             return
 
@@ -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/optimizely/version.py b/optimizely/version.py
@@ -11,5 +11,5 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-version_info = (3, 5, 0)
+version_info = (3, 5, 1)
 __version__ = '.'.join(str(v) for v in version_info)
diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py
@@ -392,6 +392,86 @@ def test_fetch_datafile(self, _):
         )
         self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified)
         self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
+        self.assertTrue(project_config_manager.is_running)
+
+    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 !!')
+
+        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', return_value=MockExceptionResponse()) 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_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. """
@@ -451,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)
