From 36e3d8fb235e24e057535a13b8cad72d10ee7199 Mon Sep 17 00:00:00 2001 From: Thibault Coudray Date: Mon, 21 Oct 2024 16:27:38 +0200 Subject: [PATCH] (BSR)[API] feat: add `catch_cinema_provider_request_timeout` decorator --- .../core/external_bookings/decorators.py | 91 ++++++++++++ .../core/external_bookings/exceptions.py | 4 + .../core/external_bookings/test_decorator.py | 139 ++++++++++++++++++ 3 files changed, 234 insertions(+) create mode 100644 api/src/pcapi/core/external_bookings/decorators.py create mode 100644 api/tests/core/external_bookings/test_decorator.py diff --git a/api/src/pcapi/core/external_bookings/decorators.py b/api/src/pcapi/core/external_bookings/decorators.py new file mode 100644 index 00000000000..4b973d4258c --- /dev/null +++ b/api/src/pcapi/core/external_bookings/decorators.py @@ -0,0 +1,91 @@ +import functools +import inspect +import logging +from typing import Any +from typing import Callable +from typing import TypeVar + +from pcapi.utils.requests import exceptions as requests_exception + +from . import exceptions +from . import models + + +logger = logging.getLogger(__name__) + + +class ExternalBookingDecoratorException(Exception): + pass + + +def _extract_logging_information(client_func: Callable, *args: list[Any], **kwargs: Any) -> dict: + """ + Return a dictionary to be passed to the `extra` parameter of the logging function. + + The returned dictionary contains the following keys: + - "cinema_id" (int): The ID of the cinema from the pivot table. + - "client" (str): The client used for the request (e.g., `EMSClientAPI`). + - "method" (str): The instance method used for the request (e.g., `book_ticket`). + - "method_params" (dict): The parameters passed to the method. Example: + { + "show_id": "12345", # ID of the show + "booking": "", # Stringified Booking instance + "beneficiary": "", # Stringified User instance + } + """ + self = args[0] + assert isinstance(self, models.ExternalBookingsClientAPI) # to make mypy happy + # as client_func is an instance method of `ExternalBookingsClientAPI`, self should have a `cinema_id` + cinema_id = self.cinema_id + + func_parameters = [param.name for param in inspect.signature(client_func).parameters.values()] + method_params = {} + + arg_index = 1 # as the first argument is `self`, start from index 1 + for arg_value in args[1:]: + arg_name = func_parameters[arg_index] + method_params[arg_name] = str(arg_value) + arg_index += 1 + + for k, v in kwargs.items(): + method_params[k] = str(v) + + return { + "client": self.__class__.__name__, # class name + "method": client_func.__name__, # method name + "cinema_id": cinema_id, + "method_params": method_params, + } + + +def _check_wrapped_function_is_an_instance_method_of_external_booking_client_api(args: list) -> None: + if len(args) == 0 or not isinstance(args[0], models.ExternalBookingsClientAPI): + raise ExternalBookingDecoratorException( + "`catch_request_timeout` can only be applied to an instance method of a class inheriting from `ExternalBookingsClientAPI`" + ) + + +# typing to ensure that mypy understands the wrapped function signature has not changed +F = TypeVar("F", bound=Callable[..., Any]) + + +def catch_cinema_provider_request_timeout(client_func: F) -> F: + """ + Decorator that catches HTTP Timeout errors, logs them and then raises `ExternalBookingTimeoutException`. + This decorator can only be applied to instance methods of classes inheriting from `ExternalBookingsClientAPI` + """ + + @functools.wraps(client_func) + def wrapped_func(*args, **kwargs): # type: ignore[no-untyped-def] + _check_wrapped_function_is_an_instance_method_of_external_booking_client_api(args) + try: + return client_func(*args, **kwargs) + except (requests_exception.Timeout, requests_exception.ReadTimeout) as e: + extra = _extract_logging_information(client_func, *args, **kwargs) + logger.error( + "Cinema Provider API Request Timeout", + extra=dict(extra, request={"method": e.request.method, "url": e.request.url}), + ) + raise exceptions.ExternalBookingTimeoutException() + + return wrapped_func # type: ignore[return-value] diff --git a/api/src/pcapi/core/external_bookings/exceptions.py b/api/src/pcapi/core/external_bookings/exceptions.py index 554d18d19f1..82339a2ef6a 100644 --- a/api/src/pcapi/core/external_bookings/exceptions.py +++ b/api/src/pcapi/core/external_bookings/exceptions.py @@ -2,6 +2,10 @@ class ExternalBookingException(Exception): pass +class ExternalBookingTimeoutException(Exception): + pass + + class ExternalBookingSoldOutError(Exception): pass diff --git a/api/tests/core/external_bookings/test_decorator.py b/api/tests/core/external_bookings/test_decorator.py new file mode 100644 index 00000000000..40bd08e5a40 --- /dev/null +++ b/api/tests/core/external_bookings/test_decorator.py @@ -0,0 +1,139 @@ +import logging +from unittest.mock import Mock + +import pytest +from requests.exceptions import ReadTimeout +from requests.exceptions import Timeout + +from pcapi.core.bookings import models as bookings_models +from pcapi.core.external_bookings import models as external_bookings_models +from pcapi.core.external_bookings.decorators import ExternalBookingDecoratorException +from pcapi.core.external_bookings.decorators import catch_cinema_provider_request_timeout +from pcapi.core.external_bookings.exceptions import ExternalBookingTimeoutException +from pcapi.core.users import models as users_models + + +class FakeExternalBookingClientAPI(external_bookings_models.ExternalBookingsClientAPI): + def __init__(self, cinema_id: str, connector) -> None: + self.cinema_id = cinema_id + self.connector = connector + + @catch_cinema_provider_request_timeout + def get_film_showtimes_stocks(self, film_id): + return self.connector.make_request(film_id) + + @catch_cinema_provider_request_timeout + def book_ticket(self, show_id, booking, beneficiary): + return self.connector.make_request(show_id, booking, beneficiary) + + +class FakeClass: + @catch_cinema_provider_request_timeout + def function_that_cannot_be_decorated(self, a): + pass + + +@catch_cinema_provider_request_timeout +def another_func_that_cannot_be_decorated(): + pass + + +class CatchCinemaProviderRequestTimeoutTest: + def test_should_raise_error_because_wrapped_function_is_not_an_instance_method(self): + fake_class = FakeClass() + + with pytest.raises(ExternalBookingDecoratorException): + fake_class.function_that_cannot_be_decorated("coucou") + + with pytest.raises(ExternalBookingDecoratorException): + another_func_that_cannot_be_decorated() + + def test_should_raise_understandable_error_if_the_decorated_func_is_called_with_incorrect_params(self): + client = FakeExternalBookingClientAPI(cinema_id=1, connector=Mock()) + + with pytest.raises(TypeError) as exception: + client.get_film_showtimes_stocks(12, show_id=1) + assert ( + str(exception.value) + == "TypeError: get_film_showtimes_stocks() got an unexpected keyword argument 'show_id'" + ) + + with pytest.raises(TypeError) as exception: + client.get_film_showtimes_stocks(12, 13, 24) + assert ( + str(exception.value) + == "TypeError: get_film_showtimes_stocks() takes 1 positional arguments but 3 were given" + ) + + @pytest.mark.parametrize( + "args_list,kwargs_dict,exception_class,request_mock,expected_extra", + [ + ( + [12345], + {"booking": bookings_models.Booking(id=1), "beneficiary": users_models.User(id=3)}, + ReadTimeout, + Mock(url="https://unproviderado.re/mais/dont/lurl/est/un/peu/flaky", method="POST"), + { + "cinema_id": 789, + "method": "book_ticket", + "client": "FakeExternalBookingClientAPI", + "method_params": { + "show_id": "12345", + "booking": "", + "beneficiary": "", + }, + "request": {"url": "https://unproviderado.re/mais/dont/lurl/est/un/peu/flaky", "method": "POST"}, + }, + ), + ( + [678, bookings_models.Booking(id=1), users_models.User(id=3)], + {}, + Timeout, + Mock(url="https://matthieu.reve/de/les/débrancher", method="GET"), + { + "cinema_id": 789, + "method": "book_ticket", + "client": "FakeExternalBookingClientAPI", + "method_params": { + "show_id": "678", + "booking": "", + "beneficiary": "", + }, + "request": {"url": "https://matthieu.reve/de/les/débrancher", "method": "GET"}, + }, + ), + ( + [], + { + "show_id": 562, + "booking": bookings_models.Booking(id=4567), + "beneficiary": users_models.User(id=12345767), + }, + ReadTimeout, + Mock(url="https://moi.aussi/faut/bien/admettre", method="PUT"), + { + "cinema_id": 789, + "method": "book_ticket", + "client": "FakeExternalBookingClientAPI", + "cinema_id": 789, + "method_params": { + "show_id": "562", + "booking": "", + "beneficiary": "", + }, + "request": {"url": "https://moi.aussi/faut/bien/admettre", "method": "PUT"}, + }, + ), + ], + ) + def test_should_log_error(self, args_list, kwargs_dict, exception_class, request_mock, expected_extra, caplog): + connector = Mock() + connector.make_request.side_effect = exception_class(request=request_mock) + client = FakeExternalBookingClientAPI(cinema_id=789, connector=connector) + + with caplog.at_level(logging.ERROR): + with pytest.raises(ExternalBookingTimeoutException): + client.book_ticket(*args_list, **kwargs_dict) + + assert caplog.records[0].extra == expected_extra + assert caplog.messages[0] == "Cinema Provider API Request Timeout"