From 19dbd32238c2400dc88644fa4041b3421d268137 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 13:56:36 +0300 Subject: [PATCH] fix: keep card updates inside the deck and stop PUT from inserting --- AGENTS.md | 2 +- app/api/cards.py | 3 ++ app/repositories.py | 30 +++++++++++--------- app/use_cases/update_cards.py | 2 +- tests/test_cards.py | 53 +++++++++++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4a36608..c8f6a91 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,7 +58,7 @@ class ListDecksUseCase: - `CreateCardsUseCase` relies on the `(deck_id, front)` unique constraint. A replay is rejected as a duplicate-key error rather than creating the cards twice. - Updates (`UpdateDeckUseCase`, `UpdateCardsUseCase`) write the same values again, so a replay is harmless. -**Persistence**: Models inherit `advanced_alchemy.base.BigIntAuditBase` (gives `id: BigInt`, `created_at`, `updated_at`). Metadata is shared with `orm.DeclarativeBase.metadata` in `app/models.py` so Alembic autogen sees everything. Repositories are `SQLAlchemyAsyncRepositoryService[Model]` with a nested `BaseRepository(SQLAlchemyAsyncRepository[Model])`. Use cases call the service methods (`get_many`, `get_one`, `create_many`) for reads and plain inserts. Writes that advanced-alchemy would spread over several round trips are one statement on the session instead: its `update` reads the row twice and refreshes it after the flush, and `upsert_many` selects the rows before and after writing them. `update_deck` sends `UPDATE ... RETURNING`, and `get_or_create` and `upsert_cards` send `INSERT ... ON CONFLICT ... RETURNING`. A statement that can hit a unique constraint runs inside `wrap_sqlalchemy_exception` so that it still raises `DuplicateKeyError`. `create_session` in `app/resources/db.py` builds a plain `AsyncSession` with `join_transaction_mode="create_savepoint"`. This is inert in production (the session binds to an engine) but enables the per-test rollback fixture: when a test binds the session to a connection already in a transaction, the session owns its own savepoint so the outer transaction survives commits. Do not "fix" it. +**Persistence**: Models inherit `advanced_alchemy.base.BigIntAuditBase` (gives `id: BigInt`, `created_at`, `updated_at`). Metadata is shared with `orm.DeclarativeBase.metadata` in `app/models.py` so Alembic autogen sees everything. Repositories are `SQLAlchemyAsyncRepositoryService[Model]` with a nested `BaseRepository(SQLAlchemyAsyncRepository[Model])`. Use cases call the service methods (`get_many`, `get_one`, `create_many`) for reads and plain inserts. Writes that advanced-alchemy would spread over several round trips are one statement on the session instead: its `update` reads the row twice and refreshes it after the flush, and `upsert_many` selects the rows before and after writing them. `update_deck` sends `UPDATE ... RETURNING` and `get_or_create` sends `INSERT ... ON CONFLICT ... RETURNING`. `update_cards` sends one `UPDATE cards ... FROM (VALUES ...) WHERE cards.id = updates.id AND cards.deck_id = :deck_id RETURNING *` and raises `NotFoundError` (404) if fewer rows come back than ids were sent, which rolls the whole request back. PUT never inserts. An `INSERT ... ON CONFLICT (id)` upsert would move a card in from another deck and would insert client-chosen ids without advancing `cards_id_seq`, so a later POST would collide on the primary key. The `update_cards` handler rejects duplicate ids with a 400 before any query runs. A statement that can hit a unique constraint runs inside `wrap_sqlalchemy_exception` so that it still raises `DuplicateKeyError`. `create_session` in `app/resources/db.py` builds a plain `AsyncSession` with `join_transaction_mode="create_savepoint"`. This is inert in production (the session binds to an engine) but enables the per-test rollback fixture: when a test binds the session to a connection already in a transaction, the session owns its own savepoint so the outer transaction survives commits. Do not "fix" it. **Test isolation** (`tests/conftest.py`): `db_session` fixture opens a connection, starts a transaction, then **overrides** `Database.dynamic_engine` in the DI container to return that connection. All requests in the test reuse this connection. Each session built against it uses `join_transaction_mode="create_savepoint"`, so `transaction.commit()` releases the session's own savepoint while the outer transaction is rolled back in teardown. DB state is clean between tests with no truncation needed. `app` and `client` fixtures build the real app and run it through `httpx.ASGITransport` + `asgi_lifespan.LifespanManager`. Polyfactory `SQLAlchemyFactory` is wired up via `set_async_session_in_base_sqlalchemy_factory`. diff --git a/app/api/cards.py b/app/api/cards.py index 6633190..4272a88 100644 --- a/app/api/cards.py +++ b/app/api/cards.py @@ -2,6 +2,7 @@ import litestar from litestar.di import NamedDependency +from litestar.exceptions import ValidationException from litestar.params import FromPath from app import schemas @@ -36,6 +37,8 @@ async def update_cards( data: list[schemas.CardUpdate], update_cards_use_case: NamedDependency[UpdateCardsUseCase], ) -> schemas.Cards: + if len({card.id for card in data}) != len(data): + raise ValidationException(detail="card ids must be unique") return schemas.Cards.from_models(await update_cards_use_case(deck_id=deck_id, data=data)) diff --git a/app/repositories.py b/app/repositories.py index 7f0cf21..1cf14ad 100644 --- a/app/repositories.py +++ b/app/repositories.py @@ -62,20 +62,24 @@ async def list_for_deck(self, deck_id: int) -> Sequence[models.Card]: async def add_cards(self, deck_id: int, cards: list[schemas.CardCreate]) -> Sequence[models.Card]: return await self.create_many([models.Card(**card.model_dump(), deck_id=deck_id) for card in cards]) - async def upsert_cards(self, deck_id: int, cards: list[schemas.CardUpdate]) -> Sequence[models.Card]: + async def update_cards(self, deck_id: int, cards: list[schemas.CardUpdate]) -> Sequence[models.Card]: if not cards: return [] - statement = postgresql.insert(models.Card).values([{**card.model_dump(), "deck_id": deck_id} for card in cards]) - statement = statement.on_conflict_do_update( - index_elements=[models.Card.id], - set_={ - "front": statement.excluded.front, - "back": statement.excluded.back, - "hint": statement.excluded.hint, - "deck_id": statement.excluded.deck_id, - "updated_at": sa.func.now(), - }, + updates: typing.Final = sa.values( + sa.column("id", sa.BigInteger), + sa.column("front", sa.String), + sa.column("back", sa.String), + sa.column("hint", sa.String), + name="updates", + ).data([(card.id, card.front, card.back, card.hint) for card in cards]) + statement: typing.Final = ( + sa.update(models.Card) + .where(models.Card.id == updates.c.id, models.Card.deck_id == deck_id) + .values(front=updates.c.front, back=updates.c.back, hint=updates.c.hint, updated_at=sa.func.now()) + .returning(models.Card) ) with wrap_sqlalchemy_exception(error_messages=DEFAULT_ERROR_MESSAGE_TEMPLATES, dialect_name="postgresql"): - upserted: typing.Final = (await self.repository.session.scalars(statement.returning(models.Card))).all() - return sorted(upserted, key=operator.attrgetter("id")) + updated: typing.Final = (await self.repository.session.scalars(statement)).all() + if len(updated) != len(cards): + raise NotFoundError + return sorted(updated, key=operator.attrgetter("id")) diff --git a/app/use_cases/update_cards.py b/app/use_cases/update_cards.py index 291a893..f3cb7ff 100644 --- a/app/use_cases/update_cards.py +++ b/app/use_cases/update_cards.py @@ -16,6 +16,6 @@ class UpdateCardsUseCase: @postgres_retry async def __call__(self, *, deck_id: int, data: list[schemas.CardUpdate]) -> Sequence[models.Card]: async with self.transaction: - cards: typing.Final = await self.cards_repository.upsert_cards(deck_id, data) + cards: typing.Final = await self.cards_repository.update_cards(deck_id, data) await self.transaction.commit() return cards diff --git a/tests/test_cards.py b/tests/test_cards.py index e162e08..3d9e4bc 100644 --- a/tests/test_cards.py +++ b/tests/test_cards.py @@ -188,3 +188,56 @@ async def test_update_cards_to_a_duplicate_front(client: AsyncClient) -> None: response = await client.put(f"/api/decks/{deck.id}/cards/", json=[{"id": card2.id, "front": card1.front}]) assert response.status_code == status_codes.HTTP_400_BAD_REQUEST + + +async def test_update_cards_does_not_move_a_card_from_another_deck( + client: AsyncClient, db_session: AsyncSession +) -> None: + deck, other_deck = await factories.DeckModelFactory.create_batch_async(size=2) + card = await factories.CardModelFactory.create_async(deck_id=other_deck.id) + + response = await client.put(f"/api/decks/{deck.id}/cards/", json=[{"id": card.id, "front": "moved"}]) + assert response.status_code == status_codes.HTTP_404_NOT_FOUND + stored = ( + await db_session.execute(sa.select(models.Card.deck_id, models.Card.front).where(models.Card.id == card.id)) + ).one() + assert stored == (other_deck.id, card.front) + + +async def test_update_cards_with_an_unknown_id_inserts_nothing(client: AsyncClient, db_session: AsyncSession) -> None: + deck = await factories.DeckModelFactory.create_async() + card = await factories.CardModelFactory.create_async(deck_id=deck.id) + + response = await client.put(f"/api/decks/{deck.id}/cards/", json=[{"id": card.id + 1, "front": "via put"}]) + assert response.status_code == status_codes.HTTP_404_NOT_FOUND + assert (await db_session.scalars(sa.select(models.Card.front).where(models.Card.deck_id == deck.id))).all() == [ + card.front + ] + + response = await client.post(f"/api/decks/{deck.id}/cards/", json=[{"front": "via post"}]) + assert response.status_code == status_codes.HTTP_201_CREATED + + +async def test_update_cards_with_one_unknown_id_updates_nothing(client: AsyncClient, db_session: AsyncSession) -> None: + deck = await factories.DeckModelFactory.create_async() + card = await factories.CardModelFactory.create_async(deck_id=deck.id) + + response = await client.put( + f"/api/decks/{deck.id}/cards/", + json=[{"id": card.id, "front": "updated"}, {"id": card.id + 1, "front": "unknown"}], + ) + assert response.status_code == status_codes.HTTP_404_NOT_FOUND + assert (await db_session.scalars(sa.select(models.Card.front).where(models.Card.id == card.id))).one() == card.front + + +async def test_update_cards_rejects_duplicate_ids(client: AsyncClient, statements: list[str]) -> None: + deck = await factories.DeckModelFactory.create_async() + card = await factories.CardModelFactory.create_async(deck_id=deck.id) + statements.clear() + + response = await client.put( + f"/api/decks/{deck.id}/cards/", + json=[{"id": card.id, "front": "first"}, {"id": card.id, "front": "second"}], + ) + assert response.status_code == status_codes.HTTP_400_BAD_REQUEST + assert statements == []