From 03305c72c762e63a834cd834ae25626c4f53a45d Mon Sep 17 00:00:00 2001 From: Peter Pfeufer Date: Mon, 22 Nov 2021 18:10:02 +0100 Subject: [PATCH 1/5] Added error handling when fetching from GitLab API --- allianceauth/templatetags/admin_status.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/allianceauth/templatetags/admin_status.py b/allianceauth/templatetags/admin_status.py index 9170b168..dbfa46ac 100644 --- a/allianceauth/templatetags/admin_status.py +++ b/allianceauth/templatetags/admin_status.py @@ -156,14 +156,26 @@ def _latests_versions(tags: list) -> tuple: def _fetch_list_from_gitlab(url: str, max_pages: int = MAX_PAGES) -> list: - """returns a list from the GitLab API. Supports pageing""" + """returns a list from the GitLab API. Supports paging""" result = list() + for page in range(1, max_pages + 1): - request = requests.get( - url, params={'page': page}, timeout=REQUESTS_TIMEOUT - ) - request.raise_for_status() + try: + request = requests.get( + url, params={'page': page}, timeout=REQUESTS_TIMEOUT + ) + except requests.exceptions.RequestException as e: + error_str = str(e) + + logger.error( + f'Unable to fetch from GitLab API. Error: {error_str}', + exc_info=True, + ) + + return result + result += request.json() + if 'x-total-pages' in request.headers: try: total_pages = int(request.headers['x-total-pages']) From 20da1ebfab47f19b9aa19655c7cc214ddb4e2e56 Mon Sep 17 00:00:00 2001 From: Peter Pfeufer Date: Mon, 22 Nov 2021 18:56:33 +0100 Subject: [PATCH 2/5] request.raise_for_status() re-added --- allianceauth/templatetags/admin_status.py | 1 + 1 file changed, 1 insertion(+) diff --git a/allianceauth/templatetags/admin_status.py b/allianceauth/templatetags/admin_status.py index dbfa46ac..c84b47fe 100644 --- a/allianceauth/templatetags/admin_status.py +++ b/allianceauth/templatetags/admin_status.py @@ -164,6 +164,7 @@ def _fetch_list_from_gitlab(url: str, max_pages: int = MAX_PAGES) -> list: request = requests.get( url, params={'page': page}, timeout=REQUESTS_TIMEOUT ) + request.raise_for_status() except requests.exceptions.RequestException as e: error_str = str(e) From 8faadc23b0621c674e4cd816f012cd64c6649765 Mon Sep 17 00:00:00 2001 From: Peter Pfeufer Date: Tue, 23 Nov 2021 01:02:08 +0100 Subject: [PATCH 3/5] Set logger to warning to not trigger a notification to admins every time --- allianceauth/templatetags/admin_status.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/allianceauth/templatetags/admin_status.py b/allianceauth/templatetags/admin_status.py index c84b47fe..f7de2513 100644 --- a/allianceauth/templatetags/admin_status.py +++ b/allianceauth/templatetags/admin_status.py @@ -168,7 +168,7 @@ def _fetch_list_from_gitlab(url: str, max_pages: int = MAX_PAGES) -> list: except requests.exceptions.RequestException as e: error_str = str(e) - logger.error( + logger.warning( f'Unable to fetch from GitLab API. Error: {error_str}', exc_info=True, ) From 49548d6f9f02ef7b890ec7eb4490b5f612b56299 Mon Sep 17 00:00:00 2001 From: Peter Pfeufer Date: Tue, 23 Nov 2021 01:23:34 +0100 Subject: [PATCH 4/5] test_can_handle_connection_timeout added --- allianceauth/authentication/tests/test_templatetags.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/allianceauth/authentication/tests/test_templatetags.py b/allianceauth/authentication/tests/test_templatetags.py index 9a7a313c..8b142e17 100644 --- a/allianceauth/authentication/tests/test_templatetags.py +++ b/allianceauth/authentication/tests/test_templatetags.py @@ -1,6 +1,7 @@ from math import ceil from unittest.mock import patch +import requests import requests_mock from packaging.version import Version as Pep440Version @@ -307,3 +308,9 @@ class TestFetchListFromGitlab(TestCase): result = _fetch_list_from_gitlab(self.url, max_pages=max_pages) self.assertEqual(result, GITHUB_TAGS[:4]) self.assertEqual(requests_mocker.call_count, max_pages) + + @requests_mock.mock() + def test_can_handle_connection_timeout(self, requests_mocker): + requests_mocker.get(self.url, exc=requests.exceptions.ConnectTimeout) + with self.assertRaises(requests.exceptions.ConnectTimeout): + result = _fetch_list_from_gitlab(self.url) From b6d6c68e54c88b6e3155ebe81e19113500080679 Mon Sep 17 00:00:00 2001 From: Peter Pfeufer Date: Tue, 23 Nov 2021 02:31:18 +0100 Subject: [PATCH 5/5] Fix tests Thanks to @ErikKalkoken for helping here --- .../authentication/tests/test_templatetags.py | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/allianceauth/authentication/tests/test_templatetags.py b/allianceauth/authentication/tests/test_templatetags.py index 8b142e17..50361724 100644 --- a/allianceauth/authentication/tests/test_templatetags.py +++ b/allianceauth/authentication/tests/test_templatetags.py @@ -310,7 +310,23 @@ class TestFetchListFromGitlab(TestCase): self.assertEqual(requests_mocker.call_count, max_pages) @requests_mock.mock() - def test_can_handle_connection_timeout(self, requests_mocker): - requests_mocker.get(self.url, exc=requests.exceptions.ConnectTimeout) - with self.assertRaises(requests.exceptions.ConnectTimeout): - result = _fetch_list_from_gitlab(self.url) + @patch(MODULE_PATH + '.admin_status.logger') + def test_should_not_raise_any_exception_from_github_request_but_log_as_warning( + self, requests_mocker, mock_logger + ): + for my_exception in [ + requests.exceptions.ConnectionError, + requests.exceptions.HTTPError, + requests.exceptions.URLRequired, + requests.exceptions.TooManyRedirects, + requests.exceptions.ConnectTimeout, + requests.exceptions.Timeout, + + ]: + requests_mocker.get(self.url, exc=my_exception) + try: + result = _fetch_list_from_gitlab(self.url) + except Exception as ex: + self.fail(f"Unexpected exception raised: {ex}") + self.assertTrue(mock_logger.warning.called) + self.assertListEqual(result, [])