From 6bc7353514e53a313b2eb13362354cb4224b76f2 Mon Sep 17 00:00:00 2001 From: Evgenii Alekseev Date: Wed, 15 Nov 2023 18:01:22 +0200 Subject: [PATCH] add retry on authorization errros --- .github/workflows/setup.sh | 2 +- Dockerfile | 2 +- package/archlinux/PKGBUILD | 2 +- pyproject.toml | 1 + src/ahriman/core/http/sync_ahriman_client.py | 49 ++++++++++++++ src/ahriman/core/http/sync_http_client.py | 4 +- .../core/http/test_sync_ahriman_client.py | 65 +++++++++++++++++++ 7 files changed, 120 insertions(+), 5 deletions(-) diff --git a/.github/workflows/setup.sh b/.github/workflows/setup.sh index 18e96b56..98dde403 100755 --- a/.github/workflows/setup.sh +++ b/.github/workflows/setup.sh @@ -10,7 +10,7 @@ echo -e '[arcanisrepo]\nServer = http://repo.arcanis.me/$arch\nSigLevel = Never' # refresh the image pacman --noconfirm -Syu # main dependencies -pacman --noconfirm -Sy base-devel devtools git pyalpm python-cerberus python-inflection python-passlib python-requests python-srcinfo python-systemd sudo +pacman --noconfirm -Sy base-devel devtools git pyalpm python-cerberus python-inflection python-passlib python-requests python-srcinfo python-tenacity sudo # make dependencies pacman --noconfirm -Sy python-build python-flit python-installer python-wheel # optional dependencies diff --git a/Dockerfile b/Dockerfile index 3567bde6..92d587d5 100644 --- a/Dockerfile +++ b/Dockerfile @@ -31,7 +31,7 @@ RUN useradd -m -d "/home/build" -s "/usr/bin/nologin" build && \ COPY "docker/install-aur-package.sh" "/usr/local/bin/install-aur-package" ## install package dependencies ## darcs is not installed by reasons, because it requires a lot haskell packages which dramatically increase image size -RUN pacman -Sy --noconfirm --asdeps devtools git pyalpm python-cerberus python-inflection python-passlib python-requests python-srcinfo && \ +RUN pacman -Sy --noconfirm --asdeps devtools git pyalpm python-cerberus python-inflection python-passlib python-requests python-srcinfo python-tenacity && \ pacman -Sy --noconfirm --asdeps python-build python-flit python-installer python-wheel && \ pacman -Sy --noconfirm --asdeps breezy mercurial python-aiohttp python-aiohttp-cors python-boto3 python-cryptography python-jinja python-requests-unixsocket python-systemd rsync subversion && \ runuser -u build -- install-aur-package python-aioauth-client python-aiohttp-apispec-git python-aiohttp-jinja2 \ diff --git a/package/archlinux/PKGBUILD b/package/archlinux/PKGBUILD index 15511be2..9a327272 100644 --- a/package/archlinux/PKGBUILD +++ b/package/archlinux/PKGBUILD @@ -7,7 +7,7 @@ pkgdesc="ArcH linux ReposItory MANager" arch=('any') url="https://github.com/arcan1s/ahriman" license=('GPL3') -depends=('devtools>=1:1.0.0' 'git' 'pyalpm' 'python-cerberus' 'python-inflection' 'python-passlib' 'python-requests' 'python-srcinfo') +depends=('devtools>=1:1.0.0' 'git' 'pyalpm' 'python-cerberus' 'python-inflection' 'python-passlib' 'python-requests' 'python-srcinfo' 'python-tenacity') makedepends=('python-build' 'python-flit' 'python-installer' 'python-wheel') optdepends=('breezy: -bzr packages support' 'darcs: -darcs packages support' diff --git a/pyproject.toml b/pyproject.toml index 75a89252..913a4495 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,6 +21,7 @@ dependencies = [ "passlib", "requests", "srcinfo", + "tenacity", ] dynamic = ["version"] diff --git a/src/ahriman/core/http/sync_ahriman_client.py b/src/ahriman/core/http/sync_ahriman_client.py index 982d633e..2defd9b1 100644 --- a/src/ahriman/core/http/sync_ahriman_client.py +++ b/src/ahriman/core/http/sync_ahriman_client.py @@ -19,8 +19,10 @@ # import contextlib import requests +import tenacity from functools import cached_property +from typing import Any from urllib.parse import urlparse from ahriman import __version__ @@ -57,6 +59,34 @@ class SyncAhrimanClient(SyncHttpClient): return session + @staticmethod + def is_retry_allowed(exception: BaseException) -> bool: + """ + check if retry is allowed for the exception + + Args: + exception(BaseException): exception raised + + Returns: + bool: True in case if exception is in white list and false otherwise + """ + if not isinstance(exception, requests.RequestException): + return False # not a request exception + status_code = exception.response.status_code if exception.response is not None else None + return status_code in (401,) + + @staticmethod + def on_retry(state: tenacity.RetryCallState) -> None: + """ + action to be called before retry + + Args: + state(tenacity.RetryCallState): current retry call state + """ + instance = next(arg for arg in state.args if isinstance(arg, SyncAhrimanClient)) + if hasattr(instance, "session"): # only if it was initialized + del instance.session # clear session + def _login(self, session: requests.Session) -> None: """ process login to the service @@ -83,3 +113,22 @@ class SyncAhrimanClient(SyncHttpClient): str: full url for web service to log in """ return f"{self.address}/api/v1/login" + + @tenacity.retry( + stop=tenacity.stop_after_attempt(2), + retry=tenacity.retry_if_exception(is_retry_allowed), + after=on_retry, + reraise=True, + ) + def make_request(self, *args: Any, **kwargs: Any) -> requests.Response: + """ + perform request with specified parameters + + Args: + *args(Any): request method positional arguments + **kwargs(Any): request method keyword arguments + + Returns: + requests.Response: response object + """ + return SyncHttpClient.make_request(self, *args, **kwargs) diff --git a/src/ahriman/core/http/sync_http_client.py b/src/ahriman/core/http/sync_http_client.py index 6b670c31..06256c9a 100644 --- a/src/ahriman/core/http/sync_http_client.py +++ b/src/ahriman/core/http/sync_http_client.py @@ -77,12 +77,12 @@ class SyncHttpClient(LazyLogging): return session @staticmethod - def exception_response_text(exception: requests.exceptions.RequestException) -> str: + def exception_response_text(exception: requests.RequestException) -> str: """ safe response exception text generation Args: - exception(requests.exceptions.RequestException): exception raised + exception(requests.RequestException): exception raised Returns: str: text of the response if it is not None and empty string otherwise diff --git a/tests/ahriman/core/http/test_sync_ahriman_client.py b/tests/ahriman/core/http/test_sync_ahriman_client.py index 2a83bcac..04882e9b 100644 --- a/tests/ahriman/core/http/test_sync_ahriman_client.py +++ b/tests/ahriman/core/http/test_sync_ahriman_client.py @@ -1,8 +1,10 @@ import pytest import requests import requests_unixsocket +import tenacity from pytest_mock import MockerFixture +from unittest.mock import call as MockCall from ahriman.core.http import SyncAhrimanClient from ahriman.models.user import User @@ -30,6 +32,37 @@ def test_session_unix_socket(ahriman_client: SyncAhrimanClient, mocker: MockerFi login_mock.assert_not_called() +def test_is_retry_allowed() -> None: + """ + must allow retries on 401 errors + """ + assert not SyncAhrimanClient.is_retry_allowed(Exception()) + + response = requests.Response() + response.status_code = 400 + assert not SyncAhrimanClient.is_retry_allowed(requests.HTTPError(response=response)) + + response.status_code = 401 + assert SyncAhrimanClient.is_retry_allowed(requests.HTTPError(response=response)) + + response.status_code = 403 + assert not SyncAhrimanClient.is_retry_allowed(requests.HTTPError(response=response)) + + +def test_on_retry(ahriman_client: SyncAhrimanClient) -> None: + """ + must remove session on retry + """ + SyncAhrimanClient.on_retry( + tenacity.RetryCallState( + retry_object=tenacity.Retrying(), + fn=SyncAhrimanClient.make_request, + args=(ahriman_client,), + kwargs={}, + ) + ) + + def test_login(ahriman_client: SyncAhrimanClient, user: User, mocker: MockerFixture) -> None: """ must login user @@ -79,3 +112,35 @@ def test_login_url(ahriman_client: SyncAhrimanClient) -> None: """ assert ahriman_client._login_url().startswith(ahriman_client.address) assert ahriman_client._login_url().endswith("/api/v1/login") + + +def test_make_request_retry(ahriman_client: SyncAhrimanClient, mocker: MockerFixture) -> None: + """ + must retry HTTP request + """ + response_ok = requests.Response() + response_ok.status_code = 200 + response_error = requests.Response() + response_error.status_code = 401 + # login on init -> request with error -> login on error -> request without error + request_mock = mocker.patch("requests.Session.request", side_effect=[ + response_ok, + response_error, + response_ok, + response_ok, + ]) + + ahriman_client.auth = ("username", "password") + assert ahriman_client.make_request("GET", "url") is not None + request_mock.assert_has_calls([ + MockCall("POST", "http://127.0.0.1:8080/api/v1/login", params=None, data=None, headers=None, files=None, + json={"username": "username", "password": "password"}, + auth=ahriman_client.auth, timeout=ahriman_client.timeout), + MockCall("GET", "url", params=None, data=None, headers=None, files=None, json=None, + auth=ahriman_client.auth, timeout=ahriman_client.timeout), + MockCall("POST", "http://127.0.0.1:8080/api/v1/login", params=None, data=None, headers=None, files=None, + json={"username": "username", "password": "password"}, + auth=ahriman_client.auth, timeout=ahriman_client.timeout), + MockCall("GET", "url", params=None, data=None, headers=None, files=None, json=None, + auth=ahriman_client.auth, timeout=ahriman_client.timeout), + ])