From 7f366ebd639d758eea36fc0b8316a3b105a44b27 Mon Sep 17 00:00:00 2001 From: Franz Forstmayr Date: Tue, 17 Sep 2024 00:23:15 +0200 Subject: [PATCH 1/4] Demonstrate get_or_upsert issue --- src/app/config/base.py | 2 +- src/app/db/models/user_role.py | 7 +++-- .../domain/accounts/controllers/user_role.py | 10 +++++-- tests/integration/test_account_role.py | 26 +++++++++++++++++++ 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/app/config/base.py b/src/app/config/base.py index 80708d2b..faefc839 100644 --- a/src/app/config/base.py +++ b/src/app/config/base.py @@ -327,7 +327,7 @@ class LogSettings: """Log event name for logs from SAQ worker.""" SAQ_LEVEL: int = 20 """Level to log SAQ logs.""" - SQLALCHEMY_LEVEL: int = 20 + SQLALCHEMY_LEVEL: int = 30 """Level to log SQLAlchemy logs.""" UVICORN_ACCESS_LEVEL: int = 20 """Level to log uvicorn access logs.""" diff --git a/src/app/db/models/user_role.py b/src/app/db/models/user_role.py index 44855bae..63f4b2e0 100644 --- a/src/app/db/models/user_role.py +++ b/src/app/db/models/user_role.py @@ -5,7 +5,7 @@ from uuid import UUID # noqa: TCH003 from advanced_alchemy.base import UUIDAuditBase -from sqlalchemy import ForeignKey +from sqlalchemy import ForeignKey, UniqueConstraint from sqlalchemy.ext.associationproxy import AssociationProxy, association_proxy from sqlalchemy.orm import Mapped, mapped_column, relationship @@ -18,7 +18,10 @@ class UserRole(UUIDAuditBase): """User Role.""" __tablename__ = "user_account_role" - __table_args__ = {"comment": "Links a user to a specific role."} + __table_args__ = ( + UniqueConstraint("user_id", "role_id"), # Avoid multiple assignments of the same role + {"comment": "Links a user to a specific role."}, + ) user_id: Mapped[UUID] = mapped_column(ForeignKey("user_account.id", ondelete="cascade"), nullable=False) role_id: Mapped[UUID] = mapped_column(ForeignKey("role.id", ondelete="cascade"), nullable=False) assigned_at: Mapped[datetime] = mapped_column(default=datetime.now(UTC)) diff --git a/src/app/domain/accounts/controllers/user_role.py b/src/app/domain/accounts/controllers/user_role.py index fb794318..cad41321 100644 --- a/src/app/domain/accounts/controllers/user_role.py +++ b/src/app/domain/accounts/controllers/user_role.py @@ -1,4 +1,5 @@ """User Routes.""" + from __future__ import annotations from litestar import Controller, post @@ -44,8 +45,13 @@ async def assign_role( """Create a new migration role.""" role_id = (await roles_service.get_one(slug=role_slug)).id user_obj = await users_service.get_one(email=data.user_name) - if all(user_role.role_id != role_id for user_role in user_obj.roles): - obj, created = await user_roles_service.get_or_upsert(role_id=role_id, user_id=user_obj.id) + # if all(user_role.role_id != role_id for user_role in user_obj.roles): + obj, created = await user_roles_service.get_or_upsert( + role_id=role_id, + user_id=user_obj.id, + # with_for_update=True, + # auto_commit=True, + ) if created: return Message(message=f"Successfully assigned the '{obj.role_slug}' role to {obj.user_email}.") return Message(message=f"User {obj.user_email} already has the '{obj.role_slug}' role.") diff --git a/tests/integration/test_account_role.py b/tests/integration/test_account_role.py index 1b1fcf54..632ae13d 100644 --- a/tests/integration/test_account_role.py +++ b/tests/integration/test_account_role.py @@ -1,8 +1,10 @@ from __future__ import annotations +import asyncio from typing import TYPE_CHECKING import pytest +from httpx import Response if TYPE_CHECKING: from httpx import AsyncClient @@ -66,3 +68,27 @@ async def test_superuser_role_access( response = await client.get("/api/teams", headers=user_token_headers) assert response.status_code == 200 assert int(response.json()["total"]) == 0 + + +@pytest.mark.parametrize("n_requests", [1, 4]) +async def test_assign_role_concurrent( + client: "AsyncClient", + superuser_token_headers: dict[str, str], + n_requests: int, +) -> None: + async def post() -> Response: + return await client.post( + "/api/roles/superuser/assign", + json={"userName": "user@example.com"}, + headers=superuser_token_headers, + ) + + responses = await asyncio.gather(*[post() for _ in range(n_requests)]) + + assert all(res.status_code == 201 for res in responses) + messages = [res.json()["message"] for res in responses] + assert "Successfully assigned the 'superuser' role to user@example.com." in messages + + responses = await asyncio.gather(*[post() for _ in range(n_requests)]) + messages = [res.json()["message"] for res in responses] + assert "User user@example.com already has the 'superuser' role." in messages From c59b5fd4fefaf13c1bd7fa9589783f04c5483e72 Mon Sep 17 00:00:00 2001 From: Franz Forstmayr Date: Tue, 17 Sep 2024 00:24:19 +0200 Subject: [PATCH 2/4] Reset log level --- src/app/config/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/config/base.py b/src/app/config/base.py index faefc839..80708d2b 100644 --- a/src/app/config/base.py +++ b/src/app/config/base.py @@ -327,7 +327,7 @@ class LogSettings: """Log event name for logs from SAQ worker.""" SAQ_LEVEL: int = 20 """Level to log SAQ logs.""" - SQLALCHEMY_LEVEL: int = 30 + SQLALCHEMY_LEVEL: int = 20 """Level to log SQLAlchemy logs.""" UVICORN_ACCESS_LEVEL: int = 20 """Level to log uvicorn access logs.""" From f5fd0981ae44f1ef8cc083b969bb099484f03391 Mon Sep 17 00:00:00 2001 From: Franz Forstmayr Date: Wed, 18 Sep 2024 00:21:09 +0200 Subject: [PATCH 3/4] Stricter checks --- tests/integration/test_account_role.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_account_role.py b/tests/integration/test_account_role.py index 632ae13d..253e919d 100644 --- a/tests/integration/test_account_role.py +++ b/tests/integration/test_account_role.py @@ -90,5 +90,6 @@ async def post() -> Response: assert "Successfully assigned the 'superuser' role to user@example.com." in messages responses = await asyncio.gather(*[post() for _ in range(n_requests)]) + assert all(res.status_code == 201 for res in responses) messages = [res.json()["message"] for res in responses] - assert "User user@example.com already has the 'superuser' role." in messages + assert all(msg == "User user@example.com already has the 'superuser' role." for msg in messages) From fe48dd4dd6be48e0fe4360e0b05866f349d14240 Mon Sep 17 00:00:00 2001 From: Franz Forstmayr Date: Wed, 18 Sep 2024 00:22:23 +0200 Subject: [PATCH 4/4] Remove commented code for linter --- src/app/domain/accounts/controllers/user_role.py | 2 -- tests/integration/test_account_role.py | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/app/domain/accounts/controllers/user_role.py b/src/app/domain/accounts/controllers/user_role.py index cad41321..5e131667 100644 --- a/src/app/domain/accounts/controllers/user_role.py +++ b/src/app/domain/accounts/controllers/user_role.py @@ -49,8 +49,6 @@ async def assign_role( obj, created = await user_roles_service.get_or_upsert( role_id=role_id, user_id=user_obj.id, - # with_for_update=True, - # auto_commit=True, ) if created: return Message(message=f"Successfully assigned the '{obj.role_slug}' role to {obj.user_email}.") diff --git a/tests/integration/test_account_role.py b/tests/integration/test_account_role.py index 253e919d..e0f5efa3 100644 --- a/tests/integration/test_account_role.py +++ b/tests/integration/test_account_role.py @@ -4,10 +4,9 @@ from typing import TYPE_CHECKING import pytest -from httpx import Response if TYPE_CHECKING: - from httpx import AsyncClient + from httpx import AsyncClient, Response pytestmark = pytest.mark.anyio