diff --git a/course_discovery/apps/course_metadata/tests/test_utils.py b/course_discovery/apps/course_metadata/tests/test_utils.py index 7ce9161016..235535f17d 100644 --- a/course_discovery/apps/course_metadata/tests/test_utils.py +++ b/course_discovery/apps/course_metadata/tests/test_utils.py @@ -11,10 +11,12 @@ import responses from django.conf import settings from django.core.exceptions import ValidationError +from django.http.response import HttpResponse from django.test import TestCase from edx_django_utils.cache import RequestCache from edx_toggles.toggles.testutils import override_waffle_switch from slugify import slugify +from slumber.exceptions import HttpClientError from course_discovery.apps.api.tests.mixins import SiteMixin from course_discovery.apps.api.v1.tests.test_views.mixins import OAuth2Mixin @@ -43,8 +45,8 @@ from course_discovery.apps.course_metadata.utils import ( calculated_seat_upgrade_deadline, clean_html, convert_svg_to_png_from_url, create_missing_entitlement, download_and_save_course_image, download_and_save_program_image, ensure_draft_world, fetch_getsmarter_products, - generate_sku, is_google_drive_url, serialize_entitlement_for_ecommerce_api, serialize_seat_for_ecommerce_api, - transform_skills_data, validate_slug_format + generate_sku, is_fatal_error, is_google_drive_url, serialize_entitlement_for_ecommerce_api, + serialize_seat_for_ecommerce_api, transform_skills_data, validate_slug_format ) @@ -1161,6 +1163,59 @@ def tearDown(self): responses.reset() super().tearDown() + def test_is_fatal_code(self): + response_with_200 = HttpResponse(status=200) + response_with_400 = HttpResponse(status=400) + response_with_429 = HttpResponse(status=429) + response_with_504 = HttpResponse(status=504) + assert not is_fatal_error(HttpClientError(response=response_with_200)) + assert is_fatal_error(HttpClientError(response=response_with_400)) + assert not is_fatal_error(HttpClientError(response=response_with_429)) + assert not is_fatal_error(HttpClientError(response=response_with_504)) + + def test_is_fatal_code_error(self): + # Success codes (2xx) should not be fatal error + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=201))) + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=204))) + + # Redirect codes (3xx) should not be fatal error + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=301))) + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=302))) + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=307))) + + # Retryable client error (429 Too Many Requests) already covered + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=429))) + + # Retryable server errors (5xx) should not be fatal + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=500))) + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=502))) + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=503))) + assert not is_fatal_error(HttpClientError(response=HttpResponse(status=504))) + + def test_is_fatal_code_positive(self): + # Client errors (4xx) that should be fatal + assert is_fatal_error(HttpClientError(response=HttpResponse(status=400))) # Bad Request + assert is_fatal_error(HttpClientError(response=HttpResponse(status=401))) # Unauthorized + assert is_fatal_error(HttpClientError(response=HttpResponse(status=403))) # Forbidden + assert is_fatal_error(HttpClientError(response=HttpResponse(status=404))) # Not Found + + # Other 4xx codes + assert is_fatal_error(HttpClientError(response=HttpResponse(status=410))) # Gone + assert is_fatal_error(HttpClientError(response=HttpResponse(status=422))) # Unprocessable Entity + + def test_is_fatal_code_additional(self): + # More client errors (4xx) that should be fatal + assert is_fatal_error(HttpClientError(response=HttpResponse(status=405))) # Method Not Allowed + assert is_fatal_error(HttpClientError(response=HttpResponse(status=406))) # Not Acceptable + assert is_fatal_error(HttpClientError(response=HttpResponse(status=407))) # Proxy Authentication Required + assert is_fatal_error(HttpClientError(response=HttpResponse(status=408))) # Request Timeout + assert is_fatal_error(HttpClientError(response=HttpResponse(status=409))) # Conflict + + # Validation-related errors + assert is_fatal_error(HttpClientError(response=HttpResponse(status=415))) # Unsupported Media Type + assert is_fatal_error(HttpClientError(response=HttpResponse(status=417))) # Expectation Failed + assert is_fatal_error(HttpClientError(response=HttpResponse(status=451))) # Unavailable For Legal Reason + def mock_product_api_call(self): """ Mock product api with success response. diff --git a/course_discovery/apps/course_metadata/utils.py b/course_discovery/apps/course_metadata/utils.py index 2f3fe88e61..118fbff086 100644 --- a/course_discovery/apps/course_metadata/utils.py +++ b/course_discovery/apps/course_metadata/utils.py @@ -8,6 +8,7 @@ from tempfile import NamedTemporaryFile from urllib.parse import urljoin, urlparse +import backoff import html2text import jsonschema import markdown @@ -73,6 +74,26 @@ def clean_query(query): return query +def is_fatal_error(ex): + """ + Return True if the exception represents a fatal client error. + + Fatal means: + - The response exists + - The status code is a 4XX client error (400–499) + - The error is not a 429 (rate limiting) + """ + response = ex.response + if response is None: + return False + + code = response.status_code + if code == 429: + return False + + return 400 <= code < 500 + + def set_official_state(obj, model, attrs=None): """ Given a draft object and the model of that object, ensure that an official version is created @@ -986,6 +1007,19 @@ def transform_skills_data(skills_data): return skills +# The courses endpoint has 40 requests/minute rate limit. +# This will back off at a rate of 60/120/240 seconds (from the factor 60 and default value of base 2). +# This backoff code can still fail because of the concurrent requests all requesting at the same time. +# So even in the case of entering into the next minute, if we still exceed our limit for that min, +# any requests that failed in both limits are still approaching their max_tries limit. +@backoff.on_exception( + backoff.expo, + (requests.exceptions.Timeout, requests.exceptions.HTTPError), + factor=60, + max_tries=4, + giveup=is_fatal_error, + max_time=300 +) def fetch_getsmarter_products(): """ Returns the products details from the getsmarter API """ products = []