chore: add test, docs, and helper for 409 retries

nejch · nejch · commit 3e1c62513307 · 2022-12-19T22:58:03.000+01:00
diff --git a/docs/api-usage-advanced.rst b/docs/api-usage-advanced.rst
@@ -123,9 +123,14 @@ GitLab server can sometimes return a transient HTTP error.
 python-gitlab can automatically retry in such case, when
 ``retry_transient_errors`` argument is set to ``True``.  When enabled,
 HTTP error codes 500 (Internal Server Error), 502 (502 Bad Gateway),
-503 (Service Unavailable), and 504 (Gateway Timeout) are retried.
-Additionally the HTTP error code 409 (Conflict) is retried if the text message
-mentions "Resource lock". It will retry until reaching the ``max_retries``
+503 (Service Unavailable), 504 (Gateway Timeout), and Cloudflare
+errors (520-530) are retried.
+
+Additionally, HTTP error code 409 (Conflict) is retried if the reason
+is a
+`Resource lock <https://gitlab.com/gitlab-org/gitlab/-/blob/443c12cf3b238385db728f03b2cdbb4f17c70292/lib/api/api.rb#L111>`__.
+
+It will retry until reaching the ``max_retries``
 value. By default, ``retry_transient_errors`` is set to ``False`` and an
 exception is raised for these errors.
 
diff --git a/gitlab/client.py b/gitlab/client.py
@@ -751,13 +751,20 @@ def http_request(
             if 200 <= result.status_code < 300:
                 return result.response
 
-            if (429 == result.status_code and obey_rate_limit) or (
-                (
-                    result.status_code in gitlab.const.RETRYABLE_TRANSIENT_ERROR_CODES
-                    or (result.status_code == 409 and "Resource lock" in result.reason)
-                )
-                and retry_transient_errors
-            ):
+            def should_retry() -> bool:
+                if result.status_code == 429 and obey_rate_limit:
+                    return True
+
+                if not retry_transient_errors:
+                    return False
+                if result.status_code in gitlab.const.RETRYABLE_TRANSIENT_ERROR_CODES:
+                    return True
+                if result.status_code == 409 and "Resource lock" in result.reason:
+                    return True
+
+                return False
+
+            if should_retry():
                 # Response headers documentation:
                 # https://docs.gitlab.com/ee/user/admin_area/settings/user_and_ip_rate_limits.html#response-headers
                 if max_retries == -1 or cur_retries < max_retries:
diff --git a/tests/unit/test_gitlab_http_methods.py b/tests/unit/test_gitlab_http_methods.py
@@ -372,6 +372,63 @@ def response_callback(
     assert "http://example.com/api/v4/user/status" in error_message
 
 
+def test_http_request_on_409_resource_lock_retries(gl_retry):
+    url = "http://localhost/api/v4/user"
+    retried = False
+
+    def response_callback(
+        response: requests.models.Response,
+    ) -> requests.models.Response:
+        """We need a callback that adds a resource lock reason only on first call"""
+        nonlocal retried
+
+        if not retried:
+            response.reason = "Resource lock"
+
+        retried = True
+        return response
+
+    with responses.RequestsMock(response_callback=response_callback) as rsps:
+        rsps.add(
+            method=responses.GET,
+            url=url,
+            status=409,
+            match=helpers.MATCH_EMPTY_QUERY_PARAMS,
+        )
+        rsps.add(
+            method=responses.GET,
+            url=url,
+            status=200,
+            match=helpers.MATCH_EMPTY_QUERY_PARAMS,
+        )
+        response = gl_retry.http_request("get", "/user")
+
+    assert response.status_code == 200
+
+
+def test_http_request_on_409_resource_lock_without_retry_raises(gl):
+    url = "http://localhost/api/v4/user"
+
+    def response_callback(
+        response: requests.models.Response,
+    ) -> requests.models.Response:
+        """Without retry, this will fail on the first call"""
+        response.reason = "Resource lock"
+        return response
+
+    with responses.RequestsMock(response_callback=response_callback) as req_mock:
+        req_mock.add(
+            method=responses.GET,
+            url=url,
+            status=409,
+            match=helpers.MATCH_EMPTY_QUERY_PARAMS,
+        )
+        with pytest.raises(GitlabHttpError) as excinfo:
+            gl.http_request("get", "/user")
+
+    assert excinfo.value.response_code == 409
+
+
 @responses.activate
 def test_get_request(gl):
     url = "http://localhost/api/v4/projects"
