fix: prevent endless loop on 403 GitHub response (#866)

Signed-off-by: Ben Selwyn-Smith <benselwynsmith@googlemail.com>

Author: benmss

src/macaron/config/defaults.ini

@@ -4,6 +4,8 @@
 [requests]
 # The default timeout in seconds for 'requests' API calls.
 timeout = 10
+# The number of times to re-attempt to retrieve a resource when an error is encountered.
+error_retries = 5
 
 [downloads]
 # The default timeout in seconds for downloading assets.

src/macaron/util.py

@@ -37,17 +37,23 @@ def send_get_http(url: str, headers: dict) -> dict:
     """
     logger.debug("GET - %s", url)
     timeout = defaults.getint("requests", "timeout", fallback=10)
+    error_retries = defaults.getint("requests", "error_retries", fallback=5)
+    retry_counter = error_retries
     response = requests.get(url=url, headers=headers, timeout=timeout)
     while response.status_code != 200:
         logger.error(
             "Receiving error code %s from server. Message: %s.",
             response.status_code,
             response.text,
         )
+        if retry_counter <= 0:
+            logger.debug("Maximum retries reached: %s", error_retries)
+            return {}
         if response.status_code == 403:
             check_rate_limit(response)
         else:
             return {}
+        retry_counter = retry_counter - 1
         response = requests.get(url=url, headers=headers, timeout=timeout)
 
     return dict(response.json())
@@ -81,6 +87,8 @@ def send_get_http_raw(
     logger.debug("GET - %s", url)
     if not timeout:
         timeout = defaults.getint("requests", "timeout", fallback=10)
+    error_retries = defaults.getint("requests", "error_retries", fallback=5)
+    retry_counter = error_retries
     try:
         response = requests.get(
             url=url,
@@ -99,10 +107,14 @@ def send_get_http_raw(
             "Receiving error code %s from server.",
             response.status_code,
         )
+        if retry_counter <= 0:
+            logger.debug("Maximum retries reached: %s", error_retries)
+            return None
         if response.status_code == 403:
             check_rate_limit(response)
         else:
             return None
+        retry_counter = retry_counter - 1
         response = requests.get(
             url=url,
             headers=headers,
@@ -121,7 +133,6 @@ def check_rate_limit(response: Response) -> None:
     response : Response
         The latest response from GitHub API.
     """
-    remains = 0
     if "X-RateLimit-Remaining" in response.headers:
         remains = int(response.headers["X-RateLimit-Remaining"])
     else:

tests/test_util.py

@@ -1,14 +1,19 @@
-# Copyright (c) 2022 - 2022, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2022 - 2024, Oracle and/or its affiliates. All rights reserved.
 # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/.
 
 """
 This module test the Util methods
 """
-
+from collections.abc import Callable
 from unittest import TestCase
 from unittest.mock import call, patch
 
+from pytest_httpserver import HTTPServer
+from werkzeug import Request, Response
+
 from macaron import util
+from macaron.config.defaults import defaults
+from macaron.util import send_get_http_raw
 
 
 class TestUtil(TestCase):
@@ -78,3 +83,55 @@ def test_copy_file_bulk(self) -> None:
                             call("/src/path/foo/file2", "/target/path/foo/file2"),
                         ]
                     )
+
+
+def _response_generator(target_value: int) -> Callable[[Request], Response]:
+    """Return a generator with closure so a value can be tracked across multiple invocations."""
+    value = 0
+
+    def generator(request: Request) -> Response:  # pylint: disable=unused-argument
+        """Add the next value as a header and adjust the status code based on the value."""
+        nonlocal value, target_value
+        value += 1
+        response = Response()
+        response.status_code = 403 if value <= (target_value + 1) else 200
+        response.headers["X-VALUE"] = str(value)
+        return response
+
+    return generator
+
+
+def _http_setup(retries: int, httpserver: HTTPServer) -> str:
+    """Set up the http server for a GET test."""
+    # Get a localhost URL.
+    mocked_url: str = httpserver.url_for("")
+
+    # Create and assign the stateful handler.
+    handler = _response_generator(retries)
+    httpserver.expect_request("").respond_with_handler(handler)
+    return mocked_url
+
+
+def test_get_http_partial_failure(httpserver: HTTPServer) -> None:
+    """Test the http GET operation when some errors are received before the request succeeds."""
+    # Retrieve the allowed number of retries on a failed request and reduce it by 1.
+    target_value = defaults.getint("requests", "error_retries", fallback=5) - 1
+
+    mocked_url = _http_setup(target_value, httpserver)
+
+    # Test for a correct response after the expected number of retries.
+    response = send_get_http_raw(mocked_url)
+    assert response
+    assert "X-VALUE" in response.headers
+    assert response.headers["X-VALUE"] == str(target_value + 2)
+
+
+def test_get_http_complete_failure(httpserver: HTTPServer) -> None:
+    """Test the http GET operation when too many errors are received and the request fails."""
+    # Retrieve the allowed number of retries on a failed request.
+    target_value = defaults.getint("requests", "error_retries", fallback=5)
+
+    mocked_url = _http_setup(target_value, httpserver)
+
+    # Assert the request fails and returns nothing.
+    assert send_get_http_raw(mocked_url) is None