From 9990de405296bdefb50976e4b60af36564e71378 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 6 Dec 2023 13:42:20 +0100 Subject: [PATCH 01/13] drafts --- .../services/healthchecks.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/services/payments/src/simcore_service_payments/services/healthchecks.py b/services/payments/src/simcore_service_payments/services/healthchecks.py index 98774700f44..680f2232917 100644 --- a/services/payments/src/simcore_service_payments/services/healthchecks.py +++ b/services/payments/src/simcore_service_payments/services/healthchecks.py @@ -1,6 +1,7 @@ import asyncio import logging +from fastapi import FastAPI from models_library.healthchecks import LivenessResult from sqlalchemy.ext.asyncio import AsyncEngine @@ -25,3 +26,32 @@ async def create_health_report( "resource_usage_tracker": rut_liveness, "postgres": db_liveness, } + + +async def _monitor_liveness(): + # + # + # logs with specific format so graylog can send alarm if found + # + # + raise NotImplementedError + + +async def _periodic(): + while True: + # do something + await _monitor_liveness() + # what if fails?, wait&repeat or stop-forever or cleanup&restart ? + + +def setup_healthchecks(app: FastAPI): + # setup _monitor_liveness as a periodic task in only one of the replicas + + async def _on_startup() -> None: + ... + + async def _on_shutdown() -> None: + ... + + app.add_event_handler("startup", _on_startup) + app.add_event_handler("shutdown", _on_shutdown) From 6e3e35d73159abf853fa0c57bd08fdee05e09c2c Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Fri, 8 Dec 2023 18:33:04 +0100 Subject: [PATCH 02/13] minor --- .../payments/src/simcore_service_payments/core/settings.py | 5 ++++- .../payments/src/simcore_service_payments/models/utils.py | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/services/payments/src/simcore_service_payments/core/settings.py b/services/payments/src/simcore_service_payments/core/settings.py index d8b89accb2e..9ae4b8a0793 100644 --- a/services/payments/src/simcore_service_payments/core/settings.py +++ b/services/payments/src/simcore_service_payments/core/settings.py @@ -53,7 +53,10 @@ class ApplicationSettings(_BaseApplicationSettings): """ PAYMENTS_GATEWAY_URL: HttpUrl = Field( - ..., description="Base url to the payment gateway" + ..., + description="Base url to the payment gateway." + "Used for both internal communication and " + "to get an external link to the gateway for the payment form", ) PAYMENTS_GATEWAY_API_SECRET: SecretStr = Field( diff --git a/services/payments/src/simcore_service_payments/models/utils.py b/services/payments/src/simcore_service_payments/models/utils.py index af5cbfe537e..d99736a07e8 100644 --- a/services/payments/src/simcore_service_payments/models/utils.py +++ b/services/payments/src/simcore_service_payments/models/utils.py @@ -6,6 +6,7 @@ def merge_models(got: GetPaymentMethod, acked: PaymentsMethodsDB) -> PaymentMethodGet: assert acked.completed_at # nosec + assert got.id == acked.payment_method_id # nosec return PaymentMethodGet( idr=acked.payment_method_id, From 43131d6140f7219d6296bebb08e644e6f8438d00 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Fri, 8 Dec 2023 18:33:30 +0100 Subject: [PATCH 03/13] retry at startup --- .../services/payments_gateway.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/services/payments/src/simcore_service_payments/services/payments_gateway.py b/services/payments/src/simcore_service_payments/services/payments_gateway.py index e6dbc126f40..0cacf5c07ce 100644 --- a/services/payments/src/simcore_service_payments/services/payments_gateway.py +++ b/services/payments/src/simcore_service_payments/services/payments_gateway.py @@ -28,7 +28,11 @@ from simcore_service_payments.models.schemas.acknowledgements import ( AckPaymentWithPaymentMethod, ) +from tenacity import AsyncRetrying, stop_after_delay, wait_exponential +from tenacity.retry import retry_if_exception_type +from tenacity.wait import wait_exponential +from ..core.errors import PaymentGatewayNotReadyError from ..core.settings import ApplicationSettings from ..models.payments_gateway import ( BatchGetPaymentMethods, @@ -216,3 +220,22 @@ def setup_payments_gateway(app: FastAPI): ) api.attach_lifespan_to(app) api.set_to_app_state(app) + + # NOTE: start policy: + # - this service will not be able to start if payments-gateway is alive + # + async def _check_service_liveness() -> None: + results = [] + async for attempt in AsyncRetrying( + wait=wait_exponential(max=2), + stop=stop_after_delay(max_delay=5), + retry=retry_if_exception_type(PaymentGatewayNotReadyError), + reraise=True, + ): + with attempt: + alive = await api.check_liveness() + results.append(alive) + if not alive: + raise PaymentGatewayNotReadyError(checks=results) + + app.add_event_handler("startup", _check_service_liveness) From 2f860d783c2872770e7cfee5af3943c880697fcd Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Sat, 9 Dec 2023 21:00:43 +0100 Subject: [PATCH 04/13] note --- .../simcore_service_payments/services/payments_methods.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/payments/src/simcore_service_payments/services/payments_methods.py b/services/payments/src/simcore_service_payments/services/payments_methods.py index 63a117e58ef..5d3ca24c737 100644 --- a/services/payments/src/simcore_service_payments/services/payments_methods.py +++ b/services/payments/src/simcore_service_payments/services/payments_methods.py @@ -172,9 +172,12 @@ async def list_payment_methods( [acked.payment_method_id for acked in acked_many] ) + # FIXME: if out-of-sync w/ gateway, then this code will raise! because it has strict=True! + # FIXME: is order correct? is one-to-one ? is order preserved? + return [ merge_models(got, acked) - for acked, got in zip(acked_many, got_many, strict=True) + for got, acked in zip(got_many, acked_many, strict=True) ] From c3801bba37814ad4f7b04976e69b9268055400be Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 10:18:42 +0100 Subject: [PATCH 05/13] adds healthcheck in mock --- .../services/payments_gateway.py | 43 +++++++++++-------- services/payments/tests/unit/conftest.py | 10 +++-- .../tests/unit/test_rpc_payments_methods.py | 5 ++- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/services/payments/src/simcore_service_payments/services/payments_gateway.py b/services/payments/src/simcore_service_payments/services/payments_gateway.py index 0cacf5c07ce..2dfb76b2024 100644 --- a/services/payments/src/simcore_service_payments/services/payments_gateway.py +++ b/services/payments/src/simcore_service_payments/services/payments_gateway.py @@ -10,6 +10,7 @@ import logging from collections.abc import Callable from contextlib import suppress +from typing import Coroutine import httpx from fastapi import FastAPI @@ -32,7 +33,7 @@ from tenacity.retry import retry_if_exception_type from tenacity.wait import wait_exponential -from ..core.errors import PaymentGatewayNotReadyError +from ..core.errors import PaymentsGatewayNotReadyError from ..core.settings import ApplicationSettings from ..models.payments_gateway import ( BatchGetPaymentMethods, @@ -206,6 +207,27 @@ async def pay_with_payment_method( return AckPaymentWithPaymentMethod.parse_obj(response.json()) +def _create_start_policy(api: PaymentsGatewayApi) -> Callable[[], Coroutine]: + # Start policy: + # - this service will not be able to start if payments-gateway is alive + # + async def _(): + results = [] + async for attempt in AsyncRetrying( + wait=wait_exponential(max=3), + stop=stop_after_delay(max_delay=6), + retry=retry_if_exception_type(PaymentsGatewayNotReadyError), + reraise=True, + ): + with attempt: + alive = await api.check_liveness() + results.append(alive) + if not alive: + raise PaymentsGatewayNotReadyError(checks=results) + + return _ + + def setup_payments_gateway(app: FastAPI): assert app.state # nosec settings: ApplicationSettings = app.state.settings @@ -221,21 +243,4 @@ def setup_payments_gateway(app: FastAPI): api.attach_lifespan_to(app) api.set_to_app_state(app) - # NOTE: start policy: - # - this service will not be able to start if payments-gateway is alive - # - async def _check_service_liveness() -> None: - results = [] - async for attempt in AsyncRetrying( - wait=wait_exponential(max=2), - stop=stop_after_delay(max_delay=5), - retry=retry_if_exception_type(PaymentGatewayNotReadyError), - reraise=True, - ): - with attempt: - alive = await api.check_liveness() - results.append(alive) - if not alive: - raise PaymentGatewayNotReadyError(checks=results) - - app.add_event_handler("startup", _check_service_liveness) + app.add_event_handler("startup", _create_start_policy(api)) diff --git a/services/payments/tests/unit/conftest.py b/services/payments/tests/unit/conftest.py index 23c67c3a64c..8f47431e466 100644 --- a/services/payments/tests/unit/conftest.py +++ b/services/payments/tests/unit/conftest.py @@ -190,15 +190,16 @@ async def app( @pytest.fixture -def mock_payments_gateway_service_api_base(app: FastAPI) -> Iterator[MockRouter]: +def mock_payments_gateway_service_api_base( + app_environment: EnvVarsDict, +) -> Iterator[MockRouter]: """ If external_environment is present, then this mock is not really used and instead the test runs against some real services """ - settings: ApplicationSettings = app.state.settings with respx.mock( - base_url=settings.PAYMENTS_GATEWAY_URL, + base_url=app_environment["PAYMENTS_GATEWAY_URL"], assert_all_called=False, assert_all_mocked=True, # IMPORTANT: KEEP always True! ) as respx_mock: @@ -422,6 +423,9 @@ def mock_payments_gateway_service_or_none( return None # OR tests against mock payments-gateway + mock_payments_gateway_service_api_base.get("/", name="healthcheck").mock( + return_value=httpx.Response(status_code=status.HTTP_200_OK, text="OK") + ) mock_payments_routes(mock_payments_gateway_service_api_base) mock_payments_methods_routes(mock_payments_gateway_service_api_base) return mock_payments_gateway_service_api_base diff --git a/services/payments/tests/unit/test_rpc_payments_methods.py b/services/payments/tests/unit/test_rpc_payments_methods.py index 61816ef633f..8871444eec0 100644 --- a/services/payments/tests/unit/test_rpc_payments_methods.py +++ b/services/payments/tests/unit/test_rpc_payments_methods.py @@ -52,7 +52,8 @@ def app_environment( postgres_env_vars_dict: EnvVarsDict, wait_for_postgres_ready_and_db_migrated: None, external_environment: EnvVarsDict, -): +) -> EnvVarsDict: + # set environs monkeypatch.delenv("PAYMENTS_RABBITMQ", raising=False) monkeypatch.delenv("PAYMENTS_POSTGRES", raising=False) @@ -71,9 +72,9 @@ def app_environment( async def test_webserver_init_and_cancel_payment_method_workflow( is_pdb_enabled: bool, + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, rpc_client: RabbitMQRPCClient, - mock_payments_gateway_service_or_none: MockRouter | None, user_id: UserID, user_name: IDStr, user_email: EmailStr, From e335568a063f6bb7f8e9e874fb10e142578f4821 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 11:22:15 +0100 Subject: [PATCH 06/13] fixes tests --- .../api/test__one_time_payment_workflows.py | 2 +- .../api/test__payment_method_workflows.py | 2 +- services/payments/tests/unit/conftest.py | 32 +++++++++++++++---- .../unit/test_db_payments_methods_repo.py | 1 + .../test_db_payments_transactions_repo.py | 1 + .../tests/unit/test_db_payments_users_repo.py | 1 + .../payments/tests/unit/test_rpc_payments.py | 13 +++++++- .../tests/unit/test_rpc_payments_methods.py | 6 ++-- .../test_services_auto_recharge_listener.py | 4 +-- .../tests/unit/test_services_payments.py | 2 +- .../unit/test_services_payments_gateway.py | 2 +- .../test_services_resource_usage_tracker.py | 2 +- 12 files changed, 50 insertions(+), 18 deletions(-) diff --git a/services/payments/tests/unit/api/test__one_time_payment_workflows.py b/services/payments/tests/unit/api/test__one_time_payment_workflows.py index 57ca6594e94..69fccffea15 100644 --- a/services/payments/tests/unit/api/test__one_time_payment_workflows.py +++ b/services/payments/tests/unit/api/test__one_time_payment_workflows.py @@ -60,11 +60,11 @@ def app_environment( ) async def test_successful_one_time_payment_workflow( is_pdb_enabled: bool, + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, client: httpx.AsyncClient, faker: Faker, rpc_client: RabbitMQRPCClient, - mock_payments_gateway_service_or_none: MockRouter | None, wallet_id: WalletID, wallet_name: IDStr, user_id: UserID, diff --git a/services/payments/tests/unit/api/test__payment_method_workflows.py b/services/payments/tests/unit/api/test__payment_method_workflows.py index 15cedd186d9..440d5170221 100644 --- a/services/payments/tests/unit/api/test__payment_method_workflows.py +++ b/services/payments/tests/unit/api/test__payment_method_workflows.py @@ -63,11 +63,11 @@ def app_environment( ) async def test_successful_create_payment_method_workflow( is_pdb_enabled: bool, + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, client: httpx.AsyncClient, faker: Faker, rpc_client: RabbitMQRPCClient, - mock_payments_gateway_service_or_none: MockRouter | None, wallet_id: WalletID, wallet_name: IDStr, user_id: UserID, diff --git a/services/payments/tests/unit/conftest.py b/services/payments/tests/unit/conftest.py index 8f47431e466..74f71e98276 100644 --- a/services/payments/tests/unit/conftest.py +++ b/services/payments/tests/unit/conftest.py @@ -59,22 +59,40 @@ def disable_rabbitmq_and_rpc_setup(mocker: MockerFixture) -> Callable: def _(): # The following services are affected if rabbitmq is not in place - mocker.patch("simcore_service_payments.core.application.setup_notifier") - mocker.patch("simcore_service_payments.core.application.setup_socketio") - mocker.patch("simcore_service_payments.core.application.setup_rabbitmq") - mocker.patch("simcore_service_payments.core.application.setup_rpc_api_routes") mocker.patch( - "simcore_service_payments.core.application.setup_auto_recharge_listener" + "simcore_service_payments.core.application.setup_notifier", autospec=True + ) + mocker.patch( + "simcore_service_payments.core.application.setup_socketio", autospec=True + ) + mocker.patch( + "simcore_service_payments.core.application.setup_rabbitmq", autospec=True + ) + mocker.patch( + "simcore_service_payments.core.application.setup_rpc_api_routes", + autospec=True, + ) + mocker.patch( + "simcore_service_payments.core.application.setup_auto_recharge_listener", + autospec=True, ) return _ @pytest.fixture -def with_disabled_rabbitmq_and_rpc(disable_rabbitmq_and_rpc_setup: Callable): +def with_disabled_rabbitmq_and_rpc(disable_rabbitmq_and_rpc_setup: Callable) -> None: disable_rabbitmq_and_rpc_setup() +@pytest.fixture +def with_disabled_gateway(mocker: MockerFixture) -> None: + mocker.patch( + "simcore_service_payments.core.application.setup_payments_gateway", + autospec=True, + ) + + @pytest.fixture async def rpc_client( faker: Faker, rabbitmq_rpc_client: Callable[[str], Awaitable[RabbitMQRPCClient]] @@ -470,7 +488,7 @@ def mock_resource_usage_tracker_service_api_base( @pytest.fixture -def mock_resoruce_usage_tracker_service_api( +def mock_resource_usage_tracker_service_api( faker: Faker, mock_resource_usage_tracker_service_api_base: MockRouter, rut_service_openapi_specs: dict[str, Any], diff --git a/services/payments/tests/unit/test_db_payments_methods_repo.py b/services/payments/tests/unit/test_db_payments_methods_repo.py index 76166c5d0be..e762c49a57c 100644 --- a/services/payments/tests/unit/test_db_payments_methods_repo.py +++ b/services/payments/tests/unit/test_db_payments_methods_repo.py @@ -26,6 +26,7 @@ def app_environment( app_environment: EnvVarsDict, postgres_env_vars_dict: EnvVarsDict, with_disabled_rabbitmq_and_rpc: None, + with_disabled_gateway: None, wait_for_postgres_ready_and_db_migrated: None, ): # set environs diff --git a/services/payments/tests/unit/test_db_payments_transactions_repo.py b/services/payments/tests/unit/test_db_payments_transactions_repo.py index ccab0fed110..47ea8101425 100644 --- a/services/payments/tests/unit/test_db_payments_transactions_repo.py +++ b/services/payments/tests/unit/test_db_payments_transactions_repo.py @@ -31,6 +31,7 @@ def app_environment( app_environment: EnvVarsDict, postgres_env_vars_dict: EnvVarsDict, with_disabled_rabbitmq_and_rpc: None, + with_disabled_gateway: None, wait_for_postgres_ready_and_db_migrated: None, ): # set environs diff --git a/services/payments/tests/unit/test_db_payments_users_repo.py b/services/payments/tests/unit/test_db_payments_users_repo.py index 82087105003..122420c598e 100644 --- a/services/payments/tests/unit/test_db_payments_users_repo.py +++ b/services/payments/tests/unit/test_db_payments_users_repo.py @@ -32,6 +32,7 @@ def app_environment( app_environment: EnvVarsDict, postgres_env_vars_dict: EnvVarsDict, with_disabled_rabbitmq_and_rpc: None, + with_disabled_gateway: None, wait_for_postgres_ready_and_db_migrated: None, ): # set environs diff --git a/services/payments/tests/unit/test_rpc_payments.py b/services/payments/tests/unit/test_rpc_payments.py index 62345b701e4..2804e2dc2aa 100644 --- a/services/payments/tests/unit/test_rpc_payments.py +++ b/services/payments/tests/unit/test_rpc_payments.py @@ -13,6 +13,7 @@ from models_library.api_schemas_webserver.wallets import WalletPaymentInitiated from models_library.rabbitmq_basic_types import RPCMethodName from pydantic import parse_obj_as +from pytest_mock import MockerFixture from pytest_simcore.helpers.typing_env import EnvVarsDict from pytest_simcore.helpers.utils_envs import setenvs_from_dict from respx import MockRouter @@ -68,7 +69,17 @@ def init_payment_kwargs(faker: Faker) -> dict[str, Any]: } +@pytest.fixture +def _disable_startup(mocker: MockerFixture): + mocker.patch( + "simcore_service_payments.services.payments_gateway._create_start_policy", + return_value=lambda: print("on-startup"), + ) + + async def test_rpc_init_payment_fail( + is_pdb_enabled: bool, + _disable_startup, app: FastAPI, rpc_client: RabbitMQRPCClient, init_payment_kwargs: dict[str, Any], @@ -81,6 +92,7 @@ async def test_rpc_init_payment_fail( PAYMENTS_RPC_NAMESPACE, parse_obj_as(RPCMethodName, "init_payment"), **init_payment_kwargs, + timeout_s=None if is_pdb_enabled else 5, ) error = exc_info.value @@ -95,7 +107,6 @@ async def test_webserver_one_time_payment_workflow( is_pdb_enabled: bool, app: FastAPI, rpc_client: RabbitMQRPCClient, - mock_payments_gateway_service_or_none: MockRouter | None, init_payment_kwargs: dict[str, Any], payments_clean_db: None, ): diff --git a/services/payments/tests/unit/test_rpc_payments_methods.py b/services/payments/tests/unit/test_rpc_payments_methods.py index 8871444eec0..efc6bdd997b 100644 --- a/services/payments/tests/unit/test_rpc_payments_methods.py +++ b/services/payments/tests/unit/test_rpc_payments_methods.py @@ -120,9 +120,9 @@ async def test_webserver_init_and_cancel_payment_method_workflow( async def test_webserver_crud_payment_method_workflow( is_pdb_enabled: bool, + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, rpc_client: RabbitMQRPCClient, - mock_payments_gateway_service_or_none: MockRouter | None, user_id: UserID, user_name: IDStr, user_email: EmailStr, @@ -201,10 +201,10 @@ async def test_webserver_crud_payment_method_workflow( async def test_webserver_pay_with_payment_method_workflow( is_pdb_enabled: bool, + mock_resource_usage_tracker_service_api: None, + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, rpc_client: RabbitMQRPCClient, - mock_resoruce_usage_tracker_service_api: None, - mock_payments_gateway_service_or_none: MockRouter | None, faker: Faker, product_name: ProductName, user_id: UserID, diff --git a/services/payments/tests/unit/test_services_auto_recharge_listener.py b/services/payments/tests/unit/test_services_auto_recharge_listener.py index bc54d8297e4..c6069fb5603 100644 --- a/services/payments/tests/unit/test_services_auto_recharge_listener.py +++ b/services/payments/tests/unit/test_services_auto_recharge_listener.py @@ -228,7 +228,7 @@ async def test_process_message__whole_autorecharge_flow_success( mocked_pay_with_payment_method: mock.AsyncMock, mock_rpc_server: RabbitMQRPCClient, mock_rpc_client: RabbitMQRPCClient, - mock_resoruce_usage_tracker_service_api: MockRouter, + mock_resource_usage_tracker_service_api: MockRouter, postgres_db: sa.engine.Engine, ): publisher = create_rabbitmq_client("publisher") @@ -241,7 +241,7 @@ async def test_process_message__whole_autorecharge_flow_success( assert row.wallet_id == wallet_id assert row.state == PaymentTransactionState.SUCCESS assert row.comment == "Payment generated by auto recharge" - assert len(mock_resoruce_usage_tracker_service_api.calls) == 1 + assert len(mock_resource_usage_tracker_service_api.calls) == 1 @pytest.mark.parametrize( diff --git a/services/payments/tests/unit/test_services_payments.py b/services/payments/tests/unit/test_services_payments.py index 5790b5f53fa..514941f7c4c 100644 --- a/services/payments/tests/unit/test_services_payments.py +++ b/services/payments/tests/unit/test_services_payments.py @@ -59,12 +59,12 @@ def app_environment( async def test_fails_to_pay_with_payment_method_without_funds( + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, create_fake_payment_method_in_db: Callable[ [PaymentMethodID, WalletID, UserID], Awaitable[PaymentsMethodsDB] ], no_funds_payment_method_id: PaymentMethodID, - mock_payments_gateway_service_or_none: MockRouter | None, wallet_id: WalletID, wallet_name: IDStr, user_id: UserID, diff --git a/services/payments/tests/unit/test_services_payments_gateway.py b/services/payments/tests/unit/test_services_payments_gateway.py index 698acba7b9d..4247737c88d 100644 --- a/services/payments/tests/unit/test_services_payments_gateway.py +++ b/services/payments/tests/unit/test_services_payments_gateway.py @@ -89,9 +89,9 @@ def amount_dollars(request: pytest.FixtureRequest) -> float: "https://github.com/ITISFoundation/osparc-simcore/pull/4715" ) async def test_one_time_payment_workflow( + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, faker: Faker, - mock_payments_gateway_service_or_none: MockRouter | None, amount_dollars: float, ): payment_gateway_api = PaymentsGatewayApi.get_from_app_state(app) diff --git a/services/payments/tests/unit/test_services_resource_usage_tracker.py b/services/payments/tests/unit/test_services_resource_usage_tracker.py index 868cdf65661..cc3b255c619 100644 --- a/services/payments/tests/unit/test_services_resource_usage_tracker.py +++ b/services/payments/tests/unit/test_services_resource_usage_tracker.py @@ -56,7 +56,7 @@ def app( async def test_add_credits_to_wallet( - app: FastAPI, faker: Faker, mock_resoruce_usage_tracker_service_api: MockRouter + app: FastAPI, faker: Faker, mock_resource_usage_tracker_service_api: MockRouter ): # test rut_api = ResourceUsageTrackerApi.get_from_app_state(app) From 24155e0c2891dc84c1f7ea765f0b70901f65638f Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 11:40:34 +0100 Subject: [PATCH 07/13] fixes rpc errors --- services/payments/tests/unit/test_rpc_payments.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/payments/tests/unit/test_rpc_payments.py b/services/payments/tests/unit/test_rpc_payments.py index 2804e2dc2aa..1d6e0132790 100644 --- a/services/payments/tests/unit/test_rpc_payments.py +++ b/services/payments/tests/unit/test_rpc_payments.py @@ -70,7 +70,7 @@ def init_payment_kwargs(faker: Faker) -> dict[str, Any]: @pytest.fixture -def _disable_startup(mocker: MockerFixture): +def _with_disabled_payments_gateway_startup(mocker: MockerFixture): mocker.patch( "simcore_service_payments.services.payments_gateway._create_start_policy", return_value=lambda: print("on-startup"), @@ -79,7 +79,7 @@ def _disable_startup(mocker: MockerFixture): async def test_rpc_init_payment_fail( is_pdb_enabled: bool, - _disable_startup, + _with_disabled_payments_gateway_startup: None, app: FastAPI, rpc_client: RabbitMQRPCClient, init_payment_kwargs: dict[str, Any], @@ -143,6 +143,8 @@ async def test_cancel_invalid_payment_id( app: FastAPI, rpc_client: RabbitMQRPCClient, mock_payments_gateway_service_or_none: MockRouter | None, + app: FastAPI, + rpc_client: RabbitMQRPCClient, init_payment_kwargs: dict[str, Any], faker: Faker, payments_clean_db: None, From 6a6d2c4a6070e6aa4ce71766f48b1b84f1209529 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 14:32:32 +0100 Subject: [PATCH 08/13] reraise errors in prc --- .../tests/unit/test_services_auto_recharge_listener.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/payments/tests/unit/test_services_auto_recharge_listener.py b/services/payments/tests/unit/test_services_auto_recharge_listener.py index c6069fb5603..e7f3a1dac09 100644 --- a/services/payments/tests/unit/test_services_auto_recharge_listener.py +++ b/services/payments/tests/unit/test_services_auto_recharge_listener.py @@ -4,10 +4,9 @@ # pylint: disable=unused-argument # pylint: disable=unused-variable -from collections.abc import Callable +from collections.abc import Awaitable, Callable, Iterator from datetime import datetime, timedelta, timezone from decimal import Decimal -from typing import Awaitable, Iterator from unittest import mock import pytest From 3904105da3a641fd9f50a23d3de174e0efa20b40 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 16:25:31 +0100 Subject: [PATCH 09/13] adds proper info --- .../db/payment_users_repo.py | 12 ++++++++++++ .../services/auto_recharge_process_message.py | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/services/payments/src/simcore_service_payments/db/payment_users_repo.py b/services/payments/src/simcore_service_payments/db/payment_users_repo.py index 2ebb951f130..7c98add9e50 100644 --- a/services/payments/src/simcore_service_payments/db/payment_users_repo.py +++ b/services/payments/src/simcore_service_payments/db/payment_users_repo.py @@ -1,5 +1,6 @@ import sqlalchemy as sa from models_library.users import GroupID, UserID +from pydantic import EmailStr from simcore_postgres_database.models.users import users from .base import BaseRepository @@ -20,3 +21,14 @@ async def get_primary_group_id(self, user_id: UserID) -> GroupID: msg = f"{user_id=} not found" raise ValueError(msg) return GroupID(row.primary_gid) + + async def get_name_and_email(self, user_id: UserID) -> tuple[str, EmailStr]: + async with self.db_engine.begin() as conn: + result = await conn.execute( + sa.select(users.c.name, users.c.email).where(users.c.id == user_id) + ) + row = result.first() + if row is None: + msg = f"{user_id=} not found" + raise ValueError(msg) + return row.name, EmailStr(row.email) diff --git a/services/payments/src/simcore_service_payments/services/auto_recharge_process_message.py b/services/payments/src/simcore_service_payments/services/auto_recharge_process_message.py index a4dae4eefe7..866615dac91 100644 --- a/services/payments/src/simcore_service_payments/services/auto_recharge_process_message.py +++ b/services/payments/src/simcore_service_payments/services/auto_recharge_process_message.py @@ -1,4 +1,5 @@ import logging +from contextlib import suppress from datetime import datetime, timedelta, timezone from decimal import Decimal from typing import cast @@ -16,6 +17,7 @@ from models_library.wallets import WalletID from pydantic import EmailStr, parse_obj_as, parse_raw_as from simcore_service_payments.db.auto_recharge_repo import AutoRechargeRepo +from simcore_service_payments.db.payment_users_repo import PaymentsUsersRepo from simcore_service_payments.db.payments_methods_repo import PaymentsMethodsRepo from simcore_service_payments.db.payments_transactions_repo import ( PaymentsTransactionsRepo, @@ -149,6 +151,15 @@ async def _perform_auto_recharge( payments_gateway = PaymentsGatewayApi.get_from_app_state(app) payments_transactions_repo = PaymentsTransactionsRepo(db_engine=app.state.engine) rut_api = ResourceUsageTrackerApi.get_from_app_state(app) + users_repo = PaymentsUsersRepo(db_engine=app.state.engine) + + # NOTE: this will probably be removed https://github.com/ITISFoundation/appmotion-exchange/issues/21 + user_name = f"id={payment_method_db.user_id}" + user_email = EmailStr(f"placeholder_{payment_method_db.user_id}@example.itis") + with suppress(ValueError): + user_name, user_email = await users_repo.get_name_and_email( + payment_method_db.user_id + ) await pay_with_payment_method( gateway=payments_gateway, @@ -162,7 +173,7 @@ async def _perform_auto_recharge( wallet_id=rabbit_message.wallet_id, wallet_name=f"id={rabbit_message.wallet_id}", user_id=payment_method_db.user_id, - user_name=f"id={payment_method_db.user_id}", - user_email=EmailStr(f"placeholder_{payment_method_db.user_id}@example.itis"), + user_name=user_name, + user_email=user_email, comment="Payment generated by auto recharge", ) From 1af1beb2651178e55376236e709b10e5ff7c9c9a Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 19:40:37 +0100 Subject: [PATCH 10/13] bad merge --- services/payments/tests/unit/test_rpc_payments.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/services/payments/tests/unit/test_rpc_payments.py b/services/payments/tests/unit/test_rpc_payments.py index 1d6e0132790..186ff59f5ec 100644 --- a/services/payments/tests/unit/test_rpc_payments.py +++ b/services/payments/tests/unit/test_rpc_payments.py @@ -140,8 +140,6 @@ async def test_webserver_one_time_payment_workflow( async def test_cancel_invalid_payment_id( is_pdb_enabled: bool, - app: FastAPI, - rpc_client: RabbitMQRPCClient, mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, rpc_client: RabbitMQRPCClient, From 1e1465329f0889512a3ffe94afd3ab03f460f605 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 19:41:23 +0100 Subject: [PATCH 11/13] bad merge --- services/payments/tests/unit/test_rpc_payments.py | 1 + 1 file changed, 1 insertion(+) diff --git a/services/payments/tests/unit/test_rpc_payments.py b/services/payments/tests/unit/test_rpc_payments.py index 186ff59f5ec..246eea6421d 100644 --- a/services/payments/tests/unit/test_rpc_payments.py +++ b/services/payments/tests/unit/test_rpc_payments.py @@ -105,6 +105,7 @@ async def test_rpc_init_payment_fail( async def test_webserver_one_time_payment_workflow( is_pdb_enabled: bool, + mock_payments_gateway_service_or_none: MockRouter | None, app: FastAPI, rpc_client: RabbitMQRPCClient, init_payment_kwargs: dict[str, Any], From e37b6957c9c3b038e48ce6087f2e0a3f54385949 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 20:04:40 +0100 Subject: [PATCH 12/13] raises unavailble --- .../simcore_service_payments/_constants.py | 3 ++ .../simcore_service_payments/core/errors.py | 4 +- .../services/payments_gateway.py | 52 ++++++++++++------- .../payments/tests/unit/test_rpc_payments.py | 11 ++-- .../unit/test_services_payments_gateway.py | 12 ++--- 5 files changed, 53 insertions(+), 29 deletions(-) diff --git a/services/payments/src/simcore_service_payments/_constants.py b/services/payments/src/simcore_service_payments/_constants.py index d2ac1f2b03c..d4768c5d860 100644 --- a/services/payments/src/simcore_service_payments/_constants.py +++ b/services/payments/src/simcore_service_payments/_constants.py @@ -4,3 +4,6 @@ PAG: Final[str] = "Payments Gateway service" PGDB: Final[str] = "Postgres service" RUT: Final[str] = "Resource Usage Tracker service" + + +MSG_GATEWAY_UNAVAILABLE_ERROR = "Our payments provider is temporary unavailable" diff --git a/services/payments/src/simcore_service_payments/core/errors.py b/services/payments/src/simcore_service_payments/core/errors.py index 3a649829130..35a200138da 100644 --- a/services/payments/src/simcore_service_payments/core/errors.py +++ b/services/payments/src/simcore_service_payments/core/errors.py @@ -13,9 +13,9 @@ def get_full_class_name(cls) -> str: # -class PaymentsGatewayError(_BaseAppError): +class BasePaymentsGatewayError(_BaseAppError): ... -class PaymentsGatewayNotReadyError(PaymentsGatewayError): +class PaymentsGatewayNotReadyError(BasePaymentsGatewayError): msg_template = "Payments-Gateway is unresponsive: {checks}" diff --git a/services/payments/src/simcore_service_payments/services/payments_gateway.py b/services/payments/src/simcore_service_payments/services/payments_gateway.py index 2dfb76b2024..a22cf18734e 100644 --- a/services/payments/src/simcore_service_payments/services/payments_gateway.py +++ b/services/payments/src/simcore_service_payments/services/payments_gateway.py @@ -16,9 +16,9 @@ from fastapi import FastAPI from fastapi.encoders import jsonable_encoder from httpx import URL, HTTPStatusError +from models_library.api_schemas_payments.errors import PaymentServiceUnavailableError from models_library.api_schemas_webserver.wallets import PaymentID, PaymentMethodID from pydantic import ValidationError, parse_raw_as -from pydantic.errors import PydanticErrorMixin from servicelib.fastapi.app_state import SingletonInAppStateMixin from servicelib.fastapi.http_client import ( AttachLifespanMixin, @@ -33,7 +33,8 @@ from tenacity.retry import retry_if_exception_type from tenacity.wait import wait_exponential -from ..core.errors import PaymentsGatewayNotReadyError +from .._constants import MSG_GATEWAY_UNAVAILABLE_ERROR, PAG +from ..core.errors import BasePaymentsGatewayError, PaymentsGatewayNotReadyError from ..core.settings import ApplicationSettings from ..models.payments_gateway import ( BatchGetPaymentMethods, @@ -57,13 +58,13 @@ def _parse_raw_as_or_none(cls: type, text: str | None): return None -class PaymentsGatewayError(PydanticErrorMixin, ValueError): +class PaymentsGatewayApiError(BasePaymentsGatewayError): msg_template = "{operation_id} error {status_code}: {reason}" @classmethod def from_http_status_error( cls, err: HTTPStatusError, operation_id: str - ) -> "PaymentsGatewayError": + ) -> "PaymentsGatewayApiError": return cls( operation_id=f"PaymentsGatewayApi.{operation_id}", reason=f"{err}", @@ -86,22 +87,37 @@ def get_detailed_message(self) -> str: @contextlib.contextmanager -def _raise_as_payments_gateway_error(operation_id: str): +def _reraise_as_service_errors_context(operation_id: str): try: yield - except HTTPStatusError as err: - error = PaymentsGatewayError.from_http_status_error( + except httpx.RequestError as err: + _logger.exception("%s: request error", PAG) + raise PaymentServiceUnavailableError( + human_reason=MSG_GATEWAY_UNAVAILABLE_ERROR + ) from err + + except httpx.HTTPStatusError as err: + error = PaymentsGatewayApiError.from_http_status_error( err, operation_id=operation_id ) - _logger.warning(error.get_detailed_message()) - raise error from err + + if err.response.is_client_error: + _logger.warning(error.get_detailed_message()) + raise error from err + + if err.response.is_server_error: + # 5XX in server -> turn into unavailable + _logger.exception(error.get_detailed_message()) + raise PaymentServiceUnavailableError( + human_reason=MSG_GATEWAY_UNAVAILABLE_ERROR + ) from err -def _handle_status_errors(coro: Callable): +def _handle_httpx_errors(coro: Callable): @functools.wraps(coro) async def _wrapper(self, *args, **kwargs): - with _raise_as_payments_gateway_error(operation_id=coro.__name__): + with _reraise_as_service_errors_context(operation_id=coro.__name__): return await coro(self, *args, **kwargs) return _wrapper @@ -125,7 +141,7 @@ class PaymentsGatewayApi( # api: one-time-payment workflow # - @_handle_status_errors + @_handle_httpx_errors async def init_payment(self, payment: InitPayment) -> PaymentInitiated: response = await self.client.post( "/init", @@ -137,7 +153,7 @@ async def init_payment(self, payment: InitPayment) -> PaymentInitiated: def get_form_payment_url(self, id_: PaymentID) -> URL: return self.client.base_url.copy_with(path="/pay", params={"id": f"{id_}"}) - @_handle_status_errors + @_handle_httpx_errors async def cancel_payment( self, payment_initiated: PaymentInitiated ) -> PaymentCancelled: @@ -152,7 +168,7 @@ async def cancel_payment( # api: payment method workflows # - @_handle_status_errors + @_handle_httpx_errors async def init_payment_method( self, payment_method: InitPaymentMethod, @@ -171,7 +187,7 @@ def get_form_payment_method_url(self, id_: PaymentMethodID) -> URL: # CRUD - @_handle_status_errors + @_handle_httpx_errors async def get_many_payment_methods( self, ids_: list[PaymentMethodID] ) -> list[GetPaymentMethod]: @@ -184,18 +200,18 @@ async def get_many_payment_methods( response.raise_for_status() return PaymentMethodsBatch.parse_obj(response.json()).items - @_handle_status_errors + @_handle_httpx_errors async def get_payment_method(self, id_: PaymentMethodID) -> GetPaymentMethod: response = await self.client.get(f"/payment-methods/{id_}") response.raise_for_status() return GetPaymentMethod.parse_obj(response.json()) - @_handle_status_errors + @_handle_httpx_errors async def delete_payment_method(self, id_: PaymentMethodID) -> None: response = await self.client.delete(f"/payment-methods/{id_}") response.raise_for_status() - @_handle_status_errors + @_handle_httpx_errors async def pay_with_payment_method( self, id_: PaymentMethodID, payment: InitPayment ) -> AckPaymentWithPaymentMethod: diff --git a/services/payments/tests/unit/test_rpc_payments.py b/services/payments/tests/unit/test_rpc_payments.py index 246eea6421d..e1ba7921b9c 100644 --- a/services/payments/tests/unit/test_rpc_payments.py +++ b/services/payments/tests/unit/test_rpc_payments.py @@ -9,7 +9,10 @@ import pytest from faker import Faker from fastapi import FastAPI -from models_library.api_schemas_payments.errors import PaymentNotFoundError +from models_library.api_schemas_payments.errors import ( + PaymentNotFoundError, + PaymentServiceUnavailableError, +) from models_library.api_schemas_webserver.wallets import WalletPaymentInitiated from models_library.rabbitmq_basic_types import RPCMethodName from pydantic import parse_obj_as @@ -17,7 +20,7 @@ from pytest_simcore.helpers.typing_env import EnvVarsDict from pytest_simcore.helpers.utils_envs import setenvs_from_dict from respx import MockRouter -from servicelib.rabbitmq import RabbitMQRPCClient, RPCServerError +from servicelib.rabbitmq import RabbitMQRPCClient from servicelib.rabbitmq._constants import RPC_REQUEST_DEFAULT_TIMEOUT_S from simcore_service_payments.api.rpc.routes import PAYMENTS_RPC_NAMESPACE @@ -87,7 +90,7 @@ async def test_rpc_init_payment_fail( ): assert app - with pytest.raises(RPCServerError) as exc_info: + with pytest.raises(PaymentServiceUnavailableError) as exc_info: await rpc_client.request( PAYMENTS_RPC_NAMESPACE, parse_obj_as(RPCMethodName, "init_payment"), @@ -101,6 +104,8 @@ async def test_rpc_init_payment_fail( assert error.method_name == "init_payment" assert error.exc_message assert error.traceback + # FIXME: should raise + assert isinstance(exc_info.value, PaymentServiceUnavailableError) async def test_webserver_one_time_payment_workflow( diff --git a/services/payments/tests/unit/test_services_payments_gateway.py b/services/payments/tests/unit/test_services_payments_gateway.py index 4247737c88d..4a0006f8cd3 100644 --- a/services/payments/tests/unit/test_services_payments_gateway.py +++ b/services/payments/tests/unit/test_services_payments_gateway.py @@ -17,8 +17,8 @@ ) from simcore_service_payments.services.payments_gateway import ( PaymentsGatewayApi, - PaymentsGatewayError, - _raise_as_payments_gateway_error, + PaymentsGatewayApiError, + _reraise_as_service_errors_context, setup_payments_gateway, ) @@ -187,7 +187,7 @@ async def test_payment_methods_workflow( # delete payment-method await payments_gateway_api.delete_payment_method(payment_method_id) - with pytest.raises(PaymentsGatewayError) as err_info: + with pytest.raises(PaymentsGatewayApiError) as err_info: await payments_gateway_api.get_payment_method(payment_method_id) assert str(err_info.value) @@ -205,7 +205,7 @@ async def test_payment_methods_workflow( async def test_payments_gateway_error_exception(): async def _go(): - with _raise_as_payments_gateway_error(operation_id="foo"): + with _reraise_as_service_errors_context(operation_id="foo"): async with httpx.AsyncClient( app=FastAPI(), base_url="http://payments.testserver.io", @@ -213,10 +213,10 @@ async def _go(): response = await client.post("/foo", params={"x": "3"}, json={"y": 12}) response.raise_for_status() - with pytest.raises(PaymentsGatewayError) as err_info: + with pytest.raises(PaymentsGatewayApiError) as err_info: await _go() err = err_info.value - assert isinstance(err, PaymentsGatewayError) + assert isinstance(err, PaymentsGatewayApiError) assert "curl -X POST" in err.get_detailed_message() From a7e3bdb769d092e38d6a61aa4bd113daaa376ff4 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 11 Dec 2023 20:10:49 +0100 Subject: [PATCH 13/13] raises unavailable --- .../payments/src/simcore_service_payments/_constants.py | 2 +- .../simcore_service_payments/services/payments_gateway.py | 4 ++-- services/payments/tests/unit/test_rpc_payments.py | 7 ------- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/services/payments/src/simcore_service_payments/_constants.py b/services/payments/src/simcore_service_payments/_constants.py index d4768c5d860..a9bfd404c56 100644 --- a/services/payments/src/simcore_service_payments/_constants.py +++ b/services/payments/src/simcore_service_payments/_constants.py @@ -6,4 +6,4 @@ RUT: Final[str] = "Resource Usage Tracker service" -MSG_GATEWAY_UNAVAILABLE_ERROR = "Our payments provider is temporary unavailable" +MSG_GATEWAY_UNAVAILABLE_ERROR = "Our payments provider is temporary innoperative" diff --git a/services/payments/src/simcore_service_payments/services/payments_gateway.py b/services/payments/src/simcore_service_payments/services/payments_gateway.py index a22cf18734e..de72274184f 100644 --- a/services/payments/src/simcore_service_payments/services/payments_gateway.py +++ b/services/payments/src/simcore_service_payments/services/payments_gateway.py @@ -94,7 +94,7 @@ def _reraise_as_service_errors_context(operation_id: str): except httpx.RequestError as err: _logger.exception("%s: request error", PAG) raise PaymentServiceUnavailableError( - human_reason=MSG_GATEWAY_UNAVAILABLE_ERROR + human_readable_detail=MSG_GATEWAY_UNAVAILABLE_ERROR ) from err except httpx.HTTPStatusError as err: @@ -110,7 +110,7 @@ def _reraise_as_service_errors_context(operation_id: str): # 5XX in server -> turn into unavailable _logger.exception(error.get_detailed_message()) raise PaymentServiceUnavailableError( - human_reason=MSG_GATEWAY_UNAVAILABLE_ERROR + human_readable_detail=MSG_GATEWAY_UNAVAILABLE_ERROR ) from err diff --git a/services/payments/tests/unit/test_rpc_payments.py b/services/payments/tests/unit/test_rpc_payments.py index e1ba7921b9c..e64a499d6f6 100644 --- a/services/payments/tests/unit/test_rpc_payments.py +++ b/services/payments/tests/unit/test_rpc_payments.py @@ -98,13 +98,6 @@ async def test_rpc_init_payment_fail( timeout_s=None if is_pdb_enabled else 5, ) - error = exc_info.value - assert isinstance(error, RPCServerError) - assert error.exc_type == "httpx.ConnectError" - assert error.method_name == "init_payment" - assert error.exc_message - assert error.traceback - # FIXME: should raise assert isinstance(exc_info.value, PaymentServiceUnavailableError)