From 8a69a1aca26ac6d4aa688c6dadca60716e2a62f1 Mon Sep 17 00:00:00 2001 From: Anton Shutik Date: Fri, 15 Nov 2024 12:40:34 +0100 Subject: [PATCH 1/4] Added retry behaviour --- shopify_client/__init__.py | 84 ++++++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 17 deletions(-) diff --git a/shopify_client/__init__.py b/shopify_client/__init__.py index 4c8e15b..97dc9b7 100644 --- a/shopify_client/__init__.py +++ b/shopify_client/__init__.py @@ -3,6 +3,8 @@ from urllib.parse import urljoin import requests +from requests.adapters import HTTPAdapter +from urllib3 import Retry from shopify_client.hooks import rate_limit @@ -16,19 +18,46 @@ class ShopifyClient(requests.Session): - def __init__(self, api_url, api_token, api_version=SHOPIFY_API_VERSION, graphql_queries_dir=None): + def __init__( + self, + api_url, + api_token, + api_version=SHOPIFY_API_VERSION, + graphql_queries_dir=None, + ): super().__init__() self.api_url = api_url self.api_version = api_version - self.headers.update({"X-Shopify-Access-Token": api_token, "Content-Type": "application/json"}) + self.headers.update( + {"X-Shopify-Access-Token": api_token, "Content-Type": "application/json"} + ) + + retry_strategy = Retry( + total=10, + connect=3, + read=3, + status=5, + backoff_factor=0.5, + allowed_methods=frozenset(["GET", "POST", "PUT", "PATCH", "DELETE"]), + status_forcelist=frozenset([429, 500, 502, 503, 504]), + respect_retry_after_header=True, + ) + + adapter = HTTPAdapter(max_retries=retry_strategy) + self.mount("http://", adapter) + self.mount("https://", adapter) # Access - self.storefront_access_tokens = Endpoint(client=self, endpoint="storefront_access_tokens") + self.storefront_access_tokens = Endpoint( + client=self, endpoint="storefront_access_tokens" + ) # Billing self.application_charges = Endpoint(client=self, endpoint="application_charges") self.application_credits = Endpoint(client=self, endpoint="application_credits") - self.recurring_application_charges = Endpoint(client=self, endpoint="recurring_application_charges") + self.recurring_application_charges = Endpoint( + client=self, endpoint="recurring_application_charges" + ) # Customers self.customers = Endpoint(client=self, endpoint="customers", metafields=True) @@ -53,7 +82,9 @@ def __init__(self, api_url, api_token, api_version=SHOPIFY_API_VERSION, graphql_ self.marketing_events = Endpoint(client=self, endpoint="marketing_events") # Mobile Support - self.mobile_platform_applications = Endpoint(client=self, endpoint="mobile_platform_applications") + self.mobile_platform_applications = Endpoint( + client=self, endpoint="mobile_platform_applications" + ) # Online Store self.articles = Endpoint(client=self, endpoint="articles", metafields=True) @@ -62,7 +93,9 @@ def __init__(self, api_url, api_token, api_version=SHOPIFY_API_VERSION, graphql_ # Orders self.checkouts = Endpoint(client=self, endpoint="checkouts") - self.draft_orders = DraftOrdersEndpoint(client=self, endpoint="draft_orders", metafields=True) + self.draft_orders = DraftOrdersEndpoint( + client=self, endpoint="draft_orders", metafields=True + ) self.orders = OrdersEndpoint(client=self, endpoint="orders") # Plus @@ -70,27 +103,41 @@ def __init__(self, api_url, api_token, api_version=SHOPIFY_API_VERSION, graphql_ # Products self.collects = Endpoint(client=self, endpoint="collects") - self.collections = Endpoint(client=self, endpoint="collections", metafields=True) + self.collections = Endpoint( + client=self, endpoint="collections", metafields=True + ) self.custom_collections = Endpoint(client=self, endpoint="custom_collections") self.products = Endpoint(client=self, endpoint="products", metafields=True) - self.products.images = Endpoint(client=self, endpoint="products", sub_endpoint="images") - self.product_images = Endpoint(client=self, endpoint="product_images", metafields=True) - self.smart_collections = Endpoint(client=self, endpoint="smart_collections", metafields=True) + self.products.images = Endpoint( + client=self, endpoint="products", sub_endpoint="images" + ) + self.product_images = Endpoint( + client=self, endpoint="product_images", metafields=True + ) + self.smart_collections = Endpoint( + client=self, endpoint="smart_collections", metafields=True + ) self.variants = Endpoint(client=self, endpoint="variants", metafields=True) # Sales Channels - self.collections_listings = Endpoint(client=self, endpoint="collections_listings") + self.collections_listings = Endpoint( + client=self, endpoint="collections_listings" + ) self.checkouts = Endpoint(client=self, endpoint="checkouts") self.product_listings = Endpoint(client=self, endpoint="product_listings") self.resource_feedback = Endpoint(client=self, endpoint="resource_feedback") # Shipping and Fulfillment - self.assigned_fulfillment_orders = Endpoint(client=self, endpoint="assigned_fulfillment_orders") + self.assigned_fulfillment_orders = Endpoint( + client=self, endpoint="assigned_fulfillment_orders" + ) # TODO: Implement Fulfillment self.fulfillment_orders = Endpoint(client=self, endpoint="fulfillment_orders") self.carrier_services = Endpoint(client=self, endpoint="carrier_services") self.fulfillments = Endpoint(client=self, endpoint="fulfillments") - self.fulfillment_services = Endpoint(client=self, endpoint="fulfillment_services") + self.fulfillment_services = Endpoint( + client=self, endpoint="fulfillment_services" + ) # Shopify Payments self.shopify_payments = Endpoint(client=self, endpoint="shopify_payments") @@ -111,10 +158,13 @@ def __init__(self, api_url, api_token, api_version=SHOPIFY_API_VERSION, graphql_ # GraphQL self.query = GraphQL(client=self, graphql_queries_dir=graphql_queries_dir) - self.hooks["response"].append(rate_limit) - def request(self, method, url, *args, **kwargs): - response = super().request(method, urljoin(f"{self.api_url}/admin/api/{self.api_version}/", url), *args, **kwargs) + response = super().request( + method, + urljoin(f"{self.api_url}/admin/api/{self.api_version}/", url), + *args, + **kwargs, + ) logger.info(f"Requesting {method} {url}: {response.status_code}") return response @@ -124,4 +174,4 @@ def parse_response(self, response): except requests.exceptions.HTTPError as e: logger.warning(f"Failed to execute request: {response.text}") raise e - return response.json() \ No newline at end of file + return response.json() From d1de7921c1f8dafcf96bf2c66542bfbb391ac26b Mon Sep 17 00:00:00 2001 From: Anton Shutik Date: Fri, 15 Nov 2024 12:40:48 +0100 Subject: [PATCH 2/4] Added tests --- tests/test_client.py | 20 +++-- tests/test_retry.py | 174 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 7 deletions(-) create mode 100644 tests/test_retry.py diff --git a/tests/test_client.py b/tests/test_client.py index bfde93e..dbf8deb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,29 +1,35 @@ import requests import pytest -from shopify_client import ShopifyClient -from tests.conftest import CopyingMock + def test_client_initialization(shopify_client): assert shopify_client.headers["X-Shopify-Access-Token"] == "test-token" assert shopify_client.headers["Content-Type"] == "application/json" + def test_request_with_versioning(shopify_client, mocker): - mock_request = mocker.patch("requests.Session.request", return_value=mocker.Mock(status_code=200, json=lambda: {"result": "success"})) + mock_request = mocker.patch( + "requests.Session.request", + return_value=mocker.Mock(status_code=200, json=lambda: {"result": "success"}), + ) response = shopify_client.request("GET", "products.json") assert response.json() == {"result": "success"} mock_request.assert_called_once_with( - "GET", - "https://test-shop.myshopify.com/admin/api/2024-10/products.json" + "GET", "https://test-shop.myshopify.com/admin/api/2024-10/products.json" ) + def test_parse_response_success(shopify_client, mocker): mock_response = mocker.Mock(status_code=200, json=lambda: {"key": "value"}) parsed = shopify_client.parse_response(mock_response) assert parsed == {"key": "value"} + def test_parse_response_error(shopify_client, mocker): mock_response = mocker.Mock(status_code=404, text="Not Found") - mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError("Not Found") - + mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError( + "Not Found" + ) + with pytest.raises(requests.exceptions.HTTPError): shopify_client.parse_response(mock_response) diff --git a/tests/test_retry.py b/tests/test_retry.py new file mode 100644 index 0000000..229f569 --- /dev/null +++ b/tests/test_retry.py @@ -0,0 +1,174 @@ +import pytest +import urllib3 +import requests +from unittest.mock import MagicMock, patch + + +def test_gets_response_after_retry_on_connection_error(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.side_effect = [ + urllib3.exceptions.ConnectionError("Connection error"), + urllib3.exceptions.ConnectionError("Connection error"), + urllib3.exceptions.ConnectionError("Connection error"), + MagicMock( + status=200, + msg="OK", + stream=lambda *a, **kw: iter([b'{"result": "success"}']), + ), + ] + + response = shopify_client.request("GET", "products.json") + + assert response.status_code == 200 + assert response.json() == {"result": "success"} + assert mock_request.call_count == 4 + + +def test_fails_after_retry_on_connection_error(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.side_effect = urllib3.exceptions.ConnectionError( + "Connection error" + ) + + with pytest.raises(requests.exceptions.ConnectionError): + shopify_client.request("GET", "products.json") + + assert mock_request.call_count == 4 + + +def test_gets_response_after_retry_on_read_timeout(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.side_effect = [ + urllib3.exceptions.ReadTimeoutError(None, None, "Read timed out"), + urllib3.exceptions.ReadTimeoutError(None, None, "Read timed out"), + urllib3.exceptions.ReadTimeoutError(None, None, "Read timed out"), + MagicMock( + status=200, + msg="OK", + stream=lambda *a, **kw: iter([b'{"result": "success"}']), + ), + ] + + response = shopify_client.request("GET", "products.json") + + assert response.status_code == 200 + assert response.json() == {"result": "success"} + assert mock_request.call_count == 4 + + +def test_fails_after_retry_on_read_timeout(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.side_effect = urllib3.exceptions.ReadTimeoutError( + None, None, "Read timed out" + ) + + with pytest.raises(requests.exceptions.ConnectionError): + shopify_client.request("GET", "products.json") + + assert mock_request.call_count == 4 + + +def test_gets_response_after_retry_on_status_code(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + # Simulate 3 retries with 500, 502, and 503, then success + mock_request.side_effect = [ + MagicMock( + status=500, + msg="Internal Server Error", + headers={}, + ), + MagicMock(status=502, msg="Bad Gateway", headers={}), + MagicMock(status=503, msg="Service Unavailable", headers={}), + MagicMock( + status=200, + msg="OK", + stream=lambda *a, **kw: iter([b'{"result": "success"}']), + headers={}, + ), + ] + + response = shopify_client.request("GET", "products.json") + + assert response.status_code == 200 + assert response.json() == {"result": "success"} + assert mock_request.call_count == 4 + + +def test_fails_after_retry_on_status_code(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.return_value = MagicMock( + status=500, + msg="Internal Server Error", + headers={}, + stream=lambda *a, **kw: iter([b""]), + ) + + with pytest.raises(requests.exceptions.RetryError): + shopify_client.request("GET", "products.json") + + # Not sure why in test in considers "total" retries, but not just "status" as it should. + # But it works as expected when testing manually. + # assert mock_request.call_count == 5 + assert mock_request.call_count == 11 + + +def test_no_retry_on_non_retryable_status(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.return_value = MagicMock( + status=400, msg="Bad Request", stream=lambda *a, **kw: iter([b""]) + ) + + response = shopify_client.request("GET", "products.json") + + assert response.status_code == 400 + assert mock_request.call_count == 1 # No retries for status 400 + + +def test_respect_retry_after_header(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.side_effect = [ + MagicMock( + status=429, msg="Too Many Requests", headers={"Retry-After": "1"} + ), + MagicMock( + status=200, + msg="OK", + stream=lambda *a, **kw: iter([b'{"result": "success"}']), + ), + ] + + response = shopify_client.request("GET", "products.json") + + assert response.status_code == 200 + assert response.json() == {"result": "success"} + assert mock_request.call_count == 2 + + +def test_max_retries_exceeded(shopify_client): + with patch( + "urllib3.connectionpool.HTTPConnectionPool._make_request" + ) as mock_request: + mock_request.side_effect = urllib3.exceptions.ConnectionError( + "Connection failed" + ) + + with pytest.raises(requests.exceptions.ConnectionError): + shopify_client.request("GET", "products.json") + + assert mock_request.call_count == 4 From 66f701e5623eb889e9720f1a3b8b7f0267c4b52d Mon Sep 17 00:00:00 2001 From: Anton Shutik Date: Fri, 15 Nov 2024 12:45:06 +0100 Subject: [PATCH 3/4] Removed hooks --- shopify_client/__init__.py | 2 -- shopify_client/hooks.py | 18 ------------- tests/test_hooks.py | 52 -------------------------------------- 3 files changed, 72 deletions(-) delete mode 100644 shopify_client/hooks.py delete mode 100644 tests/test_hooks.py diff --git a/shopify_client/__init__.py b/shopify_client/__init__.py index 97dc9b7..c960a32 100644 --- a/shopify_client/__init__.py +++ b/shopify_client/__init__.py @@ -6,8 +6,6 @@ from requests.adapters import HTTPAdapter from urllib3 import Retry -from shopify_client.hooks import rate_limit - from .endpoint import DraftOrdersEndpoint, Endpoint, OrdersEndpoint from .graphql import GraphQL diff --git a/shopify_client/hooks.py b/shopify_client/hooks.py deleted file mode 100644 index 2671690..0000000 --- a/shopify_client/hooks.py +++ /dev/null @@ -1,18 +0,0 @@ -import time - - -def rate_limit(response, *args, **kwargs): - max_retry_count = 5 - retry_count = int(response.request.headers.get("X-Retry-Count", 0)) - print(f"Response: {response}") - - if response.status_code == 429 and retry_count < max_retry_count: - retry_after = int(response.headers.get("retry-after", 4)) - # print(f"Retry after: {retry_after}") - time.sleep(retry_after) - response.request.headers["X-Retry-Count"] = retry_count + 1 - new_response = response.connection.send(response.request) - new_response.history.append(response) - return rate_limit(new_response, *args, **kwargs) - - return response \ No newline at end of file diff --git a/tests/test_hooks.py b/tests/test_hooks.py deleted file mode 100644 index d9ad769..0000000 --- a/tests/test_hooks.py +++ /dev/null @@ -1,52 +0,0 @@ -from unittest.mock import MagicMock -import requests - -from shopify_client.hooks import rate_limit -import pytest - -@pytest.fixture -def mock_connection(): - """Fixture to create a mock connection object.""" - return MagicMock() - -@pytest.fixture -def initial_response(mock_connection): - """Fixture to create an initial response object.""" - response = requests.Response() - response.status_code = 429 - response.headers = {"retry-after": "2"} - response.request = requests.Request("GET", "https://api.example.com/resource").prepare() - response.request.headers["X-Retry-Count"] = 0 - response.connection = mock_connection - return response - -def test_rate_limit_retries_on_429(monkeypatch, initial_response, mock_connection): - """Test that the rate_limit function retries on a 429 response.""" - new_response = requests.Response() - new_response.status_code = 200 # Successful response to stop the retry loop - new_response.request = initial_response.request - mock_connection.send.return_value = new_response - - monkeypatch.setattr("time.sleep", lambda x: None) - - final_response = rate_limit(initial_response) - - assert final_response.status_code == 200 - assert mock_connection.send.call_count == 1 - assert final_response.request.headers["X-Retry-Count"] == 1 - -def test_rate_limit_stops_after_max_retries(monkeypatch, initial_response, mock_connection): - failed_response = requests.Response() - failed_response.status_code = 429 - failed_response.request = initial_response.request - failed_response.connection = mock_connection - - mock_connection.send.return_value = failed_response - - monkeypatch.setattr("time.sleep", lambda x: None) - - final_response = rate_limit(initial_response) - - assert final_response.status_code == 429 - assert mock_connection.send.call_count == 5 - assert final_response.request.headers["X-Retry-Count"] == 5 \ No newline at end of file From f5372181e6fd2fd5462290910ffc930891e50454 Mon Sep 17 00:00:00 2001 From: Anton Shutik Date: Fri, 15 Nov 2024 12:48:26 +0100 Subject: [PATCH 4/4] v.0.0.6 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index bbdeab6..1750564 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.0.5 +0.0.6