Merge pull request #230 from dimagi/mk/skip-stacktrace

Show detailed exception only when needed

.github/workflows/test.yml

@@ -64,4 +64,8 @@ jobs:
           HQ_API_KEY: ${{ secrets.HQ_API_KEY }}
       - run: mypy --install-types --non-interactive @mypy_typed_modules.txt
       - run: coverage lcov -o coverage/lcov.info
-      - uses: coverallsapp/github-action@v1
+      - name: Coveralls
+        uses: coverallsapp/github-action@v2
+        with:
+          github-token:
+            ${{ secrets.GITHUB_TOKEN }}

commcare_export/commcare_hq_client.py

@@ -9,7 +9,7 @@
 
 import copy
 import logging
-import time
+import sys
 from collections import OrderedDict
 from math import ceil
 from urllib.parse import urlencode
@@ -20,7 +20,6 @@
 
 import commcare_export
 from commcare_export.repeatable_iterator import RepeatableIterator
-from datetime import datetime
 
 AUTH_MODE_PASSWORD = 'password'
 AUTH_MODE_APIKEY = 'apikey'
@@ -30,10 +29,12 @@
 LATEST_KNOWN_VERSION = '0.5'
 RESOURCE_REPEAT_LIMIT = 10
 
+
 def on_wait(details):
     time_to_wait = details["wait"]
     logger.warning(f"Rate limit reached. Waiting for {time_to_wait} seconds.")
 
+
 def on_backoff(details):
     _log_backoff(details, 'Waiting for retry.')
 
@@ -82,15 +83,15 @@ def __init__(
         password,
         auth_mode=AUTH_MODE_PASSWORD,
         version=LATEST_KNOWN_VERSION,
-        checkpoint_manager=None
     ):
         self.version = version
         self.url = url
         self.project = project
         self.__auth = self._get_auth(username, password, auth_mode)
         self.__session = None
 
-    def _get_auth(self, username, password, mode):
+    @staticmethod
+    def _get_auth(username, password, mode):
         if mode == AUTH_MODE_PASSWORD:
             return HTTPDigestAuth(username, password)
         elif mode == AUTH_MODE_APIKEY:
@@ -116,7 +117,8 @@ def session(self, session):
     def api_url(self):
         return '%s/a/%s/api/v%s' % (self.url, self.project, self.version)
 
-    def _should_raise_for_status(self, response):
+    @staticmethod
+    def _should_raise_for_status(response):
         return "Retry-After" not in response.headers
 
     def get(self, resource, params=None):
@@ -125,7 +127,7 @@ def get(self, resource, params=None):
         the amount of seconds specified in the Retry-After header from the response, after which it will raise
         an exception to trigger the retry action.
 
-        Currently a bit of a vulnerable stub that works for this
+        Currently, a bit of a vulnerable stub that works for this
         particular use case in the hands of a trusted user; would likely
         want this to work like (or via) slumber.
         """
@@ -151,7 +153,22 @@ def _get(resource, params=None):
                 resource_url, params=params, auth=self.__auth, timeout=60
             )
             if self._should_raise_for_status(response):
-                response.raise_for_status()
+                try:
+                    response.raise_for_status()
+                except Exception as e:
+                    # for non-verbose output, skip the stacktrace
+                    if not logger.isEnabledFor(logging.DEBUG):
+                        if isinstance(e, requests.exceptions.HTTPError) and response.status_code == 401:
+                            logger.error(
+                                f"#{e}. Please ensure that your CommCare HQ credentials are correct and auth-mode "
+                                f"is passed as 'apikey' if using API Key to authenticate. Also, verify that your "
+                                f"account has the necessary permissions to use commcare-export."
+                            )
+                        else:
+                            logger.error(str(e))
+                        sys.exit()
+                    raise e
+
             return response
 
         response = _get(resource, params)
@@ -168,13 +185,13 @@ def iterate(
         Assumes the endpoint is a list endpoint, and iterates over it
         making a lot of assumptions that it is like a tastypie endpoint.
         """
-        UNKNOWN_COUNT = 'unknown'
+        unknown_count = 'unknown'
         params = dict(params or {})
 
         def iterate_resource(resource=resource, params=params):
             more_to_fetch = True
             last_batch_ids = set()
-            total_count = UNKNOWN_COUNT
+            total_count = unknown_count
             fetched = 0
             repeat_counter = 0
             last_params = None
@@ -193,11 +210,11 @@ def iterate_resource(resource=resource, params=params):
                 batch = self.get(resource, params)
                 last_params = copy.copy(params)
                 batch_meta = batch['meta']
-                if total_count == UNKNOWN_COUNT or fetched >= total_count:
+                if total_count == unknown_count or fetched >= total_count:
                     if batch_meta.get('total_count'):
                         total_count = int(batch_meta['total_count'])
                     else:
-                        total_count = UNKNOWN_COUNT
+                        total_count = unknown_count
                     fetched = 0
 
                 batch_objects = batch['objects']
@@ -225,7 +242,7 @@ def iterate_resource(resource=resource, params=params):
                         # Handle the case where API is 'non-counting'
                         # and repeats the last batch
                         repeated_last_page_of_non_counting_resource = (
-                            not got_new_data and total_count == UNKNOWN_COUNT
+                            not got_new_data and total_count == unknown_count
                             and (limit and len(batch_objects) < limit)
                         )
                         more_to_fetch = not repeated_last_page_of_non_counting_resource

tests/test_commcare_hq_client.py

@@ -312,6 +312,60 @@ def test_raise_on_too_many_requests(self, session_mock):
 
         self.assertTrue(client._should_raise_for_status(response))
 
+    @patch('commcare_export.commcare_hq_client.logger')
+    @patch("commcare_export.commcare_hq_client.CommCareHqClient.session")
+    def test_get_with_forbidden_response_in_non_debug_mode(self, session_mock, logger_mock):
+        response = requests.Response()
+        response.status_code = 401
+        session_mock.get.return_value = response
+
+        logger_mock.isEnabledFor.return_value = False
+
+        with self.assertRaises(SystemExit):
+            CommCareHqClient(
+                '/fake/commcare-hq/url', 'fake-project', None, None
+            ).get("location")
+
+        logger_mock.error.assert_called_once_with(
+            "#401 Client Error: None for url: None. "
+            "Please ensure that your CommCare HQ credentials are correct and auth-mode is passed as 'apikey' "
+            "if using API Key to authenticate. Also, verify that your account has the necessary permissions "
+            "to use commcare-export.")
+
+    @patch('commcare_export.commcare_hq_client.logger')
+    @patch("commcare_export.commcare_hq_client.CommCareHqClient.session")
+    def test_get_with_other_http_failure_response_in_non_debug_mode(self, session_mock, logger_mock):
+        response = requests.Response()
+        response.status_code = 404
+        session_mock.get.return_value = response
+
+        logger_mock.isEnabledFor.return_value = False
+
+        with self.assertRaises(SystemExit):
+            CommCareHqClient(
+                '/fake/commcare-hq/url', 'fake-project', None, None
+            ).get("location")
+
+        logger_mock.error.assert_called_once_with(
+            "404 Client Error: None for url: None")
+
+    @patch('commcare_export.commcare_hq_client.logger')
+    @patch("commcare_export.commcare_hq_client.CommCareHqClient.session")
+    def test_get_with_http_failure_response_in_debug_mode(self, session_mock, logger_mock):
+        response = requests.Response()
+        response.status_code = 404
+        session_mock.get.return_value = response
+
+        logger_mock.isEnabledFor.return_value = True
+
+        try:
+            CommCareHqClient(
+                '/fake/commcare-hq/url', 'fake-project', None, None
+            ).get("location")
+        except Exception as e:
+            self.assertEqual(str(e), "404 Client Error: None for url: None")
+
+
 class TestDatePaginator(unittest.TestCase):
 
     @classmethod