From 0e2c66766a8bcebecadc396abd887a9accfa9415 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 13:51:51 +0300 Subject: [PATCH 1/2] test: pin SQL statement counts per endpoint and a BEGIN-free revocation check --- tests/api/test_statement_counts.py | 161 ++++++++++++++++++++ tests/use_cases/test_check_token_revoked.py | 30 ++++ 2 files changed, 191 insertions(+) create mode 100644 tests/api/test_statement_counts.py create mode 100644 tests/use_cases/test_check_token_revoked.py diff --git a/tests/api/test_statement_counts.py b/tests/api/test_statement_counts.py new file mode 100644 index 0000000..6fe00cd --- /dev/null +++ b/tests/api/test_statement_counts.py @@ -0,0 +1,161 @@ +import typing +import uuid + +from httpx import AsyncClient, Response +from sqlalchemy.ext.asyncio import AsyncSession + +from tests.api.helpers import count_statements, create_direct_chat, login, register, send + + +async def _measure(session: AsyncSession, request: typing.Awaitable[Response]) -> tuple[Response, list[str]]: + with count_statements(session) as statements: + response: typing.Final = await request + return response, [statement for statement in statements if "SAVEPOINT" not in statement] + + +async def test_register_runs_one_statement(client: AsyncClient, db_session: AsyncSession) -> None: + response, statements = await _measure( + db_session, + client.post("/api/auth/register/", json={"username": "zed", "password": "hunter2hunter2", "display_name": "Z"}), + ) + + assert response.status_code == 201 + assert len(statements) == 1, statements + + +async def test_logout_runs_three_statements(client: AsyncClient, db_session: AsyncSession) -> None: + await register(client, "alice") + + response, statements = await _measure(db_session, client.post("/api/auth/logout/")) + + assert response.status_code == 204 + assert len(statements) == 3, statements + + +async def test_new_direct_chat_runs_three_statements(client: AsyncClient, db_session: AsyncSession) -> None: + bob_id: typing.Final = await register(client, "bob") + await register(client, "alice") + + response, statements = await _measure( + db_session, client.post("/api/chats/", json={"chat_type": "direct", "member_ids": [bob_id]}) + ) + + assert response.status_code == 201 + assert {member["user_id"] for member in response.json()["members"]} == {bob_id, response.json()["created_by_id"]} + assert len(statements) == 3, statements + + +async def test_direct_chat_replay_runs_three_statements(client: AsyncClient, db_session: AsyncSession) -> None: + _, bob_id = await create_direct_chat(client) + + response, statements = await _measure( + db_session, client.post("/api/chats/", json={"chat_type": "direct", "member_ids": [bob_id]}) + ) + + assert response.status_code == 200 + assert len(response.json()["members"]) == 2 + assert len(statements) == 3, statements + + +async def test_new_group_chat_runs_three_statements(client: AsyncClient, db_session: AsyncSession) -> None: + member_ids: typing.Final = [await register(client, "bob"), await register(client, "carol")] + await register(client, "alice") + + response, statements = await _measure( + db_session, client.post("/api/chats/", json={"chat_type": "group", "title": "g", "member_ids": member_ids}) + ) + + assert response.status_code == 201 + assert response.json()["title"] == "g" + assert len(response.json()["members"]) == 3 + assert len(statements) == 3, statements + + +async def test_chat_listing_runs_two_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + await send(client, chat_id, "hello") + + response, statements = await _measure(db_session, client.get("/api/chats/")) + + assert response.json()["items"][0]["last_message"]["text"] == "hello" + assert len(statements) == 2, statements + + +async def test_chat_detail_runs_two_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + + response, statements = await _measure(db_session, client.get(f"/api/chats/{chat_id}/")) + + assert response.status_code == 200 + assert len(response.json()["members"]) == 2 + assert len(statements) == 2, statements + + +async def test_mark_read_runs_three_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + message: typing.Final = await send(client, chat_id, "hello") + await login(client, "bob") + + response, statements = await _measure( + db_session, client.post(f"/api/chats/{chat_id}/read/", json={"last_read_message_id": message["id"]}) + ) + + assert response.json()["last_read_message_id"] == message["id"] + assert len(statements) == 3, statements + + +async def test_new_message_runs_four_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + + response, statements = await _measure( + db_session, + client.post(f"/api/chats/{chat_id}/messages/", json={"idempotency_key": str(uuid.uuid4()), "text": "hi"}), + ) + + assert response.status_code == 201 + assert len(statements) == 4, statements + + +async def test_edit_runs_three_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + message: typing.Final = await send(client, chat_id, "hi") + + response, statements = await _measure( + db_session, client.patch(f"/api/messages/{message['id']}/", json={"text": "fixed"}) + ) + + assert response.json()["text"] == "fixed" + assert len(statements) == 3, statements + + +async def test_delete_of_an_older_message_runs_four_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + older: typing.Final = await send(client, chat_id, "one") + await send(client, chat_id, "two") + + response, statements = await _measure(db_session, client.delete(f"/api/messages/{older['id']}/")) + + assert response.status_code == 204 + assert len(statements) == 4, statements + + +async def test_delete_of_the_last_message_runs_four_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + await send(client, chat_id, "one") + last: typing.Final = await send(client, chat_id, "two") + + response, statements = await _measure(db_session, client.delete(f"/api/messages/{last['id']}/")) + + assert response.status_code == 204 + assert len(statements) == 4, statements + + +async def test_repeated_delete_runs_two_statements(client: AsyncClient, db_session: AsyncSession) -> None: + chat_id, _ = await create_direct_chat(client) + message: typing.Final = await send(client, chat_id, "one") + await client.delete(f"/api/messages/{message['id']}/") + + response, statements = await _measure(db_session, client.delete(f"/api/messages/{message['id']}/")) + + assert response.status_code == 204 + assert len(statements) == 2, statements diff --git a/tests/use_cases/test_check_token_revoked.py b/tests/use_cases/test_check_token_revoked.py new file mode 100644 index 0000000..8cbad15 --- /dev/null +++ b/tests/use_cases/test_check_token_revoked.py @@ -0,0 +1,30 @@ +import typing + +import modern_di +import sqlalchemy as sa +from sqlalchemy import event + +from app import ioc +from app.database.resources import create_database_engine + + +async def test_revocation_check_sends_no_begin_or_commit(di_container: modern_di.Container) -> None: + engine: typing.Final = create_database_engine() + in_transaction: typing.Final[list[bool]] = [] + + def record(connection: sa.Connection, *_: object) -> None: + driver_connection = connection.connection.driver_connection + assert driver_connection is not None + in_transaction.append(driver_connection.is_in_transaction()) + + event.listen(engine.sync_engine, "after_cursor_execute", record) + di_container.override(ioc.Database.database_engine, engine) + try: + async with di_container.build_child_container(scope=modern_di.Scope.REQUEST) as container: + use_case: typing.Final = container.resolve_provider(ioc.UseCases.check_token_revoked_use_case) + assert await use_case(jti="unknown") is False + finally: + di_container.reset_override(ioc.Database.database_engine) + await engine.dispose() + + assert in_transaction == [False] From 1f034c51ff61d6299221450aa7db9e84b5e4f3a5 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 14:00:37 +0300 Subject: [PATCH 2/2] perf: cut avoidable SQL round trips from every endpoint --- AGENTS.md | 4 ++ app/database/resources.py | 5 +++ app/ioc.py | 13 +++++- app/repositories/chat_members_repository.py | 30 +++++++++++-- app/repositories/chats_repository.py | 40 +++++++++++++++--- app/repositories/messages_repository.py | 42 ++++++++++++++----- app/repositories/revoked_tokens_repository.py | 5 ++- app/use_cases/create_chat.py | 11 ++--- app/use_cases/create_message.py | 6 +-- app/use_cases/delete_message.py | 17 +------- app/use_cases/edit_message.py | 7 +--- app/use_cases/fetch_chat.py | 10 ++--- app/use_cases/mark_read.py | 14 +++---- app/use_cases/message_authorization.py | 10 ++--- app/use_cases/register_user.py | 3 +- tests/conftest.py | 3 ++ tests/use_cases/test_connection_loss.py | 17 ++++---- 17 files changed, 154 insertions(+), 83 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ceb9346..1494c77 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,6 +109,10 @@ a session, a transaction or a repository directly. Provider scopes: - `Database.database_session` — request-scoped, finalizer closes the session. - `Database.transaction` — request-scoped `db_retry.Transaction`, the object every use case wraps its work in. +- `Database.autocommit_transaction` — the same, opened at `AUTOCOMMIT` so the + token revocation check sends no BEGIN or COMMIT. `rolled_back_db_session` + overrides `Database.autocommit_isolation_level` to `None`, because switching + the shared test connection would roll back its outer transaction. - `Repositories.*` — request-scoped, `auto_commit=False` (see above). - `UseCases.*` — request-scoped, one class per operation. diff --git a/app/database/resources.py b/app/database/resources.py index 49a70d3..0778eeb 100644 --- a/app/database/resources.py +++ b/app/database/resources.py @@ -1,6 +1,7 @@ import asyncio import typing +from sqlalchemy.engine.interfaces import IsolationLevel from sqlalchemy.ext import asyncio as sa from app.settings import settings @@ -31,6 +32,10 @@ def create_session(engine: sa.AsyncEngine | sa.AsyncConnection) -> sa.AsyncSessi ) +def autocommit_isolation_level() -> IsolationLevel | None: + return "AUTOCOMMIT" + + async def close_session(session: sa.AsyncSession) -> None: task: typing.Final = asyncio.create_task(session.close()) await asyncio.shield(task) diff --git a/app/ioc.py b/app/ioc.py index eea6a5f..08892aa 100644 --- a/app/ioc.py +++ b/app/ioc.py @@ -39,6 +39,15 @@ class Database(Group): creator=Transaction, kwargs={"session": database_session}, ) + autocommit_isolation_level = providers.Factory( + creator=database_resources.autocommit_isolation_level, bound_type=None + ) + autocommit_transaction = providers.Factory( + scope=Scope.REQUEST, + creator=Transaction, + bound_type=None, + kwargs={"session": database_session, "isolation_level": autocommit_isolation_level}, + ) class Repositories(Group, scope=Scope.REQUEST): @@ -77,7 +86,9 @@ class UseCases(Group, scope=Scope.REQUEST): fetch_user_use_case = providers.Factory(creator=FetchUserUseCase) mark_read_use_case = providers.Factory(creator=MarkReadUseCase) revoke_token_use_case = providers.Factory(creator=RevokeTokenUseCase) - check_token_revoked_use_case = providers.Factory(creator=CheckTokenRevokedUseCase) + check_token_revoked_use_case = providers.Factory( + creator=CheckTokenRevokedUseCase, kwargs={"transaction": Database.autocommit_transaction} + ) ALL_GROUPS: typing.Final[list[type[Group]]] = [Database, Repositories, UseCases] diff --git a/app/repositories/chat_members_repository.py b/app/repositories/chat_members_repository.py index 833b26a..27bab33 100644 --- a/app/repositories/chat_members_repository.py +++ b/app/repositories/chat_members_repository.py @@ -1,8 +1,11 @@ import typing +from collections.abc import Sequence import sqlalchemy as sa -from advanced_alchemy.repository import SQLAlchemyAsyncRepository +from advanced_alchemy.exceptions import wrap_sqlalchemy_exception +from advanced_alchemy.repository import DEFAULT_ERROR_MESSAGE_TEMPLATES, SQLAlchemyAsyncRepository from advanced_alchemy.service import SQLAlchemyAsyncRepositoryService +from sqlalchemy import orm from app.database import tables @@ -16,8 +19,29 @@ class BaseRepository(SQLAlchemyAsyncRepository[tables.ChatMembersTable]): async def is_member(self, chat_id: int, user_id: int) -> bool: return await self.exists(chat_id=chat_id, user_id=user_id) - async def fetch_member(self, chat_id: int, user_id: int) -> tables.ChatMembersTable | None: - return await self.get_one_or_none(chat_id=chat_id, user_id=user_id) + async def add_members(self, chat: tables.ChatsTable, user_ids: Sequence[int]) -> None: + with wrap_sqlalchemy_exception(error_messages=DEFAULT_ERROR_MESSAGE_TEMPLATES, dialect_name="postgresql"): + members: typing.Final = await self.repository.session.scalars( + sa.insert(tables.ChatMembersTable).returning(tables.ChatMembersTable), + [{"chat_id": chat.id, "user_id": user_id} for user_id in user_ids], + ) + orm.attributes.set_committed_value(chat, "members", members.all()) + + async def fetch_member_and_message_exists( + self, chat_id: int, user_id: int, message_id: int + ) -> tuple[tables.ChatMembersTable, bool] | None: + message_exists: typing.Final = ( + sa.exists() + .where(tables.MessagesTable.id == message_id, tables.MessagesTable.chat_id == chat_id) + .label("message_exists") + ) + result: typing.Final = await self.repository.session.execute( + sa.select(tables.ChatMembersTable, message_exists).where( + tables.ChatMembersTable.chat_id == chat_id, tables.ChatMembersTable.user_id == user_id + ) + ) + row: typing.Final = result.one_or_none() + return None if row is None else (row[0], row[1]) async def mark_read(self, member_id: int, requested_message_id: int) -> tables.ChatMembersTable: """Advance last_read_message_id to GREATEST(current, requested), atomically. diff --git a/app/repositories/chats_repository.py b/app/repositories/chats_repository.py index 448f9a6..227ede8 100644 --- a/app/repositories/chats_repository.py +++ b/app/repositories/chats_repository.py @@ -1,3 +1,4 @@ +import datetime as dt import typing from collections.abc import Sequence @@ -11,6 +12,14 @@ from app.database import tables +_WITH_MEMBERS: typing.Final[dict[str, typing.Any]] = { + "load": [orm.joinedload(tables.ChatsTable.members)], + "uniquify": True, + # A chat already in the identity map keeps its noload members without this. + "execution_options": {"populate_existing": True}, +} + + class ChatsRepository(SQLAlchemyAsyncRepositoryService[tables.ChatsTable]): class BaseRepository(SQLAlchemyAsyncRepository[tables.ChatsTable]): model_type = tables.ChatsTable @@ -18,11 +27,30 @@ class BaseRepository(SQLAlchemyAsyncRepository[tables.ChatsTable]): repository_type = BaseRepository async def fetch_with_members(self, chat_id: int) -> tables.ChatsTable: - return await self.get_one( - tables.ChatsTable.id == chat_id, - load=[orm.selectinload(tables.ChatsTable.members)], - # A chat already in the identity map keeps its noload members without this. - execution_options={"populate_existing": True}, + return await self.get_one(tables.ChatsTable.id == chat_id, **_WITH_MEMBERS) + + async def find_with_members(self, chat_id: int) -> tables.ChatsTable | None: + return await self.get_one_or_none(tables.ChatsTable.id == chat_id, **_WITH_MEMBERS) + + async def set_last_message(self, chat_id: int, message_id: int) -> None: + await self.repository.session.execute( + sa.update(tables.ChatsTable) + .where(tables.ChatsTable.id == chat_id) + .values(last_message_id=message_id, updated_at=dt.datetime.now(tz=dt.UTC)) + ) + + async def repoint_last_message(self, chat_id: int, deleted_message_id: int) -> None: + newest_active: typing.Final = ( + sa.select(tables.MessagesTable.id) + .where(tables.MessagesTable.chat_id == chat_id, tables.MessagesTable.deleted_at.is_(None)) + .order_by(tables.MessagesTable.id.desc()) + .limit(1) + .scalar_subquery() + ) + await self.repository.session.execute( + sa.update(tables.ChatsTable) + .where(tables.ChatsTable.id == chat_id, tables.ChatsTable.last_message_id == deleted_message_id) + .values(last_message_id=newest_active, updated_at=dt.datetime.now(tz=dt.UTC)) ) async def get_or_create_direct(self, *, direct_key: str, created_by_id: int) -> tuple[tables.ChatsTable, bool]: @@ -59,7 +87,7 @@ async def list_for_user(self, user_id: int) -> Sequence[tables.ChatsTable]: sa.select(tables.ChatsTable) .options( orm.with_expression(tables.ChatsTable.unread_count, unread_count), - orm.selectinload(tables.ChatsTable.last_message), + orm.joinedload(tables.ChatsTable.last_message), ) .join(tables.ChatMembersTable, tables.ChatMembersTable.chat_id == tables.ChatsTable.id) .where(tables.ChatMembersTable.user_id == user_id) diff --git a/app/repositories/messages_repository.py b/app/repositories/messages_repository.py index 36bd7dd..bcd9539 100644 --- a/app/repositories/messages_repository.py +++ b/app/repositories/messages_repository.py @@ -1,9 +1,10 @@ +import datetime as dt import typing import uuid from collections.abc import Sequence import sqlalchemy as sa -from advanced_alchemy.exceptions import wrap_sqlalchemy_exception +from advanced_alchemy.exceptions import NotFoundError, wrap_sqlalchemy_exception from advanced_alchemy.filters import LimitOffset from advanced_alchemy.repository import DEFAULT_ERROR_MESSAGE_TEMPLATES, SQLAlchemyAsyncRepository from advanced_alchemy.service import SQLAlchemyAsyncRepositoryService @@ -37,18 +38,37 @@ async def get_or_create( message, was_created = result.one() return message, was_created - async def fetch_latest_active(self, chat_id: int) -> tables.MessagesTable | None: - """Return the newest non-deleted message in a chat, or None if none remains. + async def fetch_with_membership(self, message_id: int, user_id: int) -> tuple[tables.MessagesTable, bool]: + is_member: typing.Final = ( + sa.exists() + .where( + tables.ChatMembersTable.chat_id == tables.MessagesTable.chat_id, + tables.ChatMembersTable.user_id == user_id, + ) + .label("is_member") + ) + result: typing.Final = await self.repository.session.execute( + sa.select(tables.MessagesTable, is_member).where(tables.MessagesTable.id == message_id) + ) + row: typing.Final = result.one_or_none() + if row is None: + msg = "No item found when one was expected" + raise NotFoundError(msg) + return row[0], row[1] - Used to repoint ChatsTable.last_message_id after a delete removes the current pointer. - """ - messages = await self.get_many( - tables.MessagesTable.chat_id == chat_id, - tables.MessagesTable.deleted_at.is_(None), - LimitOffset(limit=1, offset=0), - order_by=[("id", True)], + async def edit_text(self, message_id: int, text: str, edited_at: dt.datetime) -> tables.MessagesTable: + result: typing.Final = await self.repository.session.execute( + sa.update(tables.MessagesTable) + .where(tables.MessagesTable.id == message_id) + .values(text=text, edited_at=edited_at) + .returning(tables.MessagesTable) + ) + return result.scalar_one() + + async def soft_delete(self, message_id: int, deleted_at: dt.datetime) -> None: + await self.repository.session.execute( + sa.update(tables.MessagesTable).where(tables.MessagesTable.id == message_id).values(deleted_at=deleted_at) ) - return messages[0] if messages else None async def list_page( self, diff --git a/app/repositories/revoked_tokens_repository.py b/app/repositories/revoked_tokens_repository.py index f226fc1..ead14b4 100644 --- a/app/repositories/revoked_tokens_repository.py +++ b/app/repositories/revoked_tokens_repository.py @@ -1,5 +1,6 @@ import datetime as dt +import sqlalchemy as sa from advanced_alchemy.repository import SQLAlchemyAsyncRepository from advanced_alchemy.service import SQLAlchemyAsyncRepositoryService from sqlalchemy.dialects import postgresql @@ -25,4 +26,6 @@ async def revoke(self, jti: str, expires_at: dt.datetime) -> None: ) async def prune_expired(self, now: dt.datetime) -> None: - await self.delete_where(tables.RevokedTokensTable.expires_at < now) + await self.repository.session.execute( + sa.delete(tables.RevokedTokensTable).where(tables.RevokedTokensTable.expires_at < now) + ) diff --git a/app/use_cases/create_chat.py b/app/use_cases/create_chat.py index c698b40..501df6e 100644 --- a/app/use_cases/create_chat.py +++ b/app/use_cases/create_chat.py @@ -38,12 +38,13 @@ async def __call__(self, *, actor: Actor, data: CreateChatRequest) -> tuple[tabl ) else: chat = await self.chats_repository.create( - tables.ChatsTable(chat_type=data.chat_type, title=data.title, created_by_id=actor.id) + tables.ChatsTable(chat_type=data.chat_type, title=data.title, created_by_id=actor.id), + auto_refresh=False, ) created = True if created: - for user_id in sorted(member_ids): - await self.chat_members_repository.create(tables.ChatMembersTable(chat_id=chat.id, user_id=user_id)) - chat_with_members: typing.Final = await self.chats_repository.fetch_with_members(chat.id) + await self.chat_members_repository.add_members(chat, sorted(member_ids)) + else: + chat = await self.chats_repository.fetch_with_members(chat.id) await self.transaction.commit() - return chat_with_members, created + return chat, created diff --git a/app/use_cases/create_message.py b/app/use_cases/create_message.py index 30eeed3..26acb78 100644 --- a/app/use_cases/create_message.py +++ b/app/use_cases/create_message.py @@ -31,10 +31,6 @@ async def __call__( chat_id=chat_id, user_id=actor.id, idempotency_key=data.idempotency_key, text=data.text ) if created: - await self.chats_repository.update( - tables.ChatsTable(id=chat_id, last_message_id=message.id), - item_id=chat_id, - attribute_names=["last_message_id"], - ) + await self.chats_repository.set_last_message(chat_id, message.id) await self.transaction.commit() return message, created diff --git a/app/use_cases/delete_message.py b/app/use_cases/delete_message.py index e0a1779..9156fc6 100644 --- a/app/use_cases/delete_message.py +++ b/app/use_cases/delete_message.py @@ -4,8 +4,6 @@ from db_retry import Transaction, postgres_retry from app.actor import Actor -from app.database import tables -from app.repositories.chat_members_repository import ChatMembersRepository from app.repositories.chats_repository import ChatsRepository from app.repositories.messages_repository import MessagesRepository from app.use_cases.message_authorization import fetch_message_for_author @@ -15,7 +13,6 @@ class DeleteMessageUseCase: transaction: Transaction messages_repository: MessagesRepository - chat_members_repository: ChatMembersRepository chats_repository: ChatsRepository @postgres_retry @@ -23,7 +20,6 @@ async def __call__(self, *, actor: Actor, message_id: int) -> None: async with self.transaction: message = await fetch_message_for_author( messages_repository=self.messages_repository, - chat_members_repository=self.chat_members_repository, actor=actor, message_id=message_id, action="delete", @@ -31,15 +27,6 @@ async def __call__(self, *, actor: Actor, message_id: int) -> None: if message.deleted_at is not None: # DELETE is idempotent; a second delete is not an error, unlike an edit. return - message.deleted_at = dt.datetime.now(tz=dt.UTC) - await self.messages_repository.update(message, item_id=message_id) - - chat = await self.chats_repository.get_one(id=message.chat_id) - if chat.last_message_id == message_id: - newest = await self.messages_repository.fetch_latest_active(message.chat_id) - await self.chats_repository.update( - tables.ChatsTable(id=message.chat_id, last_message_id=newest.id if newest is not None else None), - item_id=message.chat_id, - attribute_names=["last_message_id"], - ) + await self.messages_repository.soft_delete(message_id, dt.datetime.now(tz=dt.UTC)) + await self.chats_repository.repoint_last_message(message.chat_id, message_id) await self.transaction.commit() diff --git a/app/use_cases/edit_message.py b/app/use_cases/edit_message.py index 0b99773..523278a 100644 --- a/app/use_cases/edit_message.py +++ b/app/use_cases/edit_message.py @@ -6,7 +6,6 @@ from app.actor import Actor from app.database import tables from app.exceptions import ConflictError -from app.repositories.chat_members_repository import ChatMembersRepository from app.repositories.messages_repository import MessagesRepository from app.schemas.api import EditMessageRequest from app.use_cases.message_authorization import fetch_message_for_author @@ -16,14 +15,12 @@ class EditMessageUseCase: transaction: Transaction messages_repository: MessagesRepository - chat_members_repository: ChatMembersRepository @postgres_retry async def __call__(self, *, actor: Actor, message_id: int, data: EditMessageRequest) -> tables.MessagesTable: async with self.transaction: message = await fetch_message_for_author( messages_repository=self.messages_repository, - chat_members_repository=self.chat_members_repository, actor=actor, message_id=message_id, action="edit", @@ -31,9 +28,7 @@ async def __call__(self, *, actor: Actor, message_id: int, data: EditMessageRequ if message.deleted_at is not None: msg = "This message has been deleted" raise ConflictError(msg) - message.text = data.text - message.edited_at = dt.datetime.now(tz=dt.UTC) - updated = await self.messages_repository.update(message, item_id=message_id) + updated = await self.messages_repository.edit_text(message_id, data.text, dt.datetime.now(tz=dt.UTC)) await self.transaction.commit() # Safe inside the block: commit() ended the transaction, so __aexit__ only closes. return updated diff --git a/app/use_cases/fetch_chat.py b/app/use_cases/fetch_chat.py index b9cf76f..6dea6e5 100644 --- a/app/use_cases/fetch_chat.py +++ b/app/use_cases/fetch_chat.py @@ -6,7 +6,6 @@ from app.actor import Actor from app.database import tables from app.exceptions import PermissionDeniedError -from app.repositories.chat_members_repository import ChatMembersRepository from app.repositories.chats_repository import ChatsRepository @@ -14,14 +13,13 @@ class FetchChatUseCase: transaction: Transaction chats_repository: ChatsRepository - chat_members_repository: ChatMembersRepository @postgres_retry async def __call__(self, *, actor: Actor, chat_id: int) -> tables.ChatsTable: async with self.transaction: - if not await self.chat_members_repository.is_member(chat_id, actor.id): - msg = "Not a member of this chat" - raise PermissionDeniedError(msg) - chat: typing.Final = await self.chats_repository.fetch_with_members(chat_id) + chat: typing.Final = await self.chats_repository.find_with_members(chat_id) await self.transaction.commit() + if chat is None or all(member.user_id != actor.id for member in chat.members): + msg = "Not a member of this chat" + raise PermissionDeniedError(msg) return chat diff --git a/app/use_cases/mark_read.py b/app/use_cases/mark_read.py index 469593a..72e7458 100644 --- a/app/use_cases/mark_read.py +++ b/app/use_cases/mark_read.py @@ -7,7 +7,6 @@ from app.database import tables from app.exceptions import PermissionDeniedError, ValidationError from app.repositories.chat_members_repository import ChatMembersRepository -from app.repositories.messages_repository import MessagesRepository from app.schemas.api import MarkReadRequest @@ -15,20 +14,19 @@ class MarkReadUseCase: transaction: Transaction chat_members_repository: ChatMembersRepository - messages_repository: MessagesRepository @postgres_retry async def __call__(self, *, actor: Actor, chat_id: int, data: MarkReadRequest) -> tables.ChatMembersTable: async with self.transaction: - member: typing.Final = await self.chat_members_repository.fetch_member(chat_id, actor.id) - if member is None: + found: typing.Final = await self.chat_members_repository.fetch_member_and_message_exists( + chat_id, actor.id, data.last_read_message_id + ) + if found is None: msg = "Not a member of this chat" raise PermissionDeniedError(msg) - target_message = await self.messages_repository.get_one_or_none( - id=data.last_read_message_id, chat_id=chat_id - ) - if target_message is None: + member, message_exists = found + if not message_exists: msg = "last_read_message_id does not name a message in this chat" raise ValidationError(msg) diff --git a/app/use_cases/message_authorization.py b/app/use_cases/message_authorization.py index 7a0244e..ab894f9 100644 --- a/app/use_cases/message_authorization.py +++ b/app/use_cases/message_authorization.py @@ -1,16 +1,12 @@ -import typing - from app.actor import Actor from app.database import tables from app.exceptions import PermissionDeniedError -from app.repositories.chat_members_repository import ChatMembersRepository from app.repositories.messages_repository import MessagesRepository async def fetch_message_for_author( *, messages_repository: MessagesRepository, - chat_members_repository: ChatMembersRepository, actor: Actor, message_id: int, action: str, @@ -18,15 +14,15 @@ async def fetch_message_for_author( """Look up a message and authorize `actor` to act on it as its author. Shared by EditMessageUseCase and DeleteMessageUseCase so the check ordering is defined in - exactly one place: existence (get_one raises NotFoundError -> 404), then chat membership + exactly one place: existence (fetch_with_membership raises NotFoundError -> 404), then chat membership (-> 403), then authorship (-> 403). Membership is checked even though a non-author is already refused by the authorship check below - mirroring FetchMessagesUseCase's and FetchChatUseCase's membership-first posture keeps this consistent with the rest of the codebase rather than leaving message mutation as the one actor-scoped use case that never reconfirms the actor still belongs to the chat. """ - message: typing.Final = await messages_repository.get_one(id=message_id) - if not await chat_members_repository.is_member(message.chat_id, actor.id): + message, is_member = await messages_repository.fetch_with_membership(message_id, actor.id) + if not is_member: msg = "Not a member of this chat" raise PermissionDeniedError(msg) if message.user_id != actor.id: diff --git a/app/use_cases/register_user.py b/app/use_cases/register_user.py index 4684b47..5f5a049 100644 --- a/app/use_cases/register_user.py +++ b/app/use_cases/register_user.py @@ -22,7 +22,8 @@ async def __call__(self, *, data: RegisterRequest) -> tables.UsersTable: username=data.username, password_hash=security.hash_password(data.password), display_name=data.display_name, - ) + ), + auto_refresh=False, ) await self.transaction.commit() return user diff --git a/tests/conftest.py b/tests/conftest.py index dc087f5..edbe170 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -46,6 +46,8 @@ async def rolled_back_db_session(di_container: modern_di.Container) -> collectio connection = await engine.connect() transaction = await connection.begin() di_container.override(ioc.Database.database_engine, connection) + # Switching the shared connection to AUTOCOMMIT would roll back the outer transaction. + di_container.override(ioc.Database.autocommit_isolation_level, None) try: yield create_session(connection) @@ -63,6 +65,7 @@ async def rolled_back_db_session(di_container: modern_di.Container) -> collectio await connection.close() await engine.dispose() di_container.reset_override(ioc.Database.database_engine) + di_container.reset_override(ioc.Database.autocommit_isolation_level) @pytest.fixture diff --git a/tests/use_cases/test_connection_loss.py b/tests/use_cases/test_connection_loss.py index fb98a67..b493431 100644 --- a/tests/use_cases/test_connection_loss.py +++ b/tests/use_cases/test_connection_loss.py @@ -71,14 +71,15 @@ async def lost_pooled_connection(request_container: modern_di.Container) -> None await _terminate_backend(engine, *watched) -READ_USE_CASES: typing.Final = pytest.mark.parametrize( +_READ_CASES: typing.Final = [ + (ioc.UseCases.authenticate_user_use_case, {"username": "nobody", "password": "hunter2hunter2"}, None), + (ioc.UseCases.fetch_chats_use_case, {"actor": UNKNOWN_ACTOR}, []), + (ioc.UseCases.fetch_user_use_case, {"actor": UNKNOWN_ACTOR}, None), +] +READ_USE_CASES: typing.Final = pytest.mark.parametrize(("provider", "kwargs", "expected"), _READ_CASES) +READ_USE_CASES_WITH_REVOCATION_CHECK: typing.Final = pytest.mark.parametrize( ("provider", "kwargs", "expected"), - [ - (ioc.UseCases.authenticate_user_use_case, {"username": "nobody", "password": "hunter2hunter2"}, None), - (ioc.UseCases.check_token_revoked_use_case, {"jti": "unknown"}, False), - (ioc.UseCases.fetch_chats_use_case, {"actor": UNKNOWN_ACTOR}, []), - (ioc.UseCases.fetch_user_use_case, {"actor": UNKNOWN_ACTOR}, None), - ], + [*_READ_CASES, (ioc.UseCases.check_token_revoked_use_case, {"jti": "unknown"}, False)], ) @@ -95,7 +96,7 @@ async def test_read_use_case_recovers_from_a_lost_connection( @pytest.mark.usefixtures("lost_pooled_connection") -@READ_USE_CASES +@READ_USE_CASES_WITH_REVOCATION_CHECK async def test_read_use_case_recovers_from_a_connection_lost_while_pooled( request_container: modern_di.Container, provider: modern_di.providers.Factory[typing.Any],